Fix two edge cases in handling of counterparty revoked commitment txn by TheBlueMatt · Pull Request #1486 · lightningdevkit/rust-lightning

TheBlueMatt · 2022-05-17T04:13:59Z

~~Based on #1481 as the tests are only useful with it.~~

Discovered while working on #1466.

This adds two new commits which correct some edge-case handling of revoked counterparty commitment transactions. They're both super duper edge, so not worth rushing too much to fix them, but will be needed for #1466 so would be nice to get them soon-ish.

The first case is only relevant for users disconnecting blocks with transaction_unconfirmed, when the counterparty broadcasts a revoked commitment transaction, which is then reorg'd out and replaced with a non-revoked commitment transaction.

The second case handles resolving HTLCs even though we haven't yet spent them, though we have broadcast the spend transactions and should spend them soon. This, too, is only relevant in case the revoked commitment transaction gets reorg'd out and replaced with a non-revoked one (and then the HTLCs claimed by our counterparty).

wpaulino

LGTM after rebase, nice catch and tests!

TheBlueMatt · 2022-05-27T18:48:20Z

Reased after merge of #1481.

codecov-commenter · 2022-05-28T18:55:29Z

Codecov Report

Merging #1486 (7cd9ebf) into main (4356809) will increase coverage by 0.84%.
The diff coverage is 90.51%.

❗ Current head 7cd9ebf differs from pull request most recent head 70ae45f. Consider uploading reports for the commit 70ae45f to get more accurate results

@@ Coverage Diff @@## main #1486 +/- ##
==========================================
+ Coverage 90.96% 91.80% +0.84% 
==========================================
Files 80 80 Lines 43533 47809 +4276 Branches 43533 47809 +4276 ==========================================
+ Hits 39598 43892 +4294 + Misses 3935 3917 -18

