Bump Engine : implement RBF for some timely channel transactions #347

ariard · 2019-07-08T23:38:25Z

Based on #336, it adds a bump_claim_tx function called in block_connected.

Every claim tx we broadcast is stored in a buffer with a pair tracking the outpoint and keeping tx material (like key, script, amount...). When we reach the given height, it means our claim txn is still in flight (likely in mempools) and its feerate isn't enough. To avoid it stucking beyond expiration of CSV/CLTV timelocks, we rebuild a claim tx and bump it using RBF.

First heuristic is using the new HighPriority estimation of the fee estimator.
Second one, is increasing the feerate by 25% compare to last claim tx broadcast.

Currently, without further protocol modifications, we can't RBF Local Commitment Transaction, no more our HTLC-Success/HTLC-Timeout transactions

TheBlueMatt · 2019-07-31T21:22:19Z

Concept ACK. Looks pretty clean implementation-wise. Obviously needs rebase and tests, but, well done.

codecov · 2019-08-01T15:14:41Z

Codecov Report

❗ No coverage uploaded for pull request base (master@01ae452). Click here to learn what that means.
The diff coverage is 70.72%.

@@            Coverage Diff            @@
##             master     #347   +/-   ##
=========================================
  Coverage          ?   87.42%           
=========================================
  Files             ?       29           
  Lines             ?    16003           
  Branches          ?        0           
=========================================
  Hits              ?    13990           
  Misses            ?     2013           
  Partials          ?        0

Impacted Files	Coverage Δ
src/chain/chaininterface.rs	`79.8% <ø> (ø)`
src/ln/functional_test_utils.rs	`94.37% <100%> (ø)`
src/util/test_utils.rs	`54.24% <100%> (ø)`
src/ln/channel.rs	`84.47% <25%> (ø)`
src/ln/channelmonitor.rs	`85.86% <59.8%> (ø)`
src/ln/functional_tests.rs	`96.06% <97.7%> (ø)`

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 01ae452...0499526. Read the comment docs.

ariard · 2019-10-26T00:22:56Z

Rebased 3fec428.

Cleaned and added test, still need to extend them to cover all cases.

TheBlueMatt

One initial comment that should make it easier to review.

TheBlueMatt · 2019-10-31T21:28:37Z

src/chain/chaininterface.rs

 	///  * satoshis-per-byte * 250
 	///  * ceil(satoshis-per-kbyte / 4)
 	fn get_est_sat_per_1000_weight(&self, confirmation_target: ConfirmationTarget) -> u64;
+	/// Gets satoshis of minimum relay fee required per 1000 Weight-Units.


Do we really need an API for this? It's relatively in-sync across the entire network (and the network would perform terribly if that weren't the case).

Hardcoding 4000sat for 1000 weight somewhere ?

Yea. Why not? It's not gonna change any time soon, I don't think, or if it does it'll go down.

Also, hardcoding it (instead of reading it from the fuzz input) should remove the full_stack_target changes, which is nice since it saves a bunch of effort regenerating them.

src/ln/channelmonitor.rs

TheBlueMatt

Really nice work here. Just a few notes. Also, for my own sanity, can you rebase on master so that we dont get all the rust warning spam garbage?

src/ln/channelmonitor.rs

TheBlueMatt · 2019-11-06T21:50:26Z

src/ln/channelmonitor.rs

+	first_seen_height: u32,
+	feerate_previous: u64,
+	soonest_timelock: u32,
+	per_input_material: HashMap<u32, InputMaterial>,


Does this need to be a HashMap or can it be a Vec?

IIRC the reason is to avoid entry duplicata as we may call check_spend_remote multiple times due to block rescanning

I think I misunderstood it (at least document what the index in this map is, cause its hella confusing): #347 (comment)

src/ln/channelmonitor.rs

TheBlueMatt · 2019-11-12T19:18:51Z

src/ln/channelmonitor.rs

