[Ready for Review] Track in-flight solving tx to delay HTLC failure update until enough confirmations #305
Conversation
|
All cases should be covered, just need to complete tests: Delayed-Failures-until-maturation-cases :
Canceled-Failures :
|
|
Heey, should be good now. Modify a lot of tests due to introduction of delay in both ChannelMonitor and ChannelManager. Hope I don't change semantic of them while fixing |
|
Rebased, and fix full_stack_target break |
There was a problem hiding this comment.
Thanks so much for tackling this! It generally looks pretty good, and I'm happy to take it as-is as an intermediate step, but I think it would be nice to go ahead and start by tracking per-tx-that-claims instead of just height. I'm pleasantly surprised by how little diff this required, or maybe you just did it really well :p.
| // In case of reorg we may have htlc outputs solved in a different way so | ||
| // we prefer to keep claims but don't store duplicate updates for a given | ||
| // (payment_hash, HTLCSource) pair. | ||
| // TODO: Note that we currently don't really use this as ChannelManager |
There was a problem hiding this comment.
Hmm, because the pending updates are stored in ChannelMonitor instead of ManyChannelMonitor (so that we can persist them), the de-duplicating code here should never trigger - it should be safe to always just push (if we want we can add debug_assert checks that its unique so that we'll catch it in fuzzing if its actually hit, but no reason to take the runtime performance penalty here).
There was a problem hiding this comment.
Okay, move the de-duplicating code at ChannelMonitor, a little bit simpler (even if not most efficient way due to HahsMap<HTLCSource, update> being non-implementable due to Hash trait absent on SecretKey)
| log_trace!(self, "Failing HTLC with payment_hash {} from {} remote commitment tx due to broadcast of revoked remote commitment transaction", log_bytes!(htlc.payment_hash.0), $commitment_tx); | ||
| htlc_updated.push(((**source).clone(), None, htlc.payment_hash.clone())); | ||
| log_trace!(self, "Failing HTLC with payment_hash {} from {} remote commitment tx due to broadcast of revoked remote commitment transaction, waiting confirmation until {} height", log_bytes!(htlc.payment_hash.0), $commitment_tx, height + HTLC_FAIL_ANTI_REORG_DELAY - 1); | ||
| match self.htlc_updated_waiting_threshold_conf.entry(height + HTLC_FAIL_ANTI_REORG_DELAY - 1) { |
There was a problem hiding this comment.
Hmm, we don't really want to just wait for some number of blocks, we really want to wait for min(htlc_expiry - some buffer(I presume HTLC_FAIL_TIMEOUT_BLOCKS), the txn in txn_to_broadcast are confirmed with at least HTLC_FAIL_ANTI_REORG_DELAY confirmations). This requires some more tracking, but I think this enables some really nice additional features - ideally we'd track the txn_to_broadcast here (or maybe the data that went into creating them) and be able to recreate them with a bumped fee as time passes.
Separating the tracking to per-HTLC-by-tx also allows us to fail backwards HTLCs that are near-expiry while holding on to others as we await confirmation (and separate out feerates for each class).
There was a problem hiding this comment.
Well, there L1192, its revoked HTLC outputs which are concerned, so we really care for maturation of revoked commitment transaction, their htlc_expiry may already been expired ? And in other places it's from broadcasting of failure-trigger tx we start timer, so don't get you on min(htlc_expiry - some_buffer) ?
Timely failure of inbound HTLC may triggered by some per-tx-that-claims (like a HTLC-timeout) but IMO it's 2 different problems : waiting delay A for maturation of failure-trigger tx and waiting delay B before bumping timeout/claim tx. If so, shouldn't be hard to track txn_broadcast in check_spend_local_tx/check_spend_remote_tx, and at height timer expiration regenerate tx with higher fee, but seems to me it merits its own PR.
|
Rebased, bf7f26a differs slightly from its previous state due to the fact I forgot at first to prune channel closing in case of block disconnected |
Broadcasting a commitment tx means that we have to fail inbound HTLC in backward channel. Doing it prematurely would put us at risk in case of reorg. So we delay passing failure update upstream until solving tx mature to HTLC_FAIL_ANTI_ REORG_DELAY. Requirements differ if HTLC is a revoked/non-revoked dust/ non-revoked non-dust one. Add connect_blocks in test_utils to fix broken tests due to anti-reorg delay enforcement Remove anti-duplicate htlc update stuff in ManySimpleChannelMonitor
Modify ChainListener API by adding height field to block_disconnect
Fix tests broken by introduced change
Add test_failure_delay_htlc_local_commitment Move some bits of check_spend_remote as we need to fail dust HTLCs which can be spread on both prev/lastest local commitment tx
Add pruning of waiting-conf channel closing at block_disconnect Fix tests broken by introduced change
|
Closing, #336 is doing the job |
2 participants
WIP, need to implement block_disconnected, fix tests (was expected they break) and surely prune duplicate stuff in is_resolving_htlc_output.