347 with travis fix and a few nits. by TheBlueMatt · Pull Request #414 · lightningdevkit/rust-lightning

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

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