Post-anchor: do not aggregate claim of revoked output #1841

ariard · 2022-11-09T00:30:26Z

This protect the justice claim of counterparty revoked output. As otherwise if the all the revoked outputs claims are batched in a single transaction, low-feerate HTLCs transactions can delay our honest justice claim transaction until BREAKDOWN_TIMEOUT expires.

wpaulino

I think this is also missing the same change in package.rs upon deserializing pending inputs.

ariard · 2022-11-09T01:01:46Z

I think this is also missing the same change in package.rs upon deserializing pending inputs.

Yes, correct. Do we prefer to add directly the malleability flag to the serializers or the channel type in PackageTemplate ?

wpaulino · 2022-11-09T01:07:59Z

We could add the opt_anchors flag to RevokedOutput as was done throughout the anchor changes with HolderHTLCOutput and HolderFundingOutput.

codecov-commenter · 2022-11-09T03:22:49Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.35 🎉

Comparison is base (56b0c96) 91.63% compared to head (6ff5e67) 91.99%.

❗ Current head 6ff5e67 differs from pull request most recent head 5e968ed. Consider uploading reports for the commit 5e968ed to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1841      +/-   ##
==========================================
+ Coverage   91.63%   91.99%   +0.35%     
==========================================
  Files         104      104              
  Lines       51985    55966    +3981     
  Branches    51985    55966    +3981     
==========================================
+ Hits        47638    51487    +3849     
- Misses       4347     4479     +132

Impacted Files	Coverage Δ
lightning/src/chain/channelmonitor.rs	`94.82% <100.00%> (ø)`
lightning/src/chain/package.rs	`91.07% <100.00%> (+0.08%)`	⬆️
lightning/src/ln/monitor_tests.rs	`97.67% <100.00%> (-0.54%)`	⬇️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

ariard · 2022-11-17T00:26:35Z

Updated at e40fcda

