Batch on-chain claims more aggressively per channel

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. 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.

Co-authored-by: Matt Corallo <[email protected]>

Author: wvanlint

lightning/src/chain/channelmonitor.rs

@@ -216,9 +216,16 @@ impl_writeable_tlv_based!(HTLCUpdate, {
 	(4, payment_preimage, option),
 });
 
-/// If an HTLC expires within this many blocks, don't try to claim it in a shared transaction,
-/// instead claiming it in its own individual transaction.
-pub(crate) const CLTV_SHARED_CLAIM_BUFFER: u32 = 12;
+/// If an output goes from claimable only by us to claimable by us or our counterparty within this
+/// many blocks, we consider it pinnable for the purposes of aggregating claims in a single
+/// transaction.
+pub(crate) const COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE: u32 = 12;
+
+/// When we go to force-close a channel because an HTLC is expiring, we should ensure that the
+/// HTLC(s) expiring are not considered pinnable, allowing us to aggregate them with other HTLC(s)
+/// expiring at the same time.
+const _: () = assert!(CLTV_CLAIM_BUFFER > COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE);
+
 /// If an HTLC expires within this many blocks, force-close the channel to broadcast the
 /// HTLC-Success transaction.
 /// In other words, this is an upper bound on how many blocks we think it can take us to get a

lightning/src/chain/onchaintx.rs

@@ -772,7 +772,7 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
 			for j in 0..i {
 				if requests[i].can_merge_with(&requests[j], cur_height) {
 					let merge = requests.remove(i);
-					if let Err(rejected) = requests[j].merge_package(merge) {
+					if let Err(rejected) = requests[j].merge_package(merge, cur_height) {
 						debug_assert!(false, "Merging package should not be rejected after verifying can_merge_with.");
 						requests.insert(i, rejected);
 					} else {
@@ -1106,7 +1106,7 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
 					OnchainEvent::ContentiousOutpoint { package } => {
 						if let Some(pending_claim) = self.claimable_outpoints.get(package.outpoints()[0]) {
 							if let Some(request) = self.pending_claim_requests.get_mut(&pending_claim.0) {
-								assert!(request.merge_package(package).is_ok());
+								assert!(request.merge_package(package, height).is_ok());
 								// Using a HashMap guarantee us than if we have multiple outpoints getting
 								// resurrected only one bump claim tx is going to be broadcast
 								bump_candidates.insert(pending_claim.clone(), request.clone());