rust-lightning icon indicating copy to clipboard operation
rust-lightning copied to clipboard

Batch on-chain claims more aggressively per channel

Open wvanlint opened this issue 1 year ago • 2 comments

When batch claiming was first added, it was only done so for claims which were not pinnable, i.e. those which can only be claimed by us.

This was the conservative choice - pinning of outputs claimed by a batch would leave the entire batch unable to confirm on-chain. However, if pinning is considered an attack that can be executed with a high probability of success, then there is no reason not to batch claims of pinnable outputs together, separate from unpinnable outputs.

Whether specific outputs are pinnable can change over time - those that are not pinnable will eventually become pinnable at the height at which our counterparty can spend them. Thus, outputs are treated as pinnable if they're within COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE of that height.

Aside from outputs being pinnable or not, locktimes are also a factor for batching claims. HTLC-Timeout claims have locktimes fixed by the counterparty's signature and thus can only be aggregated with other HTLCs of the same CLTV, which we have to check for.

The complexity required here is worth it - aggregation can save users a significant amount of fees in the case of a force-closure, and directly impacts the number of UTXOs needed as a reserve for anchors.

wvanlint avatar Sep 25 '24 04:09 wvanlint

This change depends on https://github.com/lightningdevkit/rust-lightning/pull/3297.

wvanlint avatar Sep 25 '24 04:09 wvanlint

Codecov Report

Attention: Patch coverage is 98.11617% with 12 lines in your changes missing coverage. Please review.

Project coverage is 90.46%. Comparing base (726dd5c) to head (5355ab9). Report is 29 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/chain/package.rs 97.98% 4 Missing and 1 partial :warning:
lightning/src/chain/onchaintx.rs 88.88% 3 Missing and 1 partial :warning:
lightning/src/ln/monitor_tests.rs 98.88% 2 Missing and 1 partial :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3340      +/-   ##
==========================================
+ Coverage   89.69%   90.46%   +0.77%     
==========================================
  Files         130      130              
  Lines      107335   112294    +4959     
  Branches   107335   112294    +4959     
==========================================
+ Hits        96273   101591    +5318     
+ Misses       8660     8343     -317     
+ Partials     2402     2360      -42     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Sep 25 '24 06:09 codecov[bot]

Needs rebase now :tada:

TheBlueMatt avatar Oct 18 '24 17:10 TheBlueMatt

Would like to get confirmation on one case --

In test_bump_penalty_txn_on_revoked_commitment, we have 1 revoked HTLC output where the counterparty can claim it at height 41 and another where the counterparty can claim it at 81. The current height is 25.

Currently we'll aggregate these outputs, but it seems like the former output is more urgent and may warrant more aggressive fee-bumping, so aggregating them might result in paying more fees due to the overall increased transaction size? Just want to make sure this is the intended behavior.

valentinewallace avatar Dec 04 '24 20:12 valentinewallace

Would like to get confirmation on one case --

In test_bump_penalty_txn_on_revoked_commitment, we have 1 revoked HTLC output where the counterparty can claim it at height 41 and another where the counterparty can claim it at 81. The current height is 25.

Currently we'll aggregate these outputs, but it seems like the former output is more urgent and may warrant more aggressive fee-bumping, so aggregating them might result in paying more fees due to the overall increased transaction size? Just want to make sure this is the intended behavior.

I definitely think there is some trade-off to be made there that we can optimize further at the cost of some complexity, and such cases would indeed exist right now. Between always having separate transactions and always aggregating, aggregating aggressively seems to the better choice though - the total weight across all transactions will decrease and perhaps transactions will confirm quickly before additional fee bumping.

wvanlint avatar Dec 05 '24 05:12 wvanlint

Squashed the fixups.

wvanlint avatar Dec 11 '24 03:12 wvanlint

This seriously helped with previously broken unit tests for a PR I'm working on, thank you so much!

arik-so avatar Dec 12 '24 06:12 arik-so