Consider currently confirmed FundingScope when claiming commitments #3980
Conversation
|
👋 Thanks for assigning @tankyleo as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3980 +/- ##
==========================================
- Coverage 88.93% 88.91% -0.03%
==========================================
Files 174 174
Lines 124539 125065 +526
Branches 124539 125065 +526
==========================================
+ Hits 110758 111196 +438
- Misses 11287 11361 +74
- Partials 2494 2508 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
👋 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. |
wpaulino
force-pushed
the
splice-commitment-claims
branch
from
5ecc866 to
379edfc
Compare
wpaulino
force-pushed
the
splice-commitment-claims
branch
from
379edfc to
4e03e45
Compare
|
✅ Added second reviewer: @tankyleo |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generally fine, at least once the parent PR lands, but I'm still confused on #3980 (comment)
|
Needs rebase, also might be worth addressing #3939 (comment) here but up to you. |
wpaulino
force-pushed
the
splice-commitment-claims
branch
from
4e03e45 to
53e1c59
Compare
|
Oh, also we'll need to do something with |
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
It already does for
Given their current usage, it's not required to change them but definitely worth changing. I already have the change in a separate branch.
Already had this change as well, just didn't include it here. |
wpaulino
force-pushed
the
splice-commitment-claims
branch
from
53e1c59 to
e15e131
Compare
| if let Some((_, conf_height)) = self.alternative_funding_confirmed.as_ref() { | ||
| if *conf_height == height { | ||
| self.alternative_funding_confirmed.take(); | ||
| if self.holder_tx_signed { | ||
| if self.holder_tx_signed || self.funding_spend_seen { |
There was a problem hiding this comment.
When the alternative_funding_confirmed gets reorged out, in which cases would we not broadcast the holder commitment ?
There was a problem hiding this comment.
We could be lockdown_from_offchain from a ChannelForceClosed update where should_broadcast is false, but that means we never attempted to broadcast the funding transaction.
There was a problem hiding this comment.
Do we also not broadcast if we haven't given a holder tx to OnchainTxHandler AND we haven't seen a commitment transaction hit the chain ?
There was a problem hiding this comment.
Also if should_broadcast is false, then is_funding_broadcast back in channel is false, then not sure how alternative_funding_confirmed can be Some ?
There was a problem hiding this comment.
Not sure I'm following your line of questioning here. We only want to broadcast if we already did, or if we've seen a commitment transaction confirm. In any other cases, either we're not supposed to broadcast because we were told not to or alternative_funding_confirmed is not relevant at all and the code here is unreachable.
There was a problem hiding this comment.
Right ok thank you !
Want to confirm: if the funding gets reorged out, but we haven't signed a holder tx, and we haven't seen a commit tx hit the chain, we do NOT broadcast :)
|
Finished second pass |
We generally want to hard assert on cases that could cause us to lose money.
We can't rely waiting on another (or the same) renegotiated funding transaction to confirm, since it may never happen. We also don't want to rely on the counterparty to broadcast for us, or require manual intervention from the user, so we choose to broadcast the new holder commitment immediately. This ensures we're able to claim funds from an already force closed channel after an alternative funding reorg.
wpaulino
force-pushed
the
splice-commitment-claims
branch
from
e15e131 to
c5c1e22
Compare
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
wpaulino
force-pushed
the
splice-commitment-claims
branch
from
c5c1e22 to
110e424
Compare
Once a commitment transaction confirms, we may have outputs to claim. To determine whom the commitment transaction belongs to, we generally compare its `txid` against what we know be ours and the counterparty's. This, however, relies on being able to match on the expected `FundingScope`, such that we can produce the necessary output claims. Since a commitment transaction confirming implies that the funding transaction it spends has also confirmed, we rely on `alternative_funding_confirmed` to obtain the corresponding `FundingScope`.
It's best to let the caller decide what the appropriate `ChannelTransactionParameters` are as it has the most context.
Since there may be multiple counterparty commitment transactions for the same commitment number due to splicing, we have to locate the matching `FundingScope::channel_parameters` to provide the signer. Since this is intended to be called during `Persist::update_persisted_channel`, the monitor should have already had the update applied.
wpaulino
force-pushed
the
splice-commitment-claims
branch
from
110e424 to
775f3d9
Compare
Once a commitment transaction confirms, we may have outputs to claim. To determine whom the commitment transaction belongs to, we generally compare its
txidagainst what we know be ours and the counterparty's. This, however, relies on being able to match on the expectedFundingScope, such that we can produce the necessary output claims. Since a commitment transaction confirming implies that the funding transaction it spends has also confirmed, we rely onalternative_funding_confirmedto obtain the correspondingFundingScope.