Expose counterparty-revoked-outputs in `get_claimable_balance` #1495

TheBlueMatt · 2022-05-25T00:32:59Z

Depends on ~~#1494, #1481, and #1486~~. Fixes #1466.

Currently, users have no way of (easily) determining if a ChannelMonitor for a closed channel has claimed all available outpoints and can do no further work (implying it can be safely deleted). The intention was always to allow get_claimable_balances to provide that information, however it was (until now) not able to report Balances for revoked counterparty outputs.

This implements that final missing feature, allowing us to prune ChannelMonitors after this lands (and we give it some time to simmer).

codecov-commenter · 2022-05-25T04:28:07Z

Codecov Report

Merging #1495 (bb09577) into main (b84b53a) will increase coverage by 0.10%.
The diff coverage is 96.17%.

❗ Current head bb09577 differs from pull request most recent head d8651e3. Consider uploading reports for the commit d8651e3 to get more accurate results

@@            Coverage Diff             @@
##             main    #1495      +/-   ##
==========================================
+ Coverage   90.77%   90.87%   +0.10%     
==========================================
  Files          80       80              
  Lines       44763    45173     +410     
  Branches    44763    45173     +410     
==========================================
+ Hits        40632    41052     +420     
+ Misses       4131     4121      -10

Impacted Files	Coverage Δ
lightning/src/chain/channelmonitor.rs	`91.06% <89.90%> (+0.02%)`	⬆️
lightning/src/ln/monitor_tests.rs	`99.86% <99.73%> (-0.14%)`	⬇️
lightning/src/chain/onchaintx.rs	`94.94% <100.00%> (+0.03%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`93.58% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/reorg_tests.rs	`100.00% <100.00%> (ø)`
lightning-rapid-gossip-sync/src/processing.rs	`90.43% <0.00%> (-0.97%)`	⬇️
lightning/src/util/ser.rs	`90.77% <0.00%> (-0.65%)`	⬇️
lightning/src/routing/gossip.rs	`91.32% <0.00%> (-0.48%)`	⬇️
lightning/src/util/events.rs	`39.25% <0.00%> (-0.25%)`	⬇️
... and 16 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

lightning/src/chain/channelmonitor.rs

TheBlueMatt · 2022-07-06T00:53:50Z

Okay, sorry for the delay, finally responded to all feedback here.

lightning/src/ln/reorg_tests.rs

lightning/src/chain/channelmonitor.rs

lightning/src/ln/monitor_tests.rs

lightning/src/chain/channelmonitor.rs

wpaulino · 2022-07-07T00:56:35Z

Feel free to squash the existing commits before your next push.

TheBlueMatt · 2022-07-11T22:04:52Z

Rebased due to a trivial conflict. Squashed previous fixups from the previous review, new fixups address this review. Still need to address #1495 (comment) and #1495 (comment) and we need to decide on #1495 (comment)

TheBlueMatt · 2022-07-12T03:36:21Z

Okay, addressed all the comments except for the one asking for a new test, will look into that when I get a chance.

wpaulino

Fixups look good, feel free to squash. Will do another full pass once the test is up.

lightning/src/chain/channelmonitor.rs

TheBlueMatt · 2022-07-14T21:26:53Z

Okay, sorry for the delay here. I added the requested test (code already worked right!) and addressed all the outstanding feedback.

lightning/src/chain/channelmonitor.rs

valentinewallace · 2022-07-15T16:51:46Z

lightning/src/chain/channelmonitor.rs

