Remove data dependency on OnchainTxHandler from onchain claims #3690
Conversation
This file contains 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
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3690 +/- ##
==========================================
- Coverage 89.05% 89.04% -0.02%
==========================================
Files 155 155
Lines 122019 122214 +195
Branches 122019 122214 +195
==========================================
+ Hits 108666 108820 +154
- Misses 10695 10721 +26
- Partials 2658 2673 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 1st Reminder Hey @joostjager! This PR has been waiting for your review. |
TheBlueMatt
reviewed
Mar 31, 2025
|
👋 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. |
TheBlueMatt
reviewed
Mar 31, 2025
wpaulino
force-pushed
the
onchain-tx-handler-serialization-revamp
branch
from
07535a8 to
97d6278
Compare
TheBlueMatt
reviewed
Apr 2, 2025
wpaulino
force-pushed
the
onchain-tx-handler-serialization-revamp
branch
from
97d6278 to
6f8f853
Compare
|
Looks like this needs rebase already :( |
TheBlueMatt
previously approved these changes
Apr 3, 2025
wpaulino
force-pushed
the
onchain-tx-handler-serialization-revamp
branch
2 times, most recently
from
63dd53b to
57e774c
Compare
|
Addressed all pending feedback and rebased due to a minor conflict. |
|
I'll wait for another reviewer to approve this to look again, I think it was fine. |
wpaulino
force-pushed
the
onchain-tx-handler-serialization-revamp
branch
from
57e774c to
fff884f
Compare
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
tankyleo
reviewed
Apr 7, 2025
wpaulino
force-pushed
the
onchain-tx-handler-serialization-revamp
branch
from
fff884f to
2fab513
Compare
The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. This commit removes the internal usage of `OnchainTxHandler::destination_script` and deprecates it, opting to instead provide it as a function argument wherever needed.
The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. Once splices are supported, we may run into cases where we are attempting to claim an output from a specific `FundingScope`, while also having an additional pending `FundingScope` for a splice. If the pending splice confirms over the output claim, we need to cancel the claim and re-offer it with the set of relevant parameters in the new `FundingScope`. This commit tracks the `HolderCommitmentTransaction` for the specific `FundingScope` the `HolderFundingOutput` claim originated from. This allows us to remove the dependency on `OnchainTxHandler` when obtaining the current `HolderCommitmentTransaction` and ensures any alternative commitment transaction state due to splicing does not leak into the `OnchainTxHandler`. As a result, we can now include all the information required to emit a `BumpTransactionEvent::ChannelClose` within its intermediate representation `ClaimEvent::BumpCommitment`, as opposed to relying on the `ChannelMonitor` to provide it. Along the way, this commit also fixes a bug where `BumpTransactionEvent::ChannelClose` could be emitted with an unsigned commitment transaction, resulting in the child transaction not being broadcastable due to a non-existent ancestor. This isn't a huge deal as we have retry logic for such claims; once the signer returns a valid signature, the event is re-emitted properly.
The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. Once splices are supported, we may run into cases where we are attempting to claim an output from a specific `FundingScope`, while also having an additional pending `FundingScope` for a splice. If the pending splice confirms over the output claim, we need to cancel the claim and re-offer it with the set of relevant parameters in the new `FundingScope`. This commit tracks the `ChannelTransactionParameters` for the specific `FundingScope` the `HolderFundingOutput` claim originated from. This allows us to remove the dependency on `OnchainTxHandler` when obtaining the current `ChannelTransactionParameters` and ensures any alternative state due to splicing does not leak into the `OnchainTxHandler`.
The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. This commit tracks the `HTLCDescriptor` for the specific `FundingScope` the `HolderHTLCOutput` claim originated from. Previously, when claiming `HolderHTLCOutput`s, the `OnchainTxHandler` had to check which holder commitment the HTLC belonged to in order to produce an `HTLCDescriptor` for signing. We still maintain the legacy logic for existing claims which have not had an `HTLCDescriptor` written yet. Along the way, we refactor the claim process for such outputs to decouple them from the `OnchainTxHandler`. As a result, the `OnchainTxHandler` is only used to obtain references to the signer and secp256k1 context. Once splices are supported, we may run into cases where we are attempting to claim an output from a specific `FundingScope`, while also having an additional pending `FundingScope` for a splice. If the pending splice confirms over the output claim, we need to cancel the claim and re-offer it with the set of relevant parameters in the new `FundingScope`.
The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. Once splices are supported, we may run into cases where we are attempting to claim an output from a specific `FundingScope`, while also having an additional pending `FundingScope` for a splice. If the pending splice confirms over the output claim, we need to cancel the claim and re-offer it with the set of relevant parameters in the new `FundingScope`. This commit tracks the `ChannelTransactionParameters` for the specific `FundingScope` the `CounterpartyReceivedHTLCOutput` claim originated from. This allows us to remove the dependency on `OnchainTxHandler` when obtaining the current `ChannelTransactionParameters` and ensures any alternative state due to splicing does not leak into the `OnchainTxHandler`.
The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. Once splices are supported, we may run into cases where we are attempting to claim an output from a specific `FundingScope`, while also having an additional pending `FundingScope` for a splice. If the pending splice confirms over the output claim, we need to cancel the claim and re-offer it with the set of relevant parameters in the new `FundingScope`. This commit tracks the `ChannelTransactionParameters` for the specific `FundingScope` the `CounterpartyOfferedHTLCOutput` claim originated from. This allows us to remove the dependency on `OnchainTxHandler` when obtaining the current `ChannelTransactionParameters` and ensures any alternative state due to splicing does not leak into the `OnchainTxHandler`.
The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. Once splices are supported, we may run into cases where we are attempting to claim an output from a specific `FundingScope`, while also having an additional pending `FundingScope` for a splice. If the pending splice confirms over the output claim, we need to cancel the claim and re-offer it with the set of relevant parameters in the new `FundingScope`. This commit tracks the `ChannelTransactionParameters` for the specific `FundingScope` the `RevokedHTLCOutput` claim originated from. This allows us to remove the dependency on `OnchainTxHandler` when obtaining the current `ChannelTransactionParameters` and ensures any alternative state due to splicing does not leak into the `OnchainTxHandler`.
The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. Once splices are supported, we may run into cases where we are attempting to claim an output from a specific `FundingScope`, while also having an additional pending `FundingScope` for a splice. If the pending splice confirms over the output claim, we need to cancel the claim and re-offer it with the set of relevant parameters in the new `FundingScope`. This commit tracks the `ChannelTransactionParameters` for the specific `FundingScope` the `RevokedOutput` claim originated from. This allows us to remove the dependency on `OnchainTxHandler` when obtaining the current `ChannelTransactionParameters` and ensures any alternative state due to splicing does not leak into the `OnchainTxHandler`.
The `ChannelMonitor` and `OnchainTxHandler` have historically been tied together, often tracking some of the same state twice. As we introduce support for splices in the `ChannelMonitor`, we'd like to avoid leaking some of those details to the `OnchainTxHandler`. Ultimately, the `OnchainTxHandler` should stand on its own and support claiming funds from multiple `ChannelMonitor`s, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels. This commit tracks the `ChannelTransactionParameters` for the current `FundingScope` of a `ChannelMonitor` and deprecates the one found in `OnchainTxHandler`.
wpaulino
force-pushed
the
onchain-tx-handler-serialization-revamp
branch
from
2fab513 to
9bd8557
Compare
tankyleo
reviewed
Apr 7, 2025
| transaction_parameters: self.funding.channel_parameters.clone(), | ||
| }, | ||
| outpoint: BitcoinOutPoint { | ||
| txid: commitment_txid, | ||
| vout: anchor_output_idx, | ||
| }, | ||
| }, | ||
| pending_htlcs, | ||
| pending_htlcs: pending_nondust_htlcs, |
There was a problem hiding this comment.
Making the names consistent is a short patch here - I'm ultimately undecided on pending_htlcs vs pending_nondust_htlcs, your call:
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index f709183f2..c101c48d2 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -3555,7 +3555,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
vout: anchor_output_idx,
},
},
- pending_htlcs: pending_nondust_htlcs,
+ pending_nondust_htlcs,
}));
},
ClaimEvent::BumpHTLC {
diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs
index a9550a672..ed09016e5 100644
--- a/lightning/src/events/bump_transaction.rs
+++ b/lightning/src/events/bump_transaction.rs
@@ -170,7 +170,7 @@ pub enum BumpTransactionEvent {
anchor_descriptor: AnchorDescriptor,
/// The set of pending HTLCs on the commitment transaction that need to be resolved once the
/// commitment transaction confirms.
- pending_htlcs: Vec<HTLCOutputInCommitment>,
+ pending_nondust_htlcs: Vec<HTLCOutputInCommitment>,
},
/// Indicates that a channel featuring anchor outputs has unilaterally closed on-chain by a
/// holder commitment transaction and its HTLC(s) need to be resolved on-chain. With the
@@ -963,7 +963,7 @@ mod tests {
},
outpoint: OutPoint { txid: Txid::from_byte_array([42; 32]), vout: 0 },
},
- pending_htlcs: Vec::new(),
+ pending_nondust_htlcs: Vec::new(),
});
}
}There was a problem hiding this comment.
pending_htlcs given the context seems good enough because you can only enforce non-dust HTLCs onchain.
tankyleo
reviewed
Apr 7, 2025
Comment on lines
181
to
+185
| commitment_tx: Transaction, | ||
| commitment_tx_fee_satoshis: u64, | ||
| pending_nondust_htlcs: Vec<HTLCOutputInCommitment>, | ||
| anchor_output_idx: u32, | ||
| channel_parameters: ChannelTransactionParameters, |
There was a problem hiding this comment.
We could squash Transaction, Vec<HTLCOutputInCommitment> ChannelTransactionParameters into as single field CommitmentTransaction ?
There was a problem hiding this comment.
ah not quite - ChannelTransactionParameters is not stored in CommitmentTransaction - maybe not worth it then.
There was a problem hiding this comment.
Yeah not much further grouping we can do here without introducing new structs, and that doesn't seem worth it given this is just an internal intermediate representation of BumpTransactionEvent::ChannelClose.
jkczyz
approved these changes
Apr 7, 2025
TheBlueMatt
approved these changes
Apr 8, 2025
| @@ -3967,14 +3951,50 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> { | |||
| (claimable_outpoints, outputs_to_watch) | |||
| } | |||
|
|
|||
| fn get_broadcasted_holder_htlc_descriptors( | |||
| &self, holder_tx: &HolderCommitmentTransaction, | |||
| ) -> Vec<HTLCDescriptor> { | |||
There was a problem hiding this comment.
This should return an impl Iterator since the callers just convert the vec into an iterator anyway.
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.
The
ChannelMonitorandOnchainTxHandlerhave historically been tied together, often tracking some of the same state twice. As we introduce support for splices in theChannelMonitor, we'd like to avoid leaking some of those details to theOnchainTxHandler. Ultimately, theOnchainTxHandlershould stand on its own and support claiming funds from multipleChannelMonitors, allowing us to save on fees by batching aggregatable claims across multiple in-flight closing channels.Once splices are supported, we may run into cases where we are attempting to claim an output from a specific
FundingScope, while also having an additional pendingFundingScopefor a splice. If the pending splice confirms over the output claim, we need to cancel the claim and re-offer it with the set of relevant parameters in the newFundingScope.This PR features a series of commits decoupling the data dependency on the
OnchainTxHandlerfrom onchain claims, opting to store any data required for the claim within its set of TLVs.