Added two new fields to RevokedOutput, one is is_counterparty_balanceturns toSome(())when the claimed output is the counterparty output on the commitment transaction, the other one isopt_anchorturns toSome(())` when the claimed channel is anchor type. This should capture the matrix of channel types/outputs types (legacy counterparty balance output/legacy HTLC output/anchor balance output/anchor HTLC output). Still, it raises the wonder if we should rework our package API to better capture type/malleability/aggregation, though this can be left to future PRs.

TheBlueMatt · 2022-11-21T20:48:40Z

Doesn't currently build?

ariard · 2022-11-22T23:16:30Z

Updated at 22d1238

Doesn't currently build?

That sound right we have unit test coverage here. Should be fixed.

wpaulino

Just one serialization comment, otherwise LGTM

lightning/src/chain/package.rs

ariard · 2022-11-23T22:53:23Z

Updated at 29ab7af

Should build, at least locally.

lightning/src/chain/package.rs

lightning/src/chain/channelmonitor.rs

ariard · 2022-11-30T01:56:37Z

Updated at 11ddd4f.

lightning/src/chain/package.rs

TheBlueMatt · 2022-11-30T19:59:51Z

Otherwise LGTM.

lightning/src/chain/package.rs

ariard · 2022-12-09T02:25:33Z

Updated at 4b23f36

Rebased post-#1825, good to re-review the whole.

lightning/src/chain/package.rs

wpaulino · 2022-12-12T20:15:12Z

lightning/src/chain/package.rs

+			PackageSolvingData::RevokedHTLCOutput(..) => { (PackageMalleability::Malleable, true) },
+			PackageSolvingData::CounterpartyOfferedHTLCOutput(..) => { (PackageMalleability::Malleable, true) },
+			PackageSolvingData::CounterpartyReceivedHTLCOutput(..) => { (PackageMalleability::Malleable, false) },
+			PackageSolvingData::HolderHTLCOutput(..) => { (PackageMalleability::Untractable, false) },


This got rid of some of the anchor changes, was that intentional?

Can you point me to which changes ? Yes it modifies intentionally some changes to avoid exposures to claim batch on anchor outputs on our side though dunno if those are the changes you’re thinking of ?

Looks like some changes may have gotten lost in a rebase conflict. HolderHTLCOutput should be Malleable with anchors and additionally support aggregation on those with known preimages.

So I agree on the first point that HolderHTLCOutput should be Malleable with anchors however on the second point to aggregate HTLCs with known preimages this can be a safety risk if a) the paired HTLC-timeout is final and can be broadcast therefore breaking the batch and b) the value of the HTLC do not weight for the average feerate weight of each input ?

Note there is no such aggregation based on preimage currently:

rust-lightning/lightning/src/chain/package.rs

Line 836 in 0e8da58

PackageMalleability::Malleable

Are you thinking about another branch ?

Note, this is not I’m strongly opposed to aggregate preimages claims, at the minimal we should it with the safe-guards mentioned above and if as such better to introduce them in another PR.

to aggregate HTLCs with known preimages this can be a safety risk

How are these claims any different from CounterpartyOfferedHTLCOutput claims? It seems we already aggregate those.

the paired HTLC-timeout is final and can be broadcast therefore breaking the batch

I don't see the problem here, at least we still try the batch and save on fees. If a conflicting spend confirms, then we'll retry the batch without it. Again, this doesn't seem any different than CounterpartyOfferedHTLCOutput claims.

I noticed for new requests we already have a path to try them by themselves even if we can aggregate them if their timelock is within CLTV_SHARED_CLAIM_BUFFER blocks. Maybe we can also start splitting them up once we're within this range and we've them tried in a batch a few times without getting a confirmation?

the value of the HTLC do not weight for the average feerate weight of each input ?

That seems like a separate issue, no? I think we'll always claim outputs even if it's not economically rational to do so at the current feerates. That's something we can certainly improve on, but that can happen as a follow-up and doesn't need to impact our aggregation logic.

Note, this is not I’m strongly opposed to aggregate preimages claims, at the minimal we should it with the safe-guards mentioned above and if as such better to introduce them in another PR.

Well the issue is that the aggregation already exists upstream with tests, and this PR is attempting to revert that as an unrelated change.

I was under the impression we already handled this by splitting packages when we get close to the expiration.

Wilmer pointed out offline that this doesn't happen later, only when we register the package. I think we should explore just spitting later, but I'm not sure this is a critical thing - we fundamentally assume in lightning that we can get a tx confirmed before the HTLC times out. If we cannot, its not unreasonable to have an issue, we already lost money! Certainly no need to break out packages into individual transactions for that.

Fair enough, though I still don't see how this is any different from preimage claims on a remote commitment, as this > is all possible there. It seems like my suggestion of splitting them up (sorted by earliest HTLC expiry) after a few
unconfirmed batches would address this for the most part?

Yes I assume we would split anchor output preimage claims on both holder/counterparty commitment, as all those claims fee-bumping is under our responsibility. The issues with splitting up after few unconfirmed batches, this is as much blocks gain for an adversary, and i’m not even sure we don’t fail our broadcast for the reason that an individual claim might not have an absolute fee (RBF rule 3) better than the batch already propagated across network mempools (though with a poorer feerate).

In that case, we should keep the preimage aggregated claims logic intact, and follow up with this splitting heuristic in a new PR.

Just to be clear, I’m all good with keeping the preimage aggregated claim logic intact in this PR and introduce splitting heuristics or no-batch-all requirements on a new PR.

we fundamentally assume in lightning that we can get a tx confirmed before the HTLC times out.

And this is the exact assumption I have a strong doubt about in a anchor output world where we have to manage limited fee-bumping reserves to feed our HTLC claims, compared to the legacy world where the channel value can be used to feed HTLC claims (even if this legacy world is broken as you cannot assume liveliness/cooperation of your peer to succeed an update_feeflow). Our fee-bumping reserves strategy is not really specified yet and the part of this strategy we delegate to the end user is unclear as of #2089. As there is already lightning softwares deployed with anchor output support, I’m suggesting to move this conversation offline.

And this is the exact assumption I have a strong doubt about in a anchor output world where we have to manage limited fee-bumping reserves to feed our HTLC claims, compared to the legacy world where the channel value can be used to feed HTLC claims (even if this legacy world is broken as you cannot assume liveliness/cooperation of your peer to succeed an update_feeflow)

But splitting makes it worse! If we're super concerned about our ability to get a tx confirmed in time because we're low on fees, the worst thing we can do is (preemptively) split the packages up into lots of transactions. At a minimum we should split as HTLCs near expiry, but even there I'm not super convinced. Indeed, anchors in general are pretty risky, but I'm very unconvinced that we should just go "well, we can't rely on the lightning security assumption, so lets write a ton of code to maybe kinda handle the case where we fail our security assumptions, but of course that code will hopefully never be hit, and thus never tested."

If we're super concerned about our ability to get a tx confirmed in time because we're low on fees, the worst thing we can do is (preemptively) split the packages up into lots of transactions.

Splitting packages into a lot of transactions when we’re in low-fees is far from being the less economically rational strategy to adopt if we have HTLC-preimages with strong asymmetries in term of offered HTLC outputs because they have been inbound routed to us by an adversary. With anchors, I think we’re introducing a fee-bumping reserves oracle to an adversary that can be adapted in real-time based on the packages we’re broadcasting. I share the belief on having a lot of complex never-tested code to special-case on-time package splitting and the adequate mitigation might be at the ChannelManager-level, namely refusing to route HTLC when we’re low on fee-bumping reserves. In anycase, if anyone has an up-to-date threat model for anchor output channels, you’re free to share it.

ariard · 2023-04-30T17:22:54Z

Updated at ef747d3 with @wpaulino comments addressed.

Sorry for the latency - For context, post-anchor this changes prevents a counterparty to pin the claims of the offered HTLC outputs with timeout transactions until the to_local balance output is available to claim in the hypothesis we would claim htlc output + balance output in a single batch transaction.

wpaulino · 2023-05-01T20:20:46Z

lightning/src/chain/package.rs

+			PackageSolvingData::RevokedHTLCOutput(..) => { (PackageMalleability::Malleable, true) },
+			PackageSolvingData::CounterpartyOfferedHTLCOutput(..) => { (PackageMalleability::Malleable, true) },
+			PackageSolvingData::CounterpartyReceivedHTLCOutput(..) => { (PackageMalleability::Malleable, false) },
+			PackageSolvingData::HolderHTLCOutput(..) => { (PackageMalleability::Untractable, false) },


Looks like some changes may have gotten lost in a rebase conflict. HolderHTLCOutput should be Malleable with anchors and additionally support aggregation on those with known preimages.

lightning/src/chain/package.rs

wpaulino · 2023-05-01T20:31:26Z

lightning/src/chain/package.rs

@@ -846,20 +860,6 @@ impl PackageTemplate {
 			height_original,
 		}
 	}
-
-	fn map_output_type_flags(input_solving_data: &PackageSolvingData) -> (PackageMalleability, bool) {


To avoid moving this method between commits, this could have been added under PackageSolvingData in the previous commit (a1eebd2).

Unless you insist, I’ll leave as it :)

I would prefer it's done, but will defer to the second reviewer.

ariard · 2023-05-01T23:34:41Z

Updated at dba8d68.

Thanks for the review. Took some comments and answered others, proposing to defer some to another PR.

ariard · 2023-05-03T03:13:11Z

Updated at 163db6f with aggregable flag true for anchor output. Tests should be fixed now.

lightning/src/chain/package.rs

wpaulino · 2023-05-03T19:06:09Z

The anchor tests were still failing by the way. You can run them locally with RUSTFLAGS="--cfg=anchors" cargo test -p lightning.

ariard · 2023-05-04T01:47:07Z

The anchor tests were still failing by the way. You can run them locally with RUSTFLAGS="--cfg=anchors" cargo test -p lightning.

Thanks updated at 7d1b4a5, test_revoked_counterparty_aggregated_claims should have been fixed as justice claims on counterparty to_self output have been split. Note there is still the comment above about the unreachable pattern compiler warning to solve.

TheBlueMatt · 2023-05-04T23:27:07Z

CI failure is #2266

ariard · 2023-05-05T15:57:07Z

Updated at 6ff5e67 with Wilmer suggestion applied.

CI failure is #2266

All tests are passing locally, including the anchor ones. Should I take action in consequence of #2266 ?

lightning/src/chain/channelmonitor.rs

lightning/src/chain/package.rs

TheBlueMatt · 2023-05-05T20:17:14Z

No need to do anything about CI/the message fuzz failures, no.

ariard · 2023-05-09T18:39:46Z

Updated at 5aec123 with comments addressed.

lightning/src/chain/package.rs

ariard · 2023-05-12T00:49:49Z

Updated at 7ac1ed2.

TheBlueMatt · 2023-05-12T21:57:07Z

lightning/src/chain/package.rs

 		}
 	}
 }

 impl_writeable_tlv_based!(RevokedOutput, {
 	(0, per_commitment_point, required),
+	(1, is_counterparty_balance_on_anchors, required),


We should keep the Option<()> hack over a bool - making this odd and required means (a) previous versions that don't understand it will ignore it (which we don't want, we want them to fail to read) and (b) new code will refuse to read existing objects missing the field, which all existing objects are.

Should have been modified to be compatible with the back-compatibility of the serialiazation framework.

Shouldn't this now be an even TLV with option like opt_anchors everywhere else? As is, odd with required means we'll fail to read on upgrade.

Yes, it needs to be an option to accept old data and needs to be even so that old clients will refuse to read.

This should be an even option now. Let me know if new code matches what you have in mind.

ariard · 2023-05-14T22:48:58Z

Updated at b3bf68e.

See lightning/bolts#803 This protect the justice claim of counterparty revoked output. As otherwise if the all the revoked outputs claims are batched in a single transaction, low-feerate HTLCs transactions can delay our honest justice claim transaction until BREAKDOWN_TIMEOUT expires.

ariard · 2023-05-16T01:13:36Z

Updated at 6f9adfa7

wpaulino · 2023-05-16T18:02:59Z

lightning/src/ln/monitor_tests.rs

-		assert_eq!(revoked_claim_b.input.len(), 3); // Spends both HTLC outputs and to_self output
-		assert_eq!(revoked_claim_b.output.len(), 1);
-		check_spends!(revoked_claim_b, revoked_commitment_b);
+		assert_eq!(revoked_htlc_claim_a.input.len(), 2); // Spends both HTLC outputs and to_self output


Please remove the TODO now and fix the comments on the spend assertions below. LGTM otherwise.

Should be done, thanks.

ariard · 2023-05-16T22:03:50Z

Updated at 5e968ed

wpaulino reviewed Nov 9, 2022

View reviewed changes

wpaulino mentioned this pull request Nov 16, 2022

Introduce new BumpTransactionEvent variant HTLCResolution #1825

Merged

ariard force-pushed the 2022-11-revoked-balance-non-aggregable branch from ccaf9e5 to e40fcda Compare November 17, 2022 00:23

ariard force-pushed the 2022-11-revoked-balance-non-aggregable branch from e40fcda to 22d1238 Compare November 22, 2022 23:13

wpaulino reviewed Nov 23, 2022

View reviewed changes

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

ariard force-pushed the 2022-11-revoked-balance-non-aggregable branch from 22d1238 to 29ab7af Compare November 23, 2022 22:50

wpaulino reviewed Nov 24, 2022

View reviewed changes

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

TheBlueMatt reviewed Nov 25, 2022

View reviewed changes

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

ariard force-pushed the 2022-11-revoked-balance-non-aggregable branch from 06cab04 to 11ddd4f Compare November 30, 2022 01:53

TheBlueMatt reviewed Nov 30, 2022

View reviewed changes

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

wpaulino reviewed Nov 30, 2022

View reviewed changes

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

ariard added the Anchor Support label Dec 7, 2022

ariard force-pushed the 2022-11-revoked-balance-non-aggregable branch 2 times, most recently from 679ba56 to 4b23f36 Compare December 9, 2022 02:23

wpaulino reviewed Dec 12, 2022

View reviewed changes

ariard force-pushed the 2022-11-revoked-balance-non-aggregable branch from 4b23f36 to ef747d3 Compare April 30, 2023 17:08

wpaulino reviewed May 1, 2023

View reviewed changes

ariard force-pushed the 2022-11-revoked-balance-non-aggregable branch from ef747d3 to dba8d68 Compare May 1, 2023 23:30

ariard force-pushed the 2022-11-revoked-balance-non-aggregable branch from dba8d68 to 163db6f Compare May 3, 2023 03:12

wpaulino reviewed May 3, 2023

View reviewed changes

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

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

ariard force-pushed the 2022-11-revoked-balance-non-aggregable branch from 163db6f to 7d1b4a5 Compare May 4, 2023 01:42

ariard force-pushed the 2022-11-revoked-balance-non-aggregable branch from 7d1b4a5 to 6ff5e67 Compare May 5, 2023 15:54

TheBlueMatt reviewed May 5, 2023

View reviewed changes

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

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

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

ariard force-pushed the 2022-11-revoked-balance-non-aggregable branch 2 times, most recently from 9051884 to 5aec123 Compare May 9, 2023 18:39

TheBlueMatt reviewed May 10, 2023

View reviewed changes

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

ariard force-pushed the 2022-11-revoked-balance-non-aggregable branch from 5aec123 to 7ac1ed2 Compare May 12, 2023 00:48

TheBlueMatt reviewed May 12, 2023

View reviewed changes

ariard force-pushed the 2022-11-revoked-balance-non-aggregable branch from 7ac1ed2 to b3bf68e Compare May 14, 2023 22:48

ariard force-pushed the 2022-11-revoked-balance-non-aggregable branch from b3bf68e to 6f9adfa Compare May 16, 2023 01:12

wpaulino reviewed May 16, 2023

View reviewed changes

Remove aggregable flag from PackageTemplate constructor

5e968ed

ariard force-pushed the 2022-11-revoked-balance-non-aggregable branch from 6f9adfa to 5e968ed Compare May 16, 2023 22:02

wpaulino approved these changes May 17, 2023

View reviewed changes

TheBlueMatt approved these changes May 18, 2023

View reviewed changes

TheBlueMatt merged commit 7866394 into lightningdevkit:main May 18, 2023

Post-anchor: do not aggregate claim of revoked output #1841

Post-anchor: do not aggregate claim of revoked output #1841

Conversation

ariard commented Nov 9, 2022

wpaulino left a comment

Choose a reason for hiding this comment

ariard commented Nov 9, 2022

wpaulino commented Nov 9, 2022

codecov-commenter commented Nov 9, 2022 • edited Loading

Codecov Report

ariard commented Nov 17, 2022

TheBlueMatt commented Nov 21, 2022

ariard commented Nov 22, 2022

wpaulino left a comment

Choose a reason for hiding this comment

ariard commented Nov 23, 2022

ariard commented Nov 30, 2022

TheBlueMatt commented Nov 30, 2022

ariard commented Dec 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented Apr 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented May 1, 2023

ariard commented May 3, 2023

wpaulino commented May 3, 2023

ariard commented May 4, 2023

TheBlueMatt commented May 4, 2023

ariard commented May 5, 2023

TheBlueMatt commented May 5, 2023

ariard commented May 9, 2023

ariard commented May 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented May 14, 2023

ariard commented May 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented May 16, 2023

codecov-commenter commented Nov 9, 2022 •

edited

Loading