Uh oh!
There was an error while loading. Please reload this page.
Move pending-HTLC-updated ChannelMonitor from ManyChannelMonitor - #474
Conversation
ff12ea9 to
3946b2aCompareariard
commented
Feb 4, 2020
Thanks for tackling this, IIRC when I did implement first htlc updates we agreed than the API was temporary so this should be a good improvement. Conflicts are quite minor that's fine. |
TheBlueMatt
commented
Feb 4, 2020
via email
Right, this design made sense pre-HTLCSource, now it makes none. … On Feb 4, 2020, at 02:36, Antoine Riard ***@***.***> wrote:
Thanks for tackling this, IIRC when I did implement first htlc updates we agreed than the API was temporary so this should be a good improvement.
Conflicts are quite minor that's fine.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe. |
3946b2a to
e4a7efdComparee4a7efd to
380c822Compare
ariard
left a comment
There was a problem hiding this comment.
IMO a good occasion to add more reorg test coverage.
| payment_preimages: HashMap<PaymentHash, PaymentPreimage>, | ||
| pending_htlcs_updated: HashMap<PaymentHash, Vec<(HTLCSource, Option<PaymentPreimage>)>>, |
There was a problem hiding this comment.
"Buffer of reorg-safe(ANTI_REORG_DELAY) solved htlcs waiting to be passed upstream to the off-chain state machine. Note hash of HTLC may be duplicated on the network the real_id of HTLC should be short_channel_id+htlc_id+payment_hash".
| fn block_connected(&mut self, txn_matched: &[&Transaction], height: u32, block_hash: &Sha256dHash, broadcaster: &BroadcasterInterface, fee_estimator: &FeeEstimator)-> (Vec<(Sha256dHash, Vec<TxOut>)>, Vec<SpendableOutputDescriptor>, Vec<(HTLCSource, Option<PaymentPreimage>, PaymentHash)>) { | ||
| fn append_htlc_updated(&mut self, mut htlc_updated_infos: Vec<(HTLCSource, Option<PaymentPreimage>, PaymentHash)>) { | ||
| // ChannelManager will just need to fetch pending_htlcs_updated and pass state backward |
| hash_map::Entry::Occupied(mut e) => { | ||
| // 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. |
There was a problem hiding this comment.
Hmmmm in fact comment isn't that much accurate, if we assume ANTI_REORG_DELAY is safe, the only reason we have this logic is because block-rescan else we shouldn't get the update thanks to onchain_events_waiting_threshold_conf.
| // (payment_hash, HTLCSource) pair. | ||
| let mut existing_claim = false; | ||
| e.get_mut().retain(|htlc_data| { | ||
| if htlc.0 == htlc_data.0 { |
There was a problem hiding this comment.
"If HTLC is already solved...and we have preimage don't update...if we have a timeout update...if hash1 == hash2 keep hash1 solution"
| OnchainEvent::HTLCUpdate { htlc_update } => { | ||
| log_trace!(self, "HTLC {} failure update has got enough confirmations to be passed upstream", log_bytes!((htlc_update.1).0)); | ||
| htlc_updated.push((htlc_update.0, None, htlc_update.1)); | ||
| self.append_htlc_updated(vec![(htlc_update.0, None, htlc_update.1)]); |
There was a problem hiding this comment.
The buffer onchain_events_waiting_threshold_conf is design such than it should be reorg-safe to pass unfriendly state backward, but I think we don't test the first-seeing-timeout-but-then-a-preimage case (check test pair test_htlc_on_chain_success/test_htlc_on_chain_timeout). If so would be great to add test.
380c822 to
26aafd0CompareTheBlueMatt
commented
Feb 19, 2020
Right, good points - as you pointed out having an HashMap for it is now completely useless, so I just dropped that whole complexity and added a (somewhat) basic test. Let me know if this solved all your comments. |
26aafd0 to
0d0558aCompareariard
left a comment
There was a problem hiding this comment.
Code is good but I think some new tests are redundant
| } | ||
| header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 }; | ||
| nodes[1].block_notifier.block_connected(&Block { header, txdata: claim_txn }, CHAN_CONFIRM_DEPTH + 1); |
There was a problem hiding this comment.
Why no connect just one block here ? should be enough
There was a problem hiding this comment.
We do only connect one block here? We just connect it at height CHAN_CONFIRM_DEPTH + 1.
| } else { | ||
| // Confirm the timeout tx and check that we fail the HTLC backwards | ||
| header = BlockHeader { version: 0x20000000, prev_blockhash: header.bitcoin_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 }; | ||
| nodes[1].block_notifier.block_connected_checked(&header, CHAN_CONFIRM_DEPTH + ANTI_REORG_DELAY, &vec![], &[0; 0]); |
There was a problem hiding this comment.
Why not connect ANTI_REORG_DELAY blocks here?
There was a problem hiding this comment.
We already connected ANTI_REORG_DELAY - 1 blocks above, this is just the last one to get it to ANTI_REORG_DELAY.
| do_test_onchain_htlc_reorg(true, true); | ||
| } | ||
| #[test] | ||
| fn test_onchain_htlc_timeout_delay_local_commitment() { |
There was a problem hiding this comment.
I think both local/remote cases are already covered by test_htlc_on_chain_timeout?
There was a problem hiding this comment.
Hmm, probably. I still like checking that the one-block-difference is exactly the same but does something different. If you really feel strongly I can remove it.
0d0558a to
bc52283CompareThis is important for a number of reasons: * Firstly, I hit this trying to implement rescan in the demo bitcoinrpc client - if individual ChannelMonitors are out of sync with each other, we cannot add them all into a ManyChannelMonitor together and then rescan, but need to rescan them individually without having to do a bunch of manual work. Of the three return values in ChannelMonitor::block_connected, only the HTLCsource stuff that is moved here makes no sense to be exposed to the user. * Secondly, the logic currently in ManyChannelMonitor cannot be reproduced by the user! HTLCSource is deliberately an opaque type but we use its data to decide which things to keep when inserting into the HashMap. This would prevent a user from properly implementing a replacement ManyChannelMonitor, which is unacceptable. * Finally, by moving the tracking into ChannelMonitor, we can serialize them out, which prevents us from forgetting them when loading from disk, though there are still other races which need to be handled to make this fully safe (see TODOs in ChannelManager). This is safe as no two entries can have the same HTLCSource across different channels (or, if they did, it would be a rather serious bug), though note that, IIRC, when this code was added, the HTLCSource field in the values was not present. We also take this opportunity to rename the fetch function to match our other event interfaces, makaing it clear that by calling the function the set of HTLCUpdates will also be cleared.
bc52283 to
d296360Compareariard
commented
Feb 21, 2020
ACK d296360 |
This is important for a number of reasons:
bitcoinrpc client - if individual ChannelMonitors are out of
sync with each other, we cannot add them all into a
ManyChannelMonitor together and then rescan, but need to rescan
them individually without having to do a bunch of manual work.
Of the three return values in ChannelMonitor::block_connected,
only the HTLCsource stuff that is moved here makes no sense to
be exposed to the user.
reproduced by the user! HTLCSource is deliberately an opaque
type but we use its data to decide which things to keep when
inserting into the HashMap. This would prevent a user from
properly implementing a replacement ManyChannelMonitor, which is
unacceptable.
serialize them out, which prevents us from forgetting them when
loading from disk, though there are still other races which need
to be handled to make this fully safe (see TODOs in
ChannelManager).
This is safe as no two entries can have the same HTLCSource across
different channels (or, if they did, it would be a rather serious
bug), though note that, IIRC, when this code was added, the
HTLCSource field in the values was not present.
We also take this opportunity to rename the fetch function to match
our other event interfaces, makaing it clear that by calling the
function the set of HTLCUpdates will also be cleared.
CC @ariard. I dont think this should conflict too much with your existing work, as its a pretty straightforward on-its-own change.