Post-release updates for v26.06#9223
Conversation
f8691ab to
d3b3404
Compare
|
All tests passed. Just waiting for another review to merge it. |
| > [!IMPORTANT] | ||
| > | ||
| > 26.06 FREEZE April 30th: Non-bugfix PRs not ready by this date will wait for 26.09. | ||
| > 26.09 FREEZE August 5th: Non-bugfix PRs not ready by this date will wait for 26.12. |
There was a problem hiding this comment.
nit: It would be good to split this large commit out into smaller commits as done in this PR here: https://github.com/ElementsProject/lightning/pull/9051/commits. This commit is doing too many things at once, some of those commits may also need updated changelogs.
There was a problem hiding this comment.
I hope it's better now. I don't think I can divide it much more than this...
d3b3404 to
31c59a4
Compare
Co-Authored-By: Madeline <madeline@blockstream.com> Changelog-None
Rename splice→splice_init, relative_satoshis→funding_contribution_satoshis, txsigs_tlvs→tx_signatures_tlvs, and splice_info/batch_info→funding_txid/message_type. Consolidate three CLN-specific shim patches into extracted_peer_cln_batch_element.patch. Changelog-None
… if only peer Per BOLT #2: when both nodes set next_funding in channel_reestablish but txids differ, MUST send error and fail the channel; when only the peer sets next_funding, MUST send tx_abort instead. Track we_set_next_funding before tlvs is overwritten by the received channel_reestablish so the distinction survives the receive. Includes a regression test: reconnect two nodes with a corrupted inflight txid so both set next_funding with mismatched values, and verify both sides call open_err_fatal. Changelog-None
31c59a4 to
2e23291
Compare
|
I didn't get a chance to review this PR before it got merged. I'll make sure to either comment with an ack or ✅ the PR before its merged. |
I'm adding this to my performance review notes. |
There was a problem hiding this comment.
NACK because:
- Deviating from the spec isn't good
- We probably should be implementing tlv's we add instead of adding them without actually doing them
- I'm not a fan of some of the renaming.
- I don't understand why we're deleting patches.
I hope these changes didn't get bundled into a release, as this could absolutely break channels. The is_splice_active logic for channel_ready deviates from the spec.
It might be okay but this definitely is something that should be done with deep thought and as much testing as possible -- not lumped in with a bunch of rename refactoring.
| && recv_tlvs->my_current_funding_locked | ||
| && !bitcoin_txid_eq( | ||
| &recv_tlvs->my_current_funding_locked->my_current_funding_locked_txid, | ||
| &peer->channel->funding.txid)); |
There was a problem hiding this comment.
Not sure why this got merged in with release notes but this shouldn't be here
There was a problem hiding this comment.
This should have been rolled back by #9230 as per your comments. Must have slipped on the merge from master.
| msgdata,splice_init,locktime,u32, | ||
| msgdata,splice_init,funding_pubkey,point, | ||
| msgdata,splice_init,tlvs,splice_init_tlvs, | ||
| tlvtype,splice_init_tlvs,require_confirmed_inputs,2 |
There was a problem hiding this comment.
Was this feature implemented or just the tlv for it added?
| @@ -1,62 +0,0 @@ | |||
| diff --git b/wire/peer_wire.csv a/wire/peer_wire.csv | |||
There was a problem hiding this comment.
What's the reason for deleting these patches?
There was a problem hiding this comment.
The deleted patches were provisional overrides for pre-standard behavior. Once the spec merged those definitions into the BOLTs, the patches became redundant, and would break the extraction if left in.
| @@ -1,14 +0,0 @@ | |||
| diff --git b/wire/peer_wire.csv a/wire/peer_wire.csv | |||
| @@ -1,15 +0,0 @@ | |||
| --- wire/peer_exp_wire.csv 2022-06-22 19:07:24.000000000 -0500 | |||
| struct splice_ack { | ||
| struct channel_id channel_id; | ||
| s64 relative_satoshis; | ||
| s64 funding_contribution_satoshis; |
There was a problem hiding this comment.
Not a big fan of this name because the value is not a contribution. It can be positive or negative, hance the name relative.
There was a problem hiding this comment.
The name is correct as per the BOLT spec.
| struct fuzzsplice { | ||
| struct channel_id channel_id; | ||
| s64 relative_satoshis; | ||
| s64 funding_contribution_satoshis; |
There was a problem hiding this comment.
Don't like the name here either.
There was a problem hiding this comment.
This is exactly the name used on the BOLT specification, and since the wire messages are generated from the spec, this is the name we got.
| && peer->next_index[LOCAL] == 1 | ||
| && next_commitment_number == 1) { | ||
| && next_commitment_number == 1 | ||
| && !is_splice_active) { |
There was a problem hiding this comment.
This deviates from the spec
There was a problem hiding this comment.
This also should have been rolled back by #9230 as per your comments. Must have slipped on the merge from master.
|
I double checked the release and it looks like these changes didn't go into it, so that's good. Looks like perhaps the branches got mixed up? Definitely need to clean this up 🤔 |
|
These are the post-release chores. This will be included in the next release. |
|
@ddustin re your point on the The behavioural change is a single line, so the plan is to get the semantics reviewed properly, and if we can't converge before the v26.09 freeze, roll that one line back with a FIXME and keep the plumbing for the follow-up. Would appreciate your review. |
I'm proposing some changes to the Github ruleset to enforce an approving review, require checks to pass, and require the branch to be up to date with the approval re-confirmed after any update, so nobody can rebase-and-instantly-merge stale code like what happened. |
Love this idea 👍 |
Post-release updates for v26.06 per the Release Checklist Instructions
Changelog-None