Impacted Files	Coverage Δ
lightning/src/chain/channelmonitor.rs	`90.82% <59.37%> (-0.42%)`	⬇️
lightning/src/ln/functional_tests.rs	`98.42% <100.00%> (+1.28%)`	⬆️
lightning/src/ln/monitor_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/reorg_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/chain/onchaintx.rs	`93.98% <0.00%> (-0.93%)`	⬇️
lightning/src/routing/gossip.rs	`94.38% <0.00%> (+2.69%)`	⬆️
lightning/src/ln/channelmanager.rs	`88.06% <0.00%> (+3.72%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4356809...70ae45f. Read the comment docs.

wpaulino · 2022-06-13T23:57:05Z

@TheBlueMatt CI is failing now with thread 'ln::reorg_tests::test_counterparty_revoked_reorg' panicked at 'assertion failed: self.node.get_and_clear_pending_events().is_empty()', lightning/src/ln/functional_test_utils.rs:306:13.

TheBlueMatt · 2022-06-14T09:47:40Z

Oops, yea, needed to rebase on latest upstream changes.

tnull · 2022-06-14T10:57:09Z

LGTM, feel free to squash.

Previously, while processing a confirmed revoked counterparty commitment transaction, we'd populate `OnchainEvent`s for live HTLCs with a `txid` source of the txid of the latest counterparty commitment transactions, not the confirmed revoked one. This meant that, if the user is using `transaction_unconfirmed` to notify us of reorg information, we'd end up not removing the entry if the revoked commitment transaction was reorg'd out. This would ultimately cause us to spuriously resolve the HTLC(s) as the chain advanced, even though we were doing so based on a now-reorged-out transaction. Luckily the fix is simple - set the correct txid in the `OnchainEventEntry`. We also take this opportunity to update logging in a few places with the txid of the transaction causing an event.

TheBlueMatt · 2022-06-15T14:22:21Z

Squashed without changes:

$ git diff-tree -U1 690817ee 9854c913
$

When we see a counterparty revoked commitment transaction on-chain we shouldn't immediately queue up HTLCs present in it for resolution until we have spent the HTLC outputs in some kind of claim transaction. In order to do so, we first have to change the `fail_unbroadcast_htlcs!()` call to provide it with the HTLCs which are present in the (revoked) commitment transaction which was broadcast. However, this is not sufficient - because all of those HTLCs had their `HTLCSource` removed when the commitment transaction was revoked, we also have to update `fail_unbroadcast_htlcs` to check the payment hash and amount when the `HTLCSource` is `None`. Somewhat surprisingly, several tests actually explicitly tested for the old behavior, which required amending to pass with the new changes. Finally, this adds a debug assertion when writing `ChannelMonitor`s to ensure `HTLCSource`s do not leak.

TheBlueMatt · 2022-06-21T16:15:53Z

Rebased without change:

$ git diff-tree -U1 7cd9ebfa7 70ae45fea
$

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

TheBlueMatt added this to the 0.0.108 milestone May 17, 2022

TheBlueMatt mentioned this pull request May 17, 2022
Test coverage for transaction_unconfirmed#1481
Merged

TheBlueMatt mentioned this pull request May 25, 2022
Expose counterparty-revoked-outputs in get_claimable_balance#1495
Merged

wpaulino reviewed May 27, 2022
View reviewed changes

TheBlueMatt force-pushed the 2022-05-revoked-txn-edge-cases branch from 19f4f99 to 334c0c4Compare May 27, 2022 18:45

TheBlueMatt added Seeking Code Review and removed blocked on dependent pr labels May 27, 2022

tnull reviewed May 28, 2022
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/ln/reorg_tests.rs

TheBlueMatt force-pushed the 2022-05-revoked-txn-edge-cases branch from 334c0c4 to 032673cCompare May 28, 2022 18:55

tnull reviewed Jun 1, 2022
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/chain/channelmonitor.rs Outdated

tnull removed the Seeking Code Review label Jun 1, 2022

TheBlueMatt force-pushed the 2022-05-revoked-txn-edge-cases branch from 032673c to a9ad6f9Compare June 13, 2022 18:01

TheBlueMatt assigned tnull and wpaulino Jun 13, 2022

tnull reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated

TheBlueMatt force-pushed the 2022-05-revoked-txn-edge-cases branch from 4154c55 to 690817eCompare June 14, 2022 09:46

TheBlueMatt force-pushed the 2022-05-revoked-txn-edge-cases branch from 690817e to 9854c91Compare June 15, 2022 14:22

tnull previously approved these changes Jun 15, 2022
View reviewed changes

wpaulino previously approved these changes Jun 15, 2022
View reviewed changes

valentinewallace reviewed Jun 16, 2022
View reviewed changes

Comment threadlightning/src/ln/monitor_tests.rs Outdated
Comment threadlightning/src/chain/channelmonitor.rs Outdated

TheBlueMatt dismissed stale reviews from wpaulino and tnull via 0e38bd5June 17, 2022 15:49

TheBlueMatt assigned valentinewallace and unassigned tnull and wpaulino Jun 17, 2022

TheBlueMatt force-pushed the 2022-05-revoked-txn-edge-cases branch from 7cd9ebf to 70ae45fCompare June 21, 2022 16:15

tnull approved these changes Jun 21, 2022
View reviewed changes

valentinewallace approved these changes Jun 21, 2022
View reviewed changes

wpaulino approved these changes Jun 21, 2022
View reviewed changes

TheBlueMatt merged commit c502e8d into lightningdevkit:mainJun 21, 2022

Navigation Menu

Uh oh!

Fix two edge cases in handling of counterparty revoked commitment txn - #1486

Fix two edge cases in handling of counterparty revoked commitment txn#1486
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-05-revoked-txn-edge-cases

TheBlueMatt commented May 17, 2022•
edited
Loading

Uh oh!

wpaulino left a comment

Uh oh!

TheBlueMatt commented May 27, 2022

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented May 28, 2022•
edited
Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wpaulino commented Jun 13, 2022

Uh oh!

TheBlueMatt commented Jun 14, 2022

Uh oh!

tnull commented Jun 14, 2022

Uh oh!

TheBlueMatt commented Jun 15, 2022

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jun 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

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

Uh oh!

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented May 27, 2022

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented May 28, 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!

wpaulino commented Jun 13, 2022

Uh oh!

TheBlueMatt commented Jun 14, 2022

Uh oh!

tnull commented Jun 14, 2022

Uh oh!

TheBlueMatt commented Jun 15, 2022

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jun 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

TheBlueMatt commented May 17, 2022•
edited
Loading

codecov-commenter commented May 28, 2022•
edited
Loading