Watch all outputs irrespective of claimable outpoints #3081
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #3081 +/- ##
==========================================
- Coverage 89.90% 89.88% -0.03%
==========================================
Files 117 117
Lines 97415 97406 -9
Branches 97415 97406 -9
==========================================
- Hits 87581 87550 -31
- Misses 7272 7285 +13
- Partials 2562 2571 +9 ☔ View full report in Codecov by Sentry. |
|
To fix testcases: This is because earlier we were only returning new_watched_outputs from block_confirmed/transactions_confirmed if no output existed against the txid. But we would still go ahead and replace watched_outputs against a txid. see: here In case where we see something like we would replace watched_outputs with o2, o3 but not return them from block_confirmed/transactions_confirmed. So after the fix in second commit, we will now watch all of o1, o2, o3. Not sure if this is the right fix but there is definitely something wrong here. Will fix the one remaining commented testcase which uses old_serialized monitor if this is the right fix. Ready for review! |
G8XSU
changed the title
| for (idx, outp) in tx.output.iter().enumerate() { | ||
| new_watch_outputs.push((idx as u32, outp.clone())); | ||
| } | ||
| watch_outputs.push((txid, new_watch_outputs)); |
There was a problem hiding this comment.
Discussed it offline, but this is currently also applying to holder commitment transactions, which causes issues in that its watching a different set. IMO we should consider only watching all outputs if the commitment transactions is unknown to us (or revoked-counterparty).
There was a problem hiding this comment.
in the latest patch, we check for check_spend_holder_transaction before check_spend_counterparty.
and we watch all outputs only if it is not a current/prev holder_tx.
G8XSU
force-pushed
the
2024-05-08-claimable-persist-3049-outputs
branch
3 times, most recently
from
b2decaf to
da226cc
Compare
G8XSU
dismissed
TheBlueMatt’s stale review
The merge-base changed after approval.
This removes dependency of watched_outputs from per_commitment_claimable_outpoints, it is required since we will no longer have direct access to per_commitment_claimable_outpoints once we start publishing PersistClaimInfo as part of lightningdevkit#3049.
G8XSU
force-pushed
the
2024-05-08-claimable-persist-3049-outputs
branch
from
da226cc to
149fa12
Compare
|
Rebased PR on main. |
| @@ -3252,7 +3250,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> { | |||
| ( $thing : expr ) => { | |||
| match $thing { | |||
| Ok(a) => a, | |||
| Err(_) => return (claimable_outpoints, (commitment_txid, watch_outputs), to_counterparty_output_info) | |||
| Err(_) => return (claimable_outpoints, to_counterparty_output_info) | |||
There was a problem hiding this comment.
am I blind or is ignore_error only getting called once? Should we just get rid of it while we're here?
There was a problem hiding this comment.
Created another PR for this: #3093
as it was separate commit in any case.
Let me know if i should merge that into this one, i didn't because this one already has 1 approval.
There was a problem hiding this comment.
thanks! I don't think it needs to be in this PR, and thanks for creating the followup!
Watch all outputs irrespective of claimable outpoints
This removes dependency of watched_outputs from per_commitment_claimable_outpoints, it is required since we will no longer have direct access to per_commitment_claimable_outpoints once we start publishing
PersistClaimInfoas part of [Memory] Persistcounterparty_claimable_outpointsout of channel_monitor #3049