+			match per_outp_material {
+				&InputMaterial::Revoked { ref script, ref pubkey, ref key, ref is_htlc, ref amount } => {
+					let sighash_parts = bip143::SighashComponents::new(&bumped_tx);
+					let sighash = hash_to_message!(&sighash_parts.sighash_all(&bumped_tx.input[i], &script, *amount)[..]);


In the long term it would be super super nice if we could DRY up the transaction signing logic so that we use the same codepath to sign txn the first time and after an RBF. OK for now, but really sucks to have duplicate code that could fail differently :(

Tracked in #400

ariard · 2019-11-13T00:43:01Z

Yeah will address comments tomorrow + harden testing, as said today having some kind of automatic/fuzzing testing for channel_monitor would be great as #327 shows it..

TheBlueMatt · 2019-11-28T06:16:32Z

Looks like the no-full_stack_target-change detection triggered on travis. Will review it anyway over the coming weekend, but would be nice to get a fix for that.

TheBlueMatt · 2019-12-02T22:50:13Z

lightning/src/ln/channelmonitor.rs

-					if *is_htlc {
+		for (ref outp, claim_tx_data) in self.our_claim_txn_waiting_first_conf.iter() {
+			outp.write(writer)?;
+			writer.write_all(&byte_utils::be32_to_array(claim_tx_data.height_timer))?;


nit: Probably easier to use the serialization macros and just implement Writable for ClaimTxBumpMaterial, no?

TheBlueMatt · 2019-12-02T23:00:10Z

lightning/src/ln/channelmonitor.rs

+	// timelock expiration scale in one claiming tx to save on fees. If this tx doesn't confirm before height timer
+	// we need to bump it (RFB or CPFP). If an input has been part of an aggregate tx at first claim try, we need to
+	// keep it within another bumped aggregate tx to comply with RBF rules.
+	our_claim_txn_waiting_first_conf: HashMap<BitcoinOutPoint, ClaimTxBumpMaterial>,


Hmm? I seem to be misunderstanding this - this map is from a spent outpoint to the info on the tx that spent it, but then inside it is a map to the list of other inputs being spent, but those should also go in this map? This seems really likely to end up inconsistent. Can we instead index this by the txid we're waiting on confirmation of (or original txid and a map from bumped txids to the original one), plus a map from outpoints to the original txid? This would remove the need for per_input_material being indexed by the input vout so that it could just be by the input in the transaction, and also would let us create single txn that spend multiple txes (eg one claim tx spending htlc txn plus commitment txn).

TheBlueMatt · 2019-12-02T23:01:57Z

The map indexing here seems to be more of a "this is the way it was" instead of "this is a sensible forward-looking indexing" IMO.

Hardcode min relay fee as its value is fixed on the bitcoin network and updating it would be done really conservatively.

TheBlueMatt

Looks pretty good. One or two more parameters could use some docs, and the test needs fixing.

TheBlueMatt · 2019-12-05T19:13:24Z

lightning/src/ln/channelmonitor.rs

+	pending_claim_requests: HashMap<Sha256dHash, ClaimTxBumpMaterial>,
+
+	// Used to link outpoints claimed in a connected block to a pending claim request.
+	claimable_outpoints: HashMap<BitcoinOutPoint, (Sha256dHash, u32)>,


Please document the fields!

TheBlueMatt · 2019-12-05T19:13:46Z

lightning/src/ln/channelmonitor.rs

+	// equality between spending transaction and claim request. If true, it means transaction was one our claiming one
+	// after a security delay of 6 blocks we remove pending claim request. If false, it means transaction wasn't and
+	// we need to regenerate new claim request we reduced set of stil-claimable outpoints.
+	pending_claim_requests: HashMap<Sha256dHash, ClaimTxBumpMaterial>,


Please document the fields (what is the Sha256dHash? the original, the latest, the wtxid, the txid, something else?)

TheBlueMatt · 2019-12-05T19:40:07Z

lightning/src/ln/channelmonitor.rs

 			let sighash_parts = bip143::SighashComponents::new(&spend_tx);

+			let mut per_input_material = HashMap::with_capacity(spend_tx.input.len());
+			let height_timer = Self::get_height_timer(height, height + inputs_info[0].2);


Why does the second argument suddenly gain a height+ here but didn't before (and other calls dont have it, though luckily tests fail if you remove it?)?

Also, why is OK to not check per-input and only check for the first non-HTLC input here?

Mostly, tbh, I dont remember what height_timer is, and its not documented (probably my fault, but should be easy to add a comment)

TheBlueMatt · 2019-12-05T19:46:19Z

lightning/src/ln/channelmonitor.rs

 					let sighash_parts = bip143::SighashComponents::new(&spend_tx);

+					let mut per_input_material = HashMap::with_capacity(spend_tx.input.len());
+					let mut soonest_timelock = 0xFFFFFFFF;


nit: std::u32::MAX

Add claimable_outpoints maps. Both structures are tied and should ensure their mutual consistency. Pending_claim_requests is cached by original claim txid. Medatada and per input material should be constant between bumped transactions, only change should be partial-claiming of outpoints set and block reorgs. Due to RBF rules, if an input has been part of an aggregate tx at first claim try, if we want the bumped tx to land nicely in the mempool, inputs should be distributed in multiple bumped tx but still be aggregate in a new bumped tx.

As local onchain txn are already monitored in block_connected by check_spend_local_transaction, it's useless to generate twice pending claims for HTLC outputs on local commitment tx. We could do the alternative.

Add RBF-bumping of justice txn, given they are only signed by us we can RBF at wish. Aggregation of bump-candidates and more aggresive bumping heuristics are left open Fix tests broken by introduction of more txn broadcast. Some tests may have a relaxed check (claim_htlc_ouputs_single_tx) as broadcast bumped txn are now interwining in previous broadcast ones and breaking simple expectations Use bumping engine to rebuild claiming transaction in case of partial- claim of its outpoints set.

Given they are only signed by us we can RBF at wish Fix tests broken by introduction of more txn broadcast (channel_monitor_network_test) Add locktime in RemoteHTLC as it's needed to generate timeout txn.

Test multiple rounds of 25% heuristic in bump_claim_tx on remote revoked commitment txn with htlcs pending in both directions.

A pending claim request may contain a set of multiple outpoints. If one or multiple of them get claimed by remote party, our in-flight claiming transactions aren't valid anymore so we need to react quickly and regenerate claiming transaction with accurate set. However, a claimed outpoint may be disconnected and we need to resurrect back outpoint among set of orignal pending claim request. To guarantee consistency of contentious claimed outpoint we cache it as OnchainEvent::ContentionsOutpoint and only delete it after ANTI_REORG_DELAY. Fix test broken by change, partial claiming on revoked txn force us to regenerate txn

TheBlueMatt · 2019-12-09T22:30:04Z

Will take as #414

TheBlueMatt added this to the 0.0.10 milestone Jul 19, 2019

TheBlueMatt mentioned this pull request Jul 19, 2019

2019 04 in flight txn tracking clean #336

Merged

TheBlueMatt reviewed Oct 31, 2019

View reviewed changes

TheBlueMatt reviewed Nov 12, 2019

View reviewed changes

TheBlueMatt reviewed Dec 2, 2019

View reviewed changes

Antoine Riard added 2 commits December 4, 2019 17:21

Add MIN_RELAY_FEE_SAT_PER_1000_WEIGHT

b43e51f

Hardcode min relay fee as its value is fixed on the bitcoin network and updating it would be done really conservatively.

Add log_trace on bump candidates tracking-buffer insertions

742594a

TheBlueMatt reviewed Dec 5, 2019

View reviewed changes

TheBlueMatt mentioned this pull request Dec 5, 2019

Replace keys API with Signer API to support hardware wallets eventually #404

Merged

Antoine Riard added 9 commits December 6, 2019 18:29

Remove superflous pending_claims

be619b5

As local onchain txn are already monitored in block_connected by check_spend_local_transaction, it's useless to generate twice pending claims for HTLC outputs on local commitment tx. We could do the alternative.

Add RBF-bumping of preimage/timeout txn on remote HTLC outputs

09890f3

Given they are only signed by us we can RBF at wish Fix tests broken by introduction of more txn broadcast (channel_monitor_network_test) Add locktime in RemoteHTLC as it's needed to generate timeout txn.

Add test_bump_penalty_txn_on_revoked_commitment

7b93c48

Test multiple rounds of 25% heuristic in bump_claim_tx on remote revoked commitment txn with htlcs pending in both directions.

Add test_bump_penalty_txn_on_revoked_htlcs

5c179b2

Add test_bump_penalty_txn_on_remote_commitment

3cae707

Add test_set_outpoints_partial_claiming

9284a2b

TheBlueMatt closed this Dec 9, 2019

Bump Engine : implement RBF for some timely channel transactions #347

Bump Engine : implement RBF for some timely channel transactions #347

Uh oh!

Conversation

ariard commented Jul 8, 2019

Uh oh!

TheBlueMatt commented Jul 31, 2019

Uh oh!

codecov bot commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ariard commented Oct 26, 2019

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Nov 13, 2019

Uh oh!

TheBlueMatt commented Nov 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Dec 2, 2019

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Dec 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Aug 1, 2019 •

edited

Loading