@@ -1427,67 +1427,82 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
 			($holder_commitment: expr, $htlc_iter: expr) => {
 				for htlc in $htlc_iter {
 					if let Some(htlc_commitment_tx_output_idx) = htlc.transaction_output_index {


It would be nice to get rid of the nesting here too with a similar strategy to 200c3c6

I mean its all in a single if? We can also try to clean up a few things in a followup.

Fine to revisit in follow-up. My philosophy is that this seems like the #1 module to be made as readable as possible

lightning/src/chain/channelmonitor.rs

valentinewallace · 2022-07-15T17:36:28Z

lightning/src/chain/channelmonitor.rs

+							} else if us.payment_preimages.get(&htlc.payment_hash).is_some() {
+								// Otherwise (the payment was inbound), only expose it as claimable if
+								// we know the preimage.
+								// Note that if there is a pending claim, but it did not use the


3 lines up, we have an else if we know the preimage, but should there be an additional else adding a ContentiousClaimable given we may receive the preimage at some point in the future, in time to claim?

Oh! Great catch!

Actually, that's an existing issue and this PR is already huge. I'm gonna punt it to a followup if that's okay. #1620

TheBlueMatt · 2022-07-21T21:42:50Z

Squashed, difference from yesterday is just some comment fixes and whitespace:

$ git diff-tree -U1 19ed4cd e8a1b379
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index 0a0906d18..cb11e16cc 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -1431,3 +1431,2 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 
-
 		let mut htlc_spend_txid_opt = None;
@@ -1642,5 +1641,5 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
 					// The counterparty broadcasted a revoked state!
-					// Look for a StaticOutput spend first, as it should be spending the full set
-					// of commitment transaction outputs, if we see one, assume that it did and
-					// just return that.
+					// Look for any StaticOutputs first, generating claimable balances for those.
+					// If any match the confirmed counterparty revoked to_self output, skip
+					// generating a CounterpartyRevokedOutputClaimable.
 					let mut spent_counterparty_output = false;

wpaulino

LGTM, just have two minor comments.

lightning/src/chain/channelmonitor.rs

TheBlueMatt · 2022-07-26T18:56:28Z

Rebased after merge of the new HTLCHandlingFailed event, which now need to be handled.

TheBlueMatt · 2022-07-26T19:17:57Z

Not worth delaying 110 for this, I think, given it doesn't resolve all the issues with missing entries.

valentinewallace

I'm ACK after squash :)

Github seems to think it needs a rebase, though it doesn't list any conflicted filenames?

When we see a transaction which generates some `OnchainEvent`, its useful to have the full transaction around for later analysis. Specifically, it lets us check the list of outputs which were spent in the transaction, allowing us to look up, e.g. which HTLC outpoint was spent in a transaction. This will be used in a few commits to do exactly that - figure out which HTLC a given `OnchainEvent` corresponds with.

When handling a revoked counterparty commitment transaction which was broadcast on-chain, we occasionally need to look up which output (and its value) was to the counterparty (the `to_self` output). This will allow us to generate `Balance`s for the user for the revoked output.

The test intended to disconnect a transaction previously connected but didn't disconnect enough blocks to do so, leading to it confirming two conflicting transactions. In the next few commits this will become an assertion failure.

When a counterparty broadcasts a revoked commitment transaction, followed immediately by HTLC-Success/-Timeout spends thereof, we'd like to have an `onchain_events_awaiting_threshold_conf` entry for them. This does so using the `HTLCSpendConfirmation` entry, giving it (slightly) new meaning. Because all existing uses of `HTLCSpendConfirmation` already check if the relevant commitment transaction is revoked first, this should be trivially backwards compatible. We will ultimately figure out if something is being spent via the `OnchainTxHandler`, but to do so we need to look up the output via the HTLC transaction txid, which this allows us to do.

We need this information when we look up if we still need to spend a revoked output from an HTLC-Success/HTLC-Timeout transaction for balance calculation.

Instead of a series of different `onchain_events_awaiting_threshold_conf.iter()...` calls to scan for HTLC status in balance calculation, pull them all out into one `for ... { match ... }` to do it once and simplify the code somewhat.

TheBlueMatt · 2022-08-15T23:19:20Z

Squashed and rebased (which conflicted due to the rust-bitcoin bump):

$ git range-diff b84b53a14c37a926664dd767d78b3c5a34a871ab...a52cf411347a0004d9628c7c6e52fe257cf9bf28 4005724ae6719594e42d11718ce9b83fa0641f3b...d8651e3dd57673792344d6aff65eb79fc9f2340b
 1:  feeb55022 !  1:  9115f66db Store the full event transaction in `OnchainEvent` structs
    @@ lightning/src/chain/channelmonitor.rs: impl Writeable for OnchainEventEntry {
     @@ lightning/src/chain/channelmonitor.rs: impl Writeable for OnchainEventEntry {
      impl MaybeReadable for OnchainEventEntry {
        fn read<R: io::Read>(reader: &mut R) -> Result<Option<Self>, DecodeError> {
    -           let mut txid = Default::default();
    +           let mut txid = Txid::all_zeros();
     +          let mut transaction = None;
                let mut height = 0;
                let mut event = None;
 2:  dfa03734e !  2:  0ec54ecd9 Track counterparty payout info in counterparty commitment txn
    @@ lightning/src/chain/channelmonitor.rs: impl<Signer: Sign> ChannelMonitorImpl<Sig
                                                log_bytes!(self.funding_info.0.to_channel_id()), tx.txid());
                                        self.funding_spend_seen = true;
     +                                  let mut commitment_tx_to_counterparty_output = None;
    -                                   if (tx.input[0].sequence >> 8*3) as u8 == 0x80 && (tx.lock_time >> 8*3) as u8 == 0x20 {
    +                                   if (tx.input[0].sequence.0 >> 8*3) as u8 == 0x80 && (tx.lock_time.0 >> 8*3) as u8 == 0x20 {
     -                                          let (mut new_outpoints, new_outputs) = self.check_spend_counterparty_transaction(&tx, height, &logger);
     +                                          let (mut new_outpoints, new_outputs, counterparty_output_idx_sats) =
     +                                                  self.check_spend_counterparty_transaction(&tx, height, &logger);
 3:  84050bc22 =  3:  66ced68ec Clean up nesting in `get_counterparty_output_claim_info`
 4:  0b7523dd8 !  4:  e76ac3333 Fix off-by-one in test_onchain_htlc_claim_reorg_remote_commitment
    @@ lightning/src/ln/reorg_tests.rs: fn do_test_onchain_htlc_reorg(local_commitment:
     +          disconnect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
      
                let block = Block {
    -                   header: BlockHeader { version: 0x20000000, prev_blockhash: nodes[1].best_block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 },
    +                   header: BlockHeader { version: 0x20000000, prev_blockhash: nodes[1].best_block_hash(), merkle_root: TxMerkleNode::all_zeros(), time: 42, bits: 42, nonce: 42 },
 5:  4cc93b8a4 =  5:  9469ce0f3 Track HTLC-Success/HTLC-Timeout claims of revoked outputs
 6:  23c3cd2b3 =  6:  04d8f5e3f Track the txid that resolves HTLCs even after resolution completes
 7:  acaaffd83 =  7:  a17794c38 Scan `onchain_events_awaiting_threshold_conf` once in balance calc
 8:  7fd237203 =  8:  f712eb415 Simplify claimable balance trivially with consistent accessors
 9:  96ed4da7d !  9:  5a8ede09f Expose counterparty-revoked-outputs in `get_claimable_balance`
    @@ lightning/src/ln/monitor_tests.rs: fn test_balances_on_local_commitment_htlcs()
     +  connect_blocks(&nodes[0], htlc_cltv_timeout + 1 - 10);
     +  check_closed_broadcast!(nodes[0], true);
     +  check_added_monitors!(nodes[0], 1);
    -+  let events = nodes[0].node.get_and_clear_pending_events();
    -+  assert_eq!(events.len(), 2);
    ++
    ++  let mut events = nodes[0].node.get_and_clear_pending_events();
    ++  assert_eq!(events.len(), 6);
    ++  let mut failed_payments: HashSet<_> =
    ++          [timeout_payment_hash, dust_payment_hash, live_payment_hash, missing_htlc_payment_hash]
    ++          .iter().map(|a| *a).collect();
    ++  events.retain(|ev| {
    ++          match ev {
    ++                  Event::HTLCHandlingFailed { failed_next_destination: HTLCDestination::NextHopChannel { node_id, channel_id }, .. } => {
    ++                          assert_eq!(*channel_id, chan_id);
    ++                          assert_eq!(*node_id, Some(nodes[1].node.get_our_node_id()));
    ++                          false
    ++                  },
    ++                  Event::HTLCHandlingFailed { failed_next_destination: HTLCDestination::FailedPayment { payment_hash }, .. } => {
    ++                          assert!(failed_payments.remove(payment_hash));
    ++                          false
    ++                  },
    ++                  _ => true,
    ++          }
    ++  });
    ++  assert!(failed_payments.is_empty());
     +  if let Event::PendingHTLCsForwardable { .. } = events[0] {} else { panic!(); }
     +  match &events[1] {
     +          Event::ChannelClosed { reason: ClosureReason::CommitmentTxConfirmed, .. } => {},
    @@ lightning/src/ln/monitor_tests.rs: fn test_balances_on_local_commitment_htlcs()
     +  assert_eq!(funding_outpoint.to_channel_id(), chan_id);
     +
     +  let payment_preimage = route_payment(&nodes[0], &[&nodes[1]], 3_000_000).0;
    -+  route_payment(&nodes[1], &[&nodes[0]], 1_000_000).0;
    ++  let failed_payment_hash = route_payment(&nodes[1], &[&nodes[0]], 1_000_000).1;
     +  let revoked_local_txn = get_local_commitment_txn!(nodes[1], chan_id);
     +  assert_eq!(revoked_local_txn[0].input.len(), 1);
     +  assert_eq!(revoked_local_txn[0].input[0].previous_output.txid, funding_tx.txid());
    @@ lightning/src/ln/monitor_tests.rs: fn test_balances_on_local_commitment_htlcs()
     +  assert_eq!(revoked_htlc_timeout_txn.len(), 1);
     +  check_spends!(revoked_htlc_timeout_txn[0], revoked_local_txn[0]);
     +  assert_ne!(revoked_htlc_success_txn[0].input[0].previous_output, revoked_htlc_timeout_txn[0].input[0].previous_output);
    -+  assert_eq!(revoked_htlc_success_txn[0].lock_time, 0);
    -+  assert_ne!(revoked_htlc_timeout_txn[0].lock_time, 0);
    ++  assert_eq!(revoked_htlc_success_txn[0].lock_time.0, 0);
    ++  assert_ne!(revoked_htlc_timeout_txn[0].lock_time.0, 0);
     +
     +  // A will generate justice tx from B's revoked commitment/HTLC tx
     +  mine_transaction(&nodes[0], &revoked_local_txn[0]);
    @@ lightning/src/ln/monitor_tests.rs: fn test_balances_on_local_commitment_htlcs()
     +          }]),
     +          sorted_vec(nodes[0].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances()));
     +
    -+  connect_blocks(&nodes[0], revoked_htlc_timeout_txn[0].lock_time - nodes[0].best_block_info().1);
    -+  expect_pending_htlcs_forwardable_ignore!(&nodes[0]);
    ++  connect_blocks(&nodes[0], revoked_htlc_timeout_txn[0].lock_time.0 - nodes[0].best_block_info().1);
    ++  expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(&nodes[0],
    ++          [HTLCDestination::FailedPayment { payment_hash: failed_payment_hash }]);
     +  // As time goes on A may split its revocation claim transaction into multiple.
     +  let as_fewer_input_rbf = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
     +  for tx in as_fewer_input_rbf.iter() {
10:  26a44adda <  -:  --------- f rebase on latest upstream
11:  a52cf4113 = 10:  d8651e3dd Move per-HTLC logic out of get_claimable_balances into a helper

This uses the various new tracking added in the prior commits to expose a new `Balance` type - `CounterpartyRevokedOutputClaimable`. Some nontrivial work is required, however, as we now have to track HTLC outputs as spendable in a transaction that comes *after* an HTLC-Success/HTLC-Timeout transaction, which we previously didn't need to do. Thus, we have to check if an `onchain_events_awaiting_threshold_conf` event spends a commitment transaction's HTLC output while walking events. Further, because we now need to track HTLC outputs after the HTLC-Success/HTLC-Timeout confirms, and because we have to track the counterparty's `to_self` output as a contentious output which could be claimed by either party, we have to examine the `OnchainTxHandler`'s set of outputs to spend when determining if certain outputs are still spendable. Two new tests are added which test various different transaction formats, and hopefully provide good test coverage of the various revoked output paths.

Val suggested this as an obvious cleanup to separate per_HTLC logic from the total commitment transaction logic, separating the large function into two.

TheBlueMatt added the blocked on dependent pr label May 25, 2022

TheBlueMatt added this to the 0.0.108 milestone May 25, 2022

This was referenced May 25, 2022

Correct variable names in ChannelMonitor and DRY tests #1494

Merged

Make expect_payment_failed_conditions a function #1496

Merged

TheBlueMatt force-pushed the 2022-04-all-claimables branch from 8a0d83b to 0c3b9dc Compare May 25, 2022 03:27

TheBlueMatt force-pushed the 2022-04-all-claimables branch from 0c3b9dc to dee10c5 Compare May 26, 2022 22:36

wpaulino reviewed Jun 3, 2022

View reviewed changes

TheBlueMatt modified the milestones: 0.0.109, 0.0.110 Jun 22, 2022

TheBlueMatt removed the blocked on dependent pr label Jul 5, 2022

TheBlueMatt force-pushed the 2022-04-all-claimables branch 3 times, most recently from 313ec8c to 8441ba2 Compare July 6, 2022 00:53

TheBlueMatt added the Seeking Code Review label Jul 6, 2022

wpaulino reviewed Jul 7, 2022

View reviewed changes

TheBlueMatt force-pushed the 2022-04-all-claimables branch 2 times, most recently from e3d7e64 to d565ab2 Compare July 11, 2022 22:03

TheBlueMatt force-pushed the 2022-04-all-claimables branch 2 times, most recently from b8f272f to e6d3a24 Compare July 12, 2022 03:35

wpaulino reviewed Jul 12, 2022

View reviewed changes

TheBlueMatt force-pushed the 2022-04-all-claimables branch from e6d3a24 to c9b128a Compare July 13, 2022 01:21

valentinewallace reviewed Jul 14, 2022

View reviewed changes

TheBlueMatt force-pushed the 2022-04-all-claimables branch from c9b128a to d079767 Compare July 14, 2022 21:26

valentinewallace reviewed Jul 15, 2022

View reviewed changes

TheBlueMatt dismissed valentinewallace’s stale review via e8a1b37 July 21, 2022 21:42

TheBlueMatt force-pushed the 2022-04-all-claimables branch from bb09577 to e8a1b37 Compare July 21, 2022 21:42

wpaulino reviewed Jul 25, 2022

View reviewed changes

lightning/src/chain/channelmonitor.rs Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

TheBlueMatt force-pushed the 2022-04-all-claimables branch 3 times, most recently from 095041c to a52cf41 Compare July 26, 2022 18:56

TheBlueMatt modified the milestones: 0.0.110, 0.0.111 Jul 26, 2022

TheBlueMatt mentioned this pull request Aug 7, 2022

Dust HTLCs on closed channels are always retryable #1653

Closed

valentinewallace reviewed Aug 15, 2022

View reviewed changes

TheBlueMatt added 8 commits August 15, 2022 23:17

Clean up nesting in get_counterparty_output_claim_info

66ced68

Track the txid that resolves HTLCs even after resolution completes

04d8f5e

We need this information when we look up if we still need to spend a revoked output from an HTLC-Success/HTLC-Timeout transaction for balance calculation.

Simplify claimable balance trivially with consistent accessors

f712eb4

TheBlueMatt force-pushed the 2022-04-all-claimables branch from a52cf41 to c6ea2e3 Compare August 15, 2022 23:19

TheBlueMatt added 2 commits August 16, 2022 01:06

Move per-HTLC logic out of get_claimable_balances into a helper

d8651e3

Val suggested this as an obvious cleanup to separate per_HTLC logic from the total commitment transaction logic, separating the large function into two.

TheBlueMatt force-pushed the 2022-04-all-claimables branch from c6ea2e3 to d8651e3 Compare August 16, 2022 01:06

TheBlueMatt mentioned this pull request Aug 17, 2022

Expose a Balance for inbound HTLCs even without a preimage #1673

Merged

wpaulino approved these changes Aug 17, 2022

View reviewed changes

valentinewallace approved these changes Aug 17, 2022

View reviewed changes

TheBlueMatt merged commit dc54c58 into lightningdevkit:main Aug 17, 2022

Expose counterparty-revoked-outputs in get_claimable_balance #1495

Expose counterparty-revoked-outputs in get_claimable_balance #1495

Uh oh!

Conversation

TheBlueMatt commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jul 6, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wpaulino commented Jul 7, 2022

Uh oh!

TheBlueMatt commented Jul 11, 2022

Uh oh!

TheBlueMatt commented Jul 12, 2022

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jul 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

valentinewallace Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

valentinewallace Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

valentinewallace Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jul 21, 2022

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Expose counterparty-revoked-outputs in `get_claimable_balance` #1495

Expose counterparty-revoked-outputs in `get_claimable_balance` #1495

TheBlueMatt commented May 25, 2022 •

edited

Loading

codecov-commenter commented May 25, 2022 •

edited

Loading

TheBlueMatt commented Jul 26, 2022 •

edited

Loading

TheBlueMatt commented Aug 15, 2022 •

edited

Loading