347 with travis fix and a few nits. #414

TheBlueMatt · 2019-12-09T22:29:44Z

Changes are:

1.22 compile fix,
impl Writable/Readable for ClaimTxBumpMaterial, and move the impl of InputMaterial from a later commit up,
add one commit to bump even if tx size changes with the same feerate info.
tidied up commit history between commits (no actual diff from 347).
a few commits on the end that fix bugs that I think exist that I saw during review, these need test fixes/new tests to demonstrate the bugs (or lack thereof).

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

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

While our fee may change wildly (or even go down), the previous fee *rate* is still valid, and we should use that as the basis for our RBF.

This resolves a regression introduced in "Implement bumping engine in ChannelMonitor::block_connected" in which not all inputs are checked. Several opportunities to clarify and clean up comments are also taken. Fix test_bump_penalty_txn_on_revoked_htlcs as now remote claim txn build the same way than us are going to be register as cleaning pending_claim_request after ANTI_REORG_DELAY. It means during this delay we are going to generate invalid bumped claiming txn on already claimed outpoints. Previously these txn weren't issued because all their outpoints would have been removed. Fix full_stack_target by adding more input for FuzzEstimator

This resolves an issue where we will never track 2 on-chain events which are waiting for ANTI_REORG_DELAY at the same height. This partially reverts and fixes "Move our_claim_txn_waiting_first_conf to pending_claim_requests".

When claimable_outpoints was introduced in "Move our_claim_txn_waiting_first_conf to pending_claim_requests", removal of elements from it (which are just pointers into pending_claim_requests) was never added.

ariard

Ok my branch with full_stack_targets fix/more tests/more sanitization/more logs : https://github.com/ariard/rust-lightning/commits/bump-engine-better

All tests passed locally.

ariard · 2019-12-10T22:44:06Z

lightning/src/ln/channelmonitor.rs

@@ -2396,9 +2396,9 @@ impl ChannelMonitor {
 			// Scan all input to verify is one of the outpoint spent is of interest for us
 			let mut claimed_outputs_material = Vec::new();
 			for inp in &tx.input {
-				if let Some(ancestor_claimable_txid) = self.claimable_outpoints.get(&inp.previous_output) {
+				if let Some(first_claim_txid_height) = self.claimable_outpoints.get(&inp.previous_output) {


Why first_claim_txid_height?, it's still Sha256dHash there?

its a pair - .0 is a Sha256dHash, .1 is the height at which it was added.

first_claim_txid_and_height but nevermind :p

Extend test visibility of claim-tracking maps to do so. Cover both "If 2 claimable-outpoint-spending txn are in 1 block, clean up properly" and "Clean up claimable_outpoints when pending_claim_requests is cleaned" fix commits in same patchset.

TheBlueMatt mentioned this pull request Dec 9, 2019

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

Closed

TheBlueMatt added this to the 0.0.10 milestone Dec 9, 2019

TheBlueMatt force-pushed the 2019-12-347-nits branch from 03b9960 to d73c2b7 Compare December 9, 2019 23:43

Antoine Riard added 5 commits December 9, 2019 22:19

Add MIN_RELAY_FEE_SAT_PER_1000_WEIGHT

201fb4b

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

09ca1e9

Remove superflous pending_claims

6a7c471

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.

TheBlueMatt force-pushed the 2019-12-347-nits branch 2 times, most recently from 144896a to 91ad422 Compare December 10, 2019 03:54

Antoine Riard and others added 12 commits December 10, 2019 15:50

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

6eb290d

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

4e63dff

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

d2b5999

Add test_bump_penalty_txn_on_remote_commitment

a0373dc

Add test_set_outpoints_partial_claiming

d421816

Dont forget RBF previous-feerate info if tx size changed.

7be903b

While our fee may change wildly (or even go down), the previous fee *rate* is still valid, and we should use that as the basis for our RBF.

Log block tick in ChannelMonitor

9eb8780

Clean up claimable_outpoints when pending_claim_requests is cleaned

ae042eb

When claimable_outpoints was introduced in "Move our_claim_txn_waiting_first_conf to pending_claim_requests", removal of elements from it (which are just pointers into pending_claim_requests) was never added.

Dont RBF a tx twice if it hits RBF timer when one input is spent

6a775ea

ariard reviewed Dec 10, 2019

View reviewed changes

Antoine Riard and others added 3 commits December 10, 2019 19:35

Sanitize pending_claim_requests if no more outpoints to claim

0150b1f

Add test_bump_txn_sanitize_tracking_maps

85c03c1

Extend test visibility of claim-tracking maps to do so. Cover both "If 2 claimable-outpoint-spending txn are in 1 block, clean up properly" and "Clean up claimable_outpoints when pending_claim_requests is cleaned" fix commits in same patchset.

Avoid cloning RBF state when we just want to modify fields.

4275b77

TheBlueMatt force-pushed the 2019-12-347-nits branch from db80067 to 4275b77 Compare December 11, 2019 00:39

TheBlueMatt merged commit cd21a35 into lightningdevkit:master Dec 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

347 with travis fix and a few nits. #414

347 with travis fix and a few nits. #414

TheBlueMatt commented Dec 9, 2019 •

edited

Loading

ariard left a comment

ariard Dec 10, 2019

TheBlueMatt Dec 11, 2019

ariard Dec 11, 2019

347 with travis fix and a few nits. #414

347 with travis fix and a few nits. #414

Conversation

TheBlueMatt commented Dec 9, 2019 • edited Loading

ariard left a comment

Choose a reason for hiding this comment

ariard Dec 10, 2019

Choose a reason for hiding this comment

TheBlueMatt Dec 11, 2019

Choose a reason for hiding this comment

ariard Dec 11, 2019

Choose a reason for hiding this comment

TheBlueMatt commented Dec 9, 2019 •

edited

Loading