Fix plain channel interoperability with stock LDK - #36
Open
bitwalt wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #35.
Makes the two RGB wire/transaction extensions conditional, so plain channels are byte-identical to upstream LDK while RGB channels are unaffected.
Commitment transaction fee
commitment_tx_base_weightgoes back to upstream, and the OP_RETURN weight is added only when the channel is colored.SpecTxBuildercarries that flag, taken from thergb_assetalready onFundingScope, so every fee calculation on a channel agrees with the transaction actually built for it.The constant and the colored fee helper live in
rgb_utils, next to the rest of the RGB logic, sochan_utilsandtx_builderonly wire it in. The weight is documented where it is defined: 8-byte value, 1-byte script length and a 34-byteOP_RETURN OP_PUSHBYTES_32 <commitment>script, so 43 vbytes, 172 weight units.Two details worth pointing out for review:
get_next_commitment_statsnow uses the samecommit_tx_fee_satasbuild_commitment_transaction. It previously called the free function directly, which was harmless while every path unconditionally included the OP_RETURN weight, but would have left colored channels predicting a fee 172 weight units below the one baked into their own commitment transaction.update_add_htlcrgb_paymentbecomes an optional odd TLV instead of a mandatory suffix, so peers without RGB support skip it.Open question: we chose an odd type outside the range the BOLTs may assign, following the convention already used here for
skimmed_fee_msat(65537) andhold_htlc(75537), where the TODO abovehold_htlcspells out the reasoning. If RGB has a preferred allocation, or you would rather this be specified somewhere first, say which type and we will match it — it is a one-line change.Validation
Verified at the node level rather than here, since that is where a stock peer can actually be driven: RGB-Tools/rgb-lightning-node#149 adds a stock
ldk-nodefixture and exercises the full lifecycle in both opener directions —channel_ready, a payment each way, balances moving as expected, cooperative close settling on-chain — plus a negative test asserting an RGB channel to a stock peer never becomes ready. All three pass with this commit and fail without it.Compatibility
Breaking on the wire for deployed nodes: plain channels between a fixed and an unfixed peer derive different commitment transactions and force-close on the next
commitment_signed, and RGB channels cannot parse each other's HTLCs across the encoding change. Persisted channel state is unaffected, so restarting an upgraded node is safe, but a release carrying this wants a coordinated upgrade.