Anchor-outputs (2/3): Add anchors and support Commitment CPFP bumping #725

ariard · 2020-09-30T01:05:58Z

[2021-06-11] Reviewers should Concept ACK the new interface and the CPFP bumping strategy. Also we should decide if we completely deprecate legacy commitment transaction format.

Overview of PR:

introduce new UtxoPool interface for bumping utxos
cleanup few namings/bug
implement new funder fee affordance checks
add the anchor outputs and adjust commitment tx weigh
fix the numerous tests broken by previous changes
support CPFP generation at commitment transaction broadcast if a bump is required.

TODO:

simple test to exercise logic
better test coverage around commitment weight changes
cancellation of UTXO reservation in case of parent reorg
add new bolt commitment vector tests
test multiple-round CPFP feerate-bump
document utxo interface

devrandom

some initial thoughts

devrandom · 2020-11-09T08:50:00Z

lightning/src/ln/onchain_utils.rs

+	}
+}
+
+impl Writeable for BumpingOutput {


why not use impl_writeable!?

devrandom · 2020-11-09T08:50:24Z

lightning/src/chain/utxointerface.rs

@@ -0,0 +1,24 @@
+//! Trait which allow others parts of rust-lightning to manage CPFP candidates


maybe name this utxopool.rs?

Maybe utxo.rs and a qualified trait name as utxo::Pool to keep trait name shorts and allow implementations to use the fuller name.

devrandom · 2020-11-09T09:04:01Z

lightning/src/ln/channelmonitor.rs

 {
 	fn block_connected(&self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], _indexes_of_txn_matched: &[usize]) {
 		let block_hash = header.block_hash();
 		{
 			let mut monitors = self.monitors.lock().unwrap();
 			for monitor in monitors.values_mut() {
-				let txn_outputs = monitor.block_connected(txn_matched, height, &block_hash, &*self.broadcaster, &*self.fee_estimator, &*self.logger);
+				let txn_outputs = monitor.block_connected(txn_matched, height, &block_hash, &*self.broadcaster, &*self.fee_estimator, &*self.logger, &*self.utxo_pool);


it seems like logger, utxo_pool and fee_estimator should probably be supplied to the monitor constructor instead of every time. and also for block_disconnected below

devrandom · 2020-11-09T10:01:41Z

lightning/src/ln/onchain_utils.rs

+			}
+		}
+	}
+	pub(crate) fn package_cpfp<U: Deref>(&mut self, utxo_pool: &U)


perhaps rename this to maybe_bump_package_via_cpfp, remove the panic, and move the conditional on the strategy from the caller to here?

devrandom · 2020-11-09T10:32:03Z

lightning/src/chain/keysinterface.rs

+	///
+	/// Spent amount is the value of the output spent by this input committed by this BIP 143
+	/// signature.
+	fn sign_cpfp<T: secp256k1::Signing>(&self, cpfp: &Transaction, input_index: usize, spent_amount: u64, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;


any chance we can break up cpfp into components (in the spirit of the phase 2 signer API), or is it premature because we don't know yet what are all the shapes we might want for the transaction?

devrandom · 2020-11-09T10:36:29Z

lightning/src/ln/onchain_utils.rs

+				};
+				// If commitment tx is still relying on its pre-signed fees to
+				// confirm, don't attach a CPFP
+				if utxo_input.is_some() {


maybe this function can be broken up a bit, it's getting long

UtxoPool is an interface to require utxos aiming to use them as bumping input for CPFPs. Implementor must provide an outpoint, its spending witness weight and the available amount. It should be also ready to sign the CPFP. A UTXO may be freed but this call isn't leveraged in the rest of this patchset to prevent mempool rejection on the ground of BIP125.

This commit integrates UtxoPool in the test framework and codebase.

This is a script builder to generate anchor output ones. They can be satisfied either by a signature for the committed funding pubkey or anyone after CSV delay expiration. This is used at anchor output addition while generating commitment transaction.

…rparty

We observe the only case where these parameters use to diverge was at funding signature, at which point we shouldn't have any pending HTLCs. We can merge them as a `local` commitment transaction is always generated at the request of your counterparty and vice-versa.

This commit modifies the fee-on-funder checks at any update of a commitment transaction, including `update_fee`. The funder must always be able to pay for both anchor outputs even if one of them doesn't materialize in practice due to the lack of a party's balance/pending HTLCs. The cost of an anchor output is covering both the output weight (`COMMITMENT_TX_WEIGHT_PER_ANCHOR`) and the minimal above-dust ouput amount (`ANCHOR_OUTPUT_VALUE`)

The field `count_tx_out` has been scale up to 3 to account for high-range cases of >255 pending commitment outputs. A expected weight (`COMMITMENT_TX_BASE_WEIGHT`) lower than effective weight would falsify the fee computation, jeopardizing commitment tx chances of confirmation.

The anchor ouputs pair is added if there are pending HTLCs. Or a a per-party anchor is added if this party has a pending balance.

Pre-committed feerates transactions (commitment, HTLC input-ouput pair) need the adjunction of an unspent transaction output. This will serve as a potential bumping input for a CPFP spending also a commitment transaction anchor output, thus increasing package feerate. Bumping utxo are only committed when the channel transactions goes onchain.

A HolderCommitmentTx package may be attached a CPFP if its feerate needs a bumping and thus an accurate operation need to scope both parent and children weight. Also disable feerate computation for pre-committed feerate package as their package amounts available for feerate-reducing will be 0.

A bumping CPFP must spend the holder anchor output which is encumbered by the funding public key. An external signer can thus verify that the CPFP feerate transaction is "reasonable".

This commit build a CPFP tx for HolderCommitmentTx and broadcast them as a whole thus bumping package feerate. A next commit condition CPFP on feerate fluctuation observation.

ariard · 2021-06-12T00:53:32Z

@devrandom Thanks for the review, I'll take it soonly, for now I've successfully rebased this 9-months old PR on top of main :)

TheBlueMatt

I feel like we really need to have a default implementation in place before we could ship something like this. Obviously we don't want to implement a full on-chain wallet, but is it possible to build an anchor-spender that always CPFPs using one output (ie by replacing CPFP transactions spending one parent with ones that spend more parents each time a new output needs bumping)? Not 100% sure if that would conflict with the old carve-out or not, it may not be possible with only one output (even if you ignore things like package size limits - we can assume only ever 25 channels, right :) ).

TheBlueMatt · 2021-06-17T20:09:45Z

lightning/src/chain/package.rs

+				let mut package_txn = vec![];
+				let signed_tx = onchain_handler.get_fully_signed_holder_tx(&outp.funding_redeemscript);
+
+				if let Some(ref bumping_outpoint) = outp.utxo_input {


Do we gain material flexibility by not building the transaction for the user and instead just giving them the output info (and relevant utilities needed to implement transaction construction)? That would simplify our code here significantly, I think?

If we don't gain any flexibility, then I think I'm happy with the general structure.

jkczyz

At a higher-level, I'm not sure about the circular dependency between PackageTemplate and OnchainTxHandler that was previously introduced. I wonder if that could be cleaned up in some way.

jkczyz · 2021-06-23T00:09:13Z

lightning/src/chain/utxointerface.rs

@@ -0,0 +1,24 @@
+//! Trait which allow others parts of rust-lightning to manage CPFP candidates


Maybe utxo.rs and a qualified trait name as utxo::Pool to keep trait name shorts and allow implementations to use the fuller name.

jkczyz · 2021-06-23T06:02:54Z

lightning/src/chain/package.rs

+/// A struct to describe a bumping output with the amount and witness weight. It is used by
+/// OnchainTxHandler to build a CPFP transaction to drag a local commitment transaction.


It's not clear what "bumping" and "drag" mean here. I think they need to be better defined or different words need to be chosen.

jkczyz · 2021-06-23T18:00:00Z

lightning/src/chain/package.rs

+		for (_, input_solving_data) in self.inputs.iter_mut() {
+			match input_solving_data {
+				PackageSolvingData::HolderFundingOutput(data) => {
+					if data.utxo_input.is_some() { return; }


What if the previously set UTXO is no longer sufficient to cover the necessary fees?

jkczyz · 2021-06-23T18:02:53Z

lightning/src/chain/package.rs

@@ -492,6 +497,24 @@ impl PackageTemplate {
 	pub(crate) fn outpoints(&self) -> Vec<&BitcoinOutPoint> {
 		self.inputs.iter().map(|(o, _)| o).collect()
 	}
+	pub(crate) fn set_bumping_utxo<U: Deref>(&mut self, utxo_pool: &U)


Should this take the required fee to pass to allocate_utxo?

jkczyz · 2021-06-23T18:07:41Z

lightning/src/chain/utxointerface.rs

+	/// Allocate a utxo to cover fee required to confirm a pending onchain transaction.
+	fn allocate_utxo(&self, required_fee: u64) -> Option<(BitcoinOutPoint, BumpingOutput)>;


Does required_fee also need to cover the witness data for the UTXO? Or is witness data not included in the fee calculation?

jkczyz · 2021-06-23T18:21:08Z

lightning/src/chain/package.rs

+					//// We sign and witness finalize bumping input
+					if let Ok(witness) = utxo_pool.provide_utxo_witness(&cpfp_tx, 1) {
+						cpfp_tx.input[1].witness = witness;
+					}


Where does the signer fit in here?

jkczyz · 2021-06-23T18:35:26Z

lightning/src/chain/utxointerface.rs

+	fn allocate_utxo(&self, required_fee: u64) -> Option<(BitcoinOutPoint, BumpingOutput)>;
+	/// Free a utxo. Call in case of reorg or counterparty claiming the output first.
+	fn free_utxo(&self, free_utxo: BitcoinOutPoint);
+	/// Provide a witness for the bumping utxo
+	fn provide_utxo_witness(&self, cpfp_transaction: &Transaction, utxo_index: u32) -> Result<Vec<Vec<u8>>, ()>;


Given a dependency between allocate_utxo and provide_utxo_witness (i.e., assuming the witness data is needed to know what the required fees are), would it make sense to consolidate these into a single method?

TheBlueMatt · 2022-04-20T18:51:06Z

Sadly this ended up quite out of date. I assume at this point anchor work may start from scratch, so going ahead and closing this. Not a big deal to reopen it on rebase.

ariard marked this pull request as draft September 30, 2020 01:06

devrandom reviewed Nov 9, 2020

View reviewed changes

Antoine Riard added 12 commits June 9, 2021 20:10

Integrate UtxoPool

9bef93b

This commit integrates UtxoPool in the test framework and codebase.

Add get_anchor_script

2c112a6

This is a script builder to generate anchor output ones. They can be satisfied either by a signature for the committed funding pubkey or anyone after CSV delay expiration. This is used at anchor output addition while generating commitment transaction.

Rename build_commitment_tx local/remote balances to new holder/counte…

dee7aaf

…rparty

Add anchor outputs pair in commitment tx

4377b5c

The anchor ouputs pair is added if there are pending HTLCs. Or a a per-party anchor is added if this party has a pending balance.

Extend ChannelKeys API with sign_cpfp

021717e

A bumping CPFP must spend the holder anchor output which is encumbered by the funding public key. An external signer can thus verify that the CPFP feerate transaction is "reasonable".

Build and broadcast CPFP for HolderCommitmentTx

3b4a7da

This commit build a CPFP tx for HolderCommitmentTx and broadcast them as a whole thus bumping package feerate. A next commit condition CPFP on feerate fluctuation observation.

ariard force-pushed the 2020-09-anchor-cpfp-commitment branch from 507c118 to 3b4a7da Compare June 12, 2021 00:50

TheBlueMatt reviewed Jun 17, 2021

View reviewed changes

jkczyz reviewed Jun 23, 2021

View reviewed changes

devrandom mentioned this pull request Aug 22, 2021

Low level anchor transaction build support #1055

Merged

TheBlueMatt closed this Apr 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Anchor-outputs (2/3): Add anchors and support Commitment CPFP bumping #725

Anchor-outputs (2/3): Add anchors and support Commitment CPFP bumping #725

ariard commented Sep 30, 2020 •

edited

Loading

devrandom left a comment

devrandom Nov 9, 2020

devrandom Nov 9, 2020

jkczyz Jun 23, 2021

devrandom Nov 9, 2020

devrandom Nov 9, 2020

devrandom Nov 9, 2020

devrandom Nov 9, 2020

ariard commented Jun 12, 2021

TheBlueMatt left a comment

TheBlueMatt Jun 17, 2021

jkczyz left a comment

jkczyz Jun 23, 2021

jkczyz Jun 23, 2021

jkczyz Jun 23, 2021

jkczyz Jun 23, 2021

jkczyz Jun 23, 2021

jkczyz Jun 23, 2021

jkczyz Jun 23, 2021

TheBlueMatt commented Apr 20, 2022

		@@ -0,0 +1,24 @@
		//! Trait which allow others parts of rust-lightning to manage CPFP candidates

		/// A struct to describe a bumping output with the amount and witness weight. It is used by
		/// OnchainTxHandler to build a CPFP transaction to drag a local commitment transaction.

		/// Allocate a utxo to cover fee required to confirm a pending onchain transaction.
		fn allocate_utxo(&self, required_fee: u64) -> Option<(BitcoinOutPoint, BumpingOutput)>;

Anchor-outputs (2/3): Add anchors and support Commitment CPFP bumping #725

Anchor-outputs (2/3): Add anchors and support Commitment CPFP bumping #725

Conversation

ariard commented Sep 30, 2020 • edited Loading

devrandom left a 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 Jun 12, 2021

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a 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

TheBlueMatt commented Apr 20, 2022

ariard commented Sep 30, 2020 •

edited

Loading