Introduce FundingScope abstraction to ChannelMonitor #3663
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3663 +/- ##
==========================================
- Coverage 89.23% 89.20% -0.04%
==========================================
Files 155 155
Lines 119327 119357 +30
Branches 119327 119357 +30
==========================================
- Hits 106482 106469 -13
- Misses 10242 10284 +42
- Partials 2603 2604 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
wpaulino
force-pushed
the
channel-monitor-funding-scope
branch
from
114340b to
8b47754
Compare
| /// The set of outpoints in each counterparty commitment transaction. We always need at least | ||
| /// the payment hash from `HTLCOutputInCommitment` to claim even a revoked commitment | ||
| /// transaction broadcast as we need to be able to construct the witness script in all cases. | ||
| counterparty_claimable_outpoints: HashMap<Txid, Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>>, |
There was a problem hiding this comment.
Hmm, is there a way to avoid duplicating these for all state updates while a splice is pending? In theory it should be short but it seems like a DoS issue if someone can get 100 splices in-flight at once and then never confirm any and our memory usage goes up 100x.
There was a problem hiding this comment.
Good call, we're definitely gonna need a new way of tracking these to avoid that. We could keep them at the ChannelMonitorImpl level and index them by the counterparty commitment number (since all scopes will be at the same commitment number). Then, we add a new map in FundingScope that maps txid -> commitment number.
Is there a reason for us using the txid over the commitment number in current main?
There was a problem hiding this comment.
Hm, doing this will actually break the transaction_output_index, they are not guaranteed to remain consistent across all commitments/scopes.
There was a problem hiding this comment.
It does seem like "HTLCOutputInCommiemnt's output_index is not like the other fields" keeps coming up....maybe time to finally refactor the type so that output_index is separate (cc @tankyleo)
There was a problem hiding this comment.
yes I can certainly take a look. Hopefully this allows us to delete the brute force search we talked about last week, Matt, to populate the output indices.
There was a problem hiding this comment.
Added a TODO(splicing) to make sure we don't miss this.
| channel_value_satoshis, channel_keys_id, destination_script.into(), keys, | ||
| channel_parameters.clone(), initial_holder_commitment_tx, secp_ctx | ||
| channel_parameters.channel_value_satoshis, channel_keys_id, destination_script.into(), | ||
| keys, channel_parameters.clone(), initial_holder_commitment_tx, secp_ctx |
There was a problem hiding this comment.
Will OnchainTxHandler hold parameters for each funding scope?
Unrelated to this PR, but I wonder if we need to write channel_parameters twice if we can pass it to OnchainTxHandler on read.
There was a problem hiding this comment.
No, I plan to move all channel-specific data from OnchainTxHandler to ChannelMonitor, and provide it as needed to OnchainTxHandler via the outputs API.
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.
Yeah it will happen in the PR introducing splice monitor updates. |
wpaulino
force-pushed
the
channel-monitor-funding-scope
branch
from
8b47754 to
6097917
Compare
When establishing a channel, the funding transaction may be replaced either:
tx_init_rbf, ortx_init_rbf.In each of these cases, fields in the
ChannelMonitorwill need to be updated once the new funding transaction confirms. This commit introduces aFundingScopeto hold the aforementioned fields, allowing to swap in anotherFundingScopewhen necessary.This is the same abstraction as in
channel.rsrepresenting a different set of data.