Replace use of HolderSignedTx with HolderCommitment #3664
Conversation
When establishing a channel, the funding transaction may be replaced either: - after the funding transaction has confirmed using splicing, - before the funding transaction has confirmed for v2 channel establishment using `tx_init_rbf`, or - before the splice's funding transaction has confirmed using `tx_init_rbf`. In each of these cases, fields in the `ChannelMonitor` will need to be updated once the new funding transaction confirms. This commit introduces a `FundingScope` to hold the aforementioned fields, allowing to swap in another `FundingScope` when necessary. This is the same abstraction as in `channel.rs` representing a different set of data.
The `ChannelTransactionParameters` will change across `FundingScope`s, so we make sure to track it to ensure it is updated accordingly when a new `FundingScope` is applied/considered. Along the way, we also clean up some unnecessary function arguments to `ChannelMonitor::new` that we can just obtain from the `ChannelTransactionParameters` already provided.
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
| @@ -3531,23 +3531,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider { | |||
| return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_stats.num_nondust_htlcs))); | |||
| } | |||
|
|
|||
| // Up to LDK 0.0.115, HTLC information was required to be duplicated in the | |||
There was a problem hiding this comment.
Hmm, did we fuck this up? Is there no way to indicate to old LDK to decline to decode the update now that its no longer actually compatible? At a minimum we need release notes but kinda annoying that we'll just silently corrupt state if a user downgrades to 0.0.115.
There was a problem hiding this comment.
Yeah... I think we would've needed to add an optional even TLV back then that only gets set when the sources are provided separately. Release notes will have to do for now.
lightning/src/ln/channel.rs
Outdated
| if let Some(source) = source_opt.take() { | ||
| nondust_htlc_sources.push(source); | ||
| } else { | ||
| debug_assert!(false, "Missing outbound HTLC source"); |
There was a problem hiding this comment.
This should probably be a hard assert, right? And also probably we should change the types to avoid it?
There was a problem hiding this comment.
Change the types on what? The source still needs to be optional in case the HTLC is not offered by the holder.
| PackageSolvingData::HolderHTLCOutput(htlc_output), | ||
| counterparty_spendable_height, | ||
| ); | ||
| claim_requests.push(htlc_package); | ||
| } else { | ||
| debug_assert!(false, "Expected transaction output index for non-dust HTLC"); |
There was a problem hiding this comment.
Ugh, we really need to change the types on htlc info so that we don't have these checks everywhere...if we're gonna separate tracking of dust from non-dust we should use different types...
There was a problem hiding this comment.
Yeah, unfortunately there are still areas where we track them both together in a single Vec, so I don't think we can get there quite yet as of this PR.
| writer.write_all(&[1; 1])?; | ||
| prev_holder_tx.write(writer)?; | ||
| HolderSignedTx::from(prev_holder_commitment).write(writer)?; |
There was a problem hiding this comment.
Bleh, can we just implement a write_as_legacy?
There was a problem hiding this comment.
Just to avoid the from magic? Because we still need that further below to make sure the HolderCommitmentTransaction we got from the OnchainTxHandler maps to the HolderSignedTx we read.
| let holder_commitment = HolderCommitment::try_from(( | ||
| current_holder_commitment_tx, ¤t_holder_signed_tx, | ||
| )).map_err(|_| DecodeError::InvalidValue)?; | ||
| if HolderSignedTx::from(&holder_commitment) != current_holder_signed_tx { |
There was a problem hiding this comment.
What's gonna happen here? Are we just gonna keep writing the legacy versions and converting, or should we have some upgrade path?
There was a problem hiding this comment.
Was planning to address this in a follow-up PR that removes them from OnchainTxHandler such that we can no longer rely on getting them from there and instead store them as a new TLV in the monitor.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
|
This does seem like the kind of thing that might want an 👋 upgrade test 👋 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3664 +/- ##
==========================================
- Coverage 89.23% 89.19% -0.05%
==========================================
Files 155 155
Lines 119327 119447 +120
Branches 119327 119447 +120
==========================================
+ Hits 106482 106536 +54
- Misses 10242 10299 +57
- Partials 2603 2612 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Will need to wait for #3584 first |
Currently, non-dust HTLCs are duplicated across the commitment transaction itself, and the full set of HTLCs (dust & non-dust) along with their `HTLCSource` considered in the commitment transaction. As of v0.0.15, we've had support for providing non-dust HTLC sources separately such that we no longer track duplicate non-dust HTLC data, but only enabled it under testing environments. This commit enables it such that it always happens. Note that we still need to support reading `ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo` updates that did not separate the non-dust HTLC sources in case they were written in an older version and they've yet to be processed.
As we move to a custom commitment transactions world, we'll no longer be able to rely on `HolderSignedTx` and should instead track the actual commitment transaction itself. This commit does so by introducing a new `HolderCommitment` struct, which we'll use as a replacement to `HolderSignedTx` to hold all relevant holder commitment data, including the `HolderCommitmentTransaction`, without data duplication. Luckily, this is a backwards and forwards compatible change due to the `OnchainTxHandler` tracking the latest `HolderCommitmentTransaction`s. In the future, we aim to remove them from the `OnchainTxHandler` entirely and begin storing them at the `ChannelMonitor` level. Aside from how the data is represented internally, there shouldn't be any functional changes within this patch.
wpaulino
force-pushed
the
deprecate-holder-signed-tx
branch
from
2884bad to
fdc1706
Compare
As we move to a custom commitment transactions world, we'll no longer be able to rely on
HolderSignedTxand should instead track the actual commitment transaction itself. This commit does so by introducing a newHolderCommitmentstruct, which we'll use as a replacement toHolderSignedTxto hold all relevant holder commitment data, including theHolderCommitmentTransaction, without data duplication.Luckily, this is a backwards and forwards compatible change due to the
OnchainTxHandlertracking the latestHolderCommitmentTransactions. In the future, we aim to remove them from theOnchainTxHandlerentirely and begin storing them at theChannelMonitorlevel.Aside from how the data is represented internally, there shouldn't be any functional changes within this patch.
Depends on #3663.