Handle `transaction_unconfirmed` as a full reorg to the tx height #1846

TheBlueMatt · 2022-11-10T01:09:00Z

In ChannelMonitor, if we see a transaction_unconfirmed for a transaction we last saw in a block at height X, we shouldn't only remove the onchain_events_awaiting_threshold_conf entry for the given tx but rather for all transactions that we last saw at height >= X.

This avoids any potential onchain_events_awaiting_threshold_conf inconsistencies due to the order in whcih users mark transactions unconfirmed (which the chain::Confirm docs do not currently set any requirements on).

This also matches the OnchainTxHandler behavior, which does the same lookup.

ariard

If so, I think it should be noted somewhere that our default implementation of Chain::transaction_unconfirmed in fact alleviate the caller to trigger it for all transaction returned by get_relevant_txids. At least clarify things.

ariard · 2022-11-10T02:23:58Z

lightning/src/chain/channelmonitor.rs

+
+		if let Some(height) = height {
+			log_info!(logger, "transaction_unconfirmed of txid {} implies height {} was reorg'd out", txid, height);
+			self.onchain_events_awaiting_threshold_conf.retain(|ref entry| entry.height < height);


Okay I think it's correct to not call ChannelMonitor::block_disconnected as chain::Confirm doc implies to call best_block_updated. This method updates our best_block and call OnchainTxHandler::block_disconnected.

ariard · 2022-11-10T02:28:33Z

lightning/src/chain/channelmonitor.rs

+		}
+
+		debug_assert!(!self.onchain_events_awaiting_threshold_conf.iter().any(|ref entry| entry.txid == *txid));
+
 		self.onchain_tx_handler.transaction_unconfirmed(txid, broadcaster, fee_estimator, logger);


Well all the OnchainEventEntry do have a txid we could go batch the call to transaction_unconfirmed() ?

Not sure what you're suggesting here? I don't think batching from the user level works, and I'm not sure how we could "batch" without that?

Perhaps @ariard is suggesting that since OnchainTxHandler::transaction_unconfirmed also only requires the txid, which is already present in each OnchainEventEntry, we can call OnchainTxHandler::transaction_unconfirmed for each txid in onchain_events_awaiting_threshold_conf with a height >= the height disconnected. This doesn't seem necessary though as OnchainTxHandler::transaction_unconfirmed/block_disconnected already take care of this.

TheBlueMatt · 2022-11-10T18:00:07Z

If so, I think it should be noted somewhere that our default implementation of Chain::transaction_unconfirmed in fact alleviate the caller to trigger it for all transaction returned by get_relevant_txids. At least clarify things.

I don't think that's true - for example if the user calls get_relevant_txids on a ChainMonitor we only actually disconnect-to-height at the individual ChannelMonitor level. Users really have to keep calling transaction_unconfirmed for all transactions in the relevant list.

wpaulino

This avoids any potential onchain_events_awaiting_threshold_conf
inconsistencies due to the order in whcih users mark transactions
unconfirmed (which the chain::Confirm docs do not currently set
any requirements on).

Wouldn't it be ok as long as users also call best_block_updated for each disconnected block? So this patch keeps things consistent for users that don't, and perhaps only call best_block_updated with the latest tip after the reorg.

wpaulino · 2022-11-10T20:28:48Z

lightning/src/chain/channelmonitor.rs

+		}
+
+		debug_assert!(!self.onchain_events_awaiting_threshold_conf.iter().any(|ref entry| entry.txid == *txid));
+
 		self.onchain_tx_handler.transaction_unconfirmed(txid, broadcaster, fee_estimator, logger);


Perhaps @ariard is suggesting that since OnchainTxHandler::transaction_unconfirmed also only requires the txid, which is already present in each OnchainEventEntry, we can call OnchainTxHandler::transaction_unconfirmed for each txid in onchain_events_awaiting_threshold_conf with a height >= the height disconnected. This doesn't seem necessary though as OnchainTxHandler::transaction_unconfirmed/block_disconnected already take care of this.

TheBlueMatt · 2022-11-10T20:53:58Z

Wouldn't it be ok as long as users also call best_block_updated for each disconnected block? So this patch keeps things consistent for users that don't, and perhaps only call best_block_updated with the latest tip after the reorg.

Hmm, so per the docs we actually don't require calling best_block_updated with the fork point block at all, or any other block disconnections, really (recall that, at least from Bitcoin Core's RPC perspective, the chain never jumps back to the fork point, it just does one big jump from the current tip to the new tip). I think this is okay (at least after this patch), but we could add that requirement if we think its needed?

wpaulino · 2022-11-10T21:16:34Z

Hmm, so per the docs we actually don't require calling best_block_updated with the fork point block at all, or any other block disconnections, really (recall that, at least from Bitcoin Core's RPC perspective, the chain never jumps back to the fork point, it just does one big jump from the current tip to the new tip). I think this is okay (at least after this patch), but we could add that requirement if we think its needed?

It would've been nice to have it documented before, but with this patch I'd say there's no reason for a user to worry about it anymore.

TheBlueMatt · 2022-11-16T20:51:59Z

Tagging 113 cause I don't feel super comfortable with the current code for users doing electrum syncs.

tnull

Two questions/nits, otherwise LGTM.

lightning/src/chain/channelmonitor.rs

codecov-commenter · 2022-11-17T22:33:09Z

Codecov Report

Base: 90.81% // Head: 91.81% // Increases project coverage by +0.99% 🎉

Coverage data is based on head (66d7b7d) compared to base (384c4dc).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1846      +/-   ##
==========================================
+ Coverage   90.81%   91.81%   +0.99%     
==========================================
  Files          89       91       +2     
  Lines       48011    57634    +9623     
  Branches    48011    57634    +9623     
==========================================
+ Hits        43601    52916    +9315     
- Misses       4410     4718     +308

Impacted Files	Coverage Δ
lightning/src/chain/channelmonitor.rs	`90.78% <100.00%> (+0.08%)`	⬆️
lightning/src/util/events.rs	`34.91% <0.00%> (-2.97%)`	⬇️
lightning/src/offers/offer.rs	`92.26% <0.00%> (-2.30%)`	⬇️
lightning/src/util/ser_macros.rs	`87.89% <0.00%> (-0.51%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.73% <0.00%> (-0.31%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/offers/parse.rs	`93.47% <0.00%> (ø)`
lightning/src/ln/reload_tests.rs	`95.24% <0.00%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.72% <0.00%> (+0.05%)`	⬆️
... and 20 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

tnull · 2022-11-18T09:01:29Z

LGTM, feel free to squash!

In `ChannelMonitor`, if we see a `transaction_unconfirmed` for a transaction we last saw in a block at height X, we shouldn't *only* remove the `onchain_events_awaiting_threshold_conf` entry for the given tx but rather for all transactions that we last saw at height >= X. This avoids any potential `onchain_events_awaiting_threshold_conf` inconsistencies due to the order in whcih users mark transactions unconfirmed (which the `chain::Confirm` docs do not currently set any requirements on). This also matches the `OnchainTxHandler` behavior, which does the same lookup.

TheBlueMatt · 2022-11-18T20:49:51Z

Rebased without changes.

TheBlueMatt mentioned this pull request Nov 10, 2022

Initial chain access implementation lightningdevkit/ldk-node#9

Closed

ariard reviewed Nov 10, 2022

View reviewed changes

wpaulino reviewed Nov 10, 2022

View reviewed changes

wpaulino previously approved these changes Nov 10, 2022

View reviewed changes

TheBlueMatt added the Seeking Code Review label Nov 10, 2022

TheBlueMatt added this to the 0.0.113 milestone Nov 16, 2022

tnull reviewed Nov 17, 2022

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

tnull removed the Seeking Code Review label Nov 17, 2022

TheBlueMatt dismissed wpaulino’s stale review via 3bb6762 November 17, 2022 22:32

TheBlueMatt force-pushed the 2022-11-more-robust-unconfirmed branch from 3bb6762 to 66d7b7d Compare November 18, 2022 20:49

TheBlueMatt assigned tnull, wpaulino and ariard Nov 18, 2022

tnull approved these changes Nov 18, 2022

View reviewed changes

wpaulino approved these changes Nov 18, 2022

View reviewed changes

TheBlueMatt merged commit d7d3b0e into lightningdevkit:main Nov 19, 2022

TheBlueMatt mentioned this pull request Nov 21, 2022

Ensure transactions_confirmed is idempotent #1861

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle `transaction_unconfirmed` as a full reorg to the tx height #1846

Handle `transaction_unconfirmed` as a full reorg to the tx height #1846

TheBlueMatt commented Nov 10, 2022

ariard left a comment

ariard Nov 10, 2022

ariard Nov 10, 2022

TheBlueMatt Nov 10, 2022

wpaulino Nov 10, 2022 •

edited

Loading

TheBlueMatt commented Nov 10, 2022

wpaulino left a comment

wpaulino Nov 10, 2022 •

edited

Loading

TheBlueMatt commented Nov 10, 2022

wpaulino commented Nov 10, 2022

TheBlueMatt commented Nov 16, 2022

tnull left a comment

codecov-commenter commented Nov 17, 2022 •

edited

Loading

tnull commented Nov 18, 2022

TheBlueMatt commented Nov 18, 2022

Handle transaction_unconfirmed as a full reorg to the tx height #1846

Handle transaction_unconfirmed as a full reorg to the tx height #1846

Conversation

TheBlueMatt commented Nov 10, 2022

ariard left a comment

Choose a reason for hiding this comment

ariard Nov 10, 2022

Choose a reason for hiding this comment

ariard Nov 10, 2022

Choose a reason for hiding this comment

TheBlueMatt Nov 10, 2022

Choose a reason for hiding this comment

wpaulino Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt commented Nov 10, 2022

wpaulino left a comment

Choose a reason for hiding this comment

wpaulino Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt commented Nov 10, 2022

wpaulino commented Nov 10, 2022

TheBlueMatt commented Nov 16, 2022

tnull left a comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 17, 2022 • edited Loading

Codecov Report

tnull commented Nov 18, 2022

TheBlueMatt commented Nov 18, 2022

Handle `transaction_unconfirmed` as a full reorg to the tx height #1846

Handle `transaction_unconfirmed` as a full reorg to the tx height #1846

wpaulino Nov 10, 2022 •

edited

Loading

wpaulino Nov 10, 2022 •

edited

Loading

codecov-commenter commented Nov 17, 2022 •

edited

Loading