Track confirmation block hash and return via Confirm::get_relevant_txids

[tnull]

Previously, Confirm::get_relevant_txids() only returned a list of transactions that have to be monitored for reorganization out of the chain. This interface however required a double bookkeeping: while we internally keep track of the best block, height, etc, it would also require the user to keep track which transaction was previously confirmed in which block and to take actions based on any change, e.g, to reconfirm them when the block would be reorged-out and the transactions had been reconfirmed in another block.

Here, we track the confirmation block hash internally and return it via Confirm::get_relevant_txids() to the user, which alleviates the requirement for double bookkeeping: the user can now simply check whether the given transactions are still confirmed via the given blocks, and take action if they are not.

[tnull]

Marked draft for now, will switch after concept ACK.

[codecov-commenter]

Codecov Report

Base: 90.78% // Head: 91.23% // Increases project coverage by +0.44% 🎉

Coverage data is based on head (de9cd52) compared to base (e55e0d5).
Patch coverage: 89.39% of modified lines in pull request are covered.

❗ Current head de9cd52 differs from pull request most recent head 9685d6c. Consider uploading reports for the commit 9685d6c to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1796 +/- ##
==========================================
+ Coverage 90.78% 91.23% +0.44% 
==========================================
Files 87 89 +2 Lines 47604 50973 +3369 Branches 47604 50973 +3369 ==========================================
+ Hits 43218 46503 +3285 - Misses 4386 4470 +84 

Impacted Files	Coverage Δ
lightning/src/chain/chainmonitor.rs	97.78% <ø> (ø)
lightning/src/chain/mod.rs	68.18% <ø> (ø)
lightning/src/chain/channelmonitor.rs	90.69% <80.64%> (+0.04%)	⬆️
lightning/src/chain/onchaintx.rs	95.08% <90.00%> (+0.07%)	⬆️
lightning/src/ln/channel.rs	88.71% <100.00%> (+<0.01%)	⬆️
lightning/src/ln/channelmanager.rs	88.85% <100.00%> (+3.44%)	⬆️
lightning/src/ln/reorg_tests.rs	100.00% <100.00%> (ø)
lightning-net-tokio/src/lib.rs	76.73% <0.00%> (-0.31%)	⬇️
lightning/src/ln/functional_tests.rs	96.95% <0.00%> (-0.11%)	⬇️
lightning/src/lib.rs	100.00% <0.00%> (ø)
... and 12 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]

Marked draft for now, will switch after concept ACK.

Just discussed this with Matt in Discord, I'll count this as concept ACK.

[arik-so]

Would it be helpful to also return the confirmation height, so a user could trivially verify if a transaction has been dropped by knowing the depth of recent reorgs?

[tnull]

Would it be helpful to also return the confirmation height, so a user could trivially verify if a transaction has been dropped by knowing the depth of recent reorgs?

Not sure, as the height to compare it to is always due to change meaning it may be ambiguous what 'recent reorgs' are. So including height might invite users to do some comparisons that are likely gonna be race-y again?

[wpaulino]

LGTM, feel free to squash.

[TheBlueMatt]

This really needs some kind of testing. It should be pretty straightforward to just add relevant assertions in some of the existing reorg tests, I hope.

[tnull]

This really needs some kind of testing. It should be pretty straightforward to just add relevant assertions in some of the existing reorg tests, I hope.

Now added a test that we're indeed getting the expected confirmation block hash with c937807. Let me know if it's fine to squash.

[TheBlueMatt]

I know its not really your fault, but man this is confusing - update_claims_view takes block hash and height parameters for "what block/height this tx was included in" but is often called without any txn and dummy (current block) values. Can we split the function in two and have the part that operates on txn_matched be separate? Should be a straightforward change and would make this PR much more clearly correct.

[tnull]

Now split the method into update_claims_view_from_requests and update_claims_view_from_matched_txn in 0f9fc8a. I also DRYed the onchain event processing that would always run into process_onchain_events_awaiting_threshold_conf.

[TheBlueMatt]

Should drop the conf_height - nothing is being confirmed. Can also drop the corresponding docs which talk about txn_matched.

Can you fix the above docs on conf_height which mentions txn_matched?

[tnull]

Done hope I'm not misinterpreting something here:

$ git diff-tree -U2 0f9fc8a 84073d
diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index ee557eb7..8fbfa215 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -552,7 +552,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// preimage after force-close.
///
- /// `conf_height` represents the height at which the transactions in `txn_matched` were
- /// confirmed. This does not need to equal the current blockchain tip height, which should be
- /// provided via `cur_height`, however it must never be higher than `cur_height`.
+ /// `conf_height` represents the minimal height at which the request could get confirmed. This
+ /// does not need to equal the current blockchain tip height, which should be provided via
+ /// `cur_height`, however it must never be higher than `cur_height`.
pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(&mut self,
requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B,

[TheBlueMatt]

Basically LGTM, feel free to squash from my PoV, but will need re-review from multiple reviewers here.

[tnull]

Squashed commits.

[wpaulino]

LGTM

[jkczyz]

Just some nits.

[tnull]

Addressed nits, squashed again (and accidentally rebased on main in the process... Sorry 😢 )

[tnull]

Recovered the most recent base before the accident. So the latest squash diff is:

> git diff-tree -U2 1a9fec5 1a80eac
diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index aa1116ae..14b5a298 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -555,10 +555,11 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// does not need to equal the current blockchain tip height, which should be provided via
/// `cur_height`, however it must never be higher than `cur_height`.
-	pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(&mut self,
- requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B,
- fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
- where B::Target: BroadcasterInterface,
- F::Target: FeeEstimator,
- L::Target: Logger,
+	pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(
+ &mut self, requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32,
+ broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+	) where
+ B::Target: BroadcasterInterface,
+ F::Target: FeeEstimator,
+ L::Target: Logger,
{
log_debug!(logger, "Updating claims view at height {} with {} claim requests", cur_height, requests.len());
@@ -654,10 +655,11 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// confirmed. This does not need to equal the current blockchain tip height, which should be
/// provided via `cur_height`, however it must never be higher than `cur_height`.
-	pub(crate) fn update_claims_view_from_matched_txn<B: Deref, F: Deref, L: Deref>(&mut self,
- txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash, cur_height: u32,
- broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
- where B::Target: BroadcasterInterface,
- F::Target: FeeEstimator,
- L::Target: Logger,
+	pub(crate) fn update_claims_view_from_matched_txn<B: Deref, F: Deref, L: Deref>(
+ &mut self, txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash,
+ cur_height: u32, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+	) where
+ B::Target: BroadcasterInterface,
+ F::Target: FeeEstimator,
+ L::Target: Logger,
{
log_debug!(logger, "Updating claims view at height {} with {} matched transactions in block {}", cur_height, txn_matched.len(), conf_height);
diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs
index 6d4fe16f..1e26d339 100644
--- a/lightning/src/ln/reorg_tests.rs
+++ b/lightning/src/ln/reorg_tests.rs
@@ -285,7 +285,6 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
assert_eq!(relevant_txids.len(), 1);
let block_hash_opt = relevant_txids[0].1;
- assert!(block_hash_opt.is_some());
let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
- assert_eq!(block_hash_opt.unwrap(), expected_hash);
+ assert_eq!(block_hash_opt, Some(expected_hash));
let txid = relevant_txids[0].0;
assert_eq!(txid, chan.3.txid());
@@ -364,7 +363,6 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
assert_eq!(relevant_txids.len(), 1);
let block_hash_opt = relevant_txids[0].1;
- assert!(block_hash_opt.is_some());
let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
- assert_eq!(block_hash_opt.unwrap(), expected_hash);
+ assert_eq!(block_hash_opt, Some(expected_hash));
let txid = relevant_txids[0].0;
assert_eq!(txid, chan.3.txid());

[TheBlueMatt]

FWIW, in the future, the update_claims_view split could be in a separate commit. Not worth trying to split it now that its done, though.

[tnull]

FWIW, in the future, the update_claims_view split could be in a separate commit. Not worth trying to split it now that its done, though.

Alright, split it out again.

[TheBlueMatt]

Two doc nits, feel free to ignore unless/until you have more review comments from others.

[tnull]

Addressed nits and squashed again, CI should be happy now.

[jkczyz]

Addressed nits and squashed again, CI should be happy now.

Didn't we go with two commits earlier?

[TheBlueMatt]

Yea, we did split it before, but not sure its worth bothering to hold this up more for that - the total diff is only +129 −60 so its not like its big enough that its hard to read.

[tnull]

Previously, Confirm::get_relevant_txids() only returned a list of transactions that have to be monitored for reorganization out of the chain. This interface however required a double bookkeeping: while we internally keep track of the best block, height, etc, it would also require the user to keep track which transaction was previously confirmed in which block and to take actions based on any change, e.g, to reconfirm them when the block would be reorged-out and the transactions had been reconfirmed in another block.

Here, we track the confirmation block hash internally and return it via Confirm::get_relevant_txids() to the user, which alleviates the requirement for double bookkeeping: the user can now simply check whether the given transactions are still confirmed via the given blocks, and take action if they are not.

[tnull]

Marked draft for now, will switch after concept ACK.

[codecov-commenter]

Codecov Report

Base: 90.78% // Head: 91.23% // Increases project coverage by +0.44% 🎉

Coverage data is based on head (de9cd52) compared to base (e55e0d5).
Patch coverage: 89.39% of modified lines in pull request are covered.

❗ Current head de9cd52 differs from pull request most recent head 9685d6c. Consider uploading reports for the commit 9685d6c to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1796 +/- ##
==========================================
+ Coverage 90.78% 91.23% +0.44% 
==========================================
Files 87 89 +2 Lines 47604 50973 +3369 Branches 47604 50973 +3369 ==========================================
+ Hits 43218 46503 +3285 - Misses 4386 4470 +84 

Impacted Files	Coverage Δ
lightning/src/chain/chainmonitor.rs	97.78% <ø> (ø)
lightning/src/chain/mod.rs	68.18% <ø> (ø)
lightning/src/chain/channelmonitor.rs	90.69% <80.64%> (+0.04%)	⬆️
lightning/src/chain/onchaintx.rs	95.08% <90.00%> (+0.07%)	⬆️
lightning/src/ln/channel.rs	88.71% <100.00%> (+<0.01%)	⬆️
lightning/src/ln/channelmanager.rs	88.85% <100.00%> (+3.44%)	⬆️
lightning/src/ln/reorg_tests.rs	100.00% <100.00%> (ø)
lightning-net-tokio/src/lib.rs	76.73% <0.00%> (-0.31%)	⬇️
lightning/src/ln/functional_tests.rs	96.95% <0.00%> (-0.11%)	⬇️
lightning/src/lib.rs	100.00% <0.00%> (ø)
... and 12 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]

Marked draft for now, will switch after concept ACK.

Just discussed this with Matt in Discord, I'll count this as concept ACK.

[arik-so]

Would it be helpful to also return the confirmation height, so a user could trivially verify if a transaction has been dropped by knowing the depth of recent reorgs?

[tnull]

Would it be helpful to also return the confirmation height, so a user could trivially verify if a transaction has been dropped by knowing the depth of recent reorgs?

Not sure, as the height to compare it to is always due to change meaning it may be ambiguous what 'recent reorgs' are. So including height might invite users to do some comparisons that are likely gonna be race-y again?

[wpaulino]

LGTM, feel free to squash.

[TheBlueMatt]

This really needs some kind of testing. It should be pretty straightforward to just add relevant assertions in some of the existing reorg tests, I hope.

[tnull]

This really needs some kind of testing. It should be pretty straightforward to just add relevant assertions in some of the existing reorg tests, I hope.

Now added a test that we're indeed getting the expected confirmation block hash with c937807. Let me know if it's fine to squash.

[TheBlueMatt]

I know its not really your fault, but man this is confusing - update_claims_view takes block hash and height parameters for "what block/height this tx was included in" but is often called without any txn and dummy (current block) values. Can we split the function in two and have the part that operates on txn_matched be separate? Should be a straightforward change and would make this PR much more clearly correct.

[tnull]

Now split the method into update_claims_view_from_requests and update_claims_view_from_matched_txn in 0f9fc8a. I also DRYed the onchain event processing that would always run into process_onchain_events_awaiting_threshold_conf.

[TheBlueMatt]

Should drop the conf_height - nothing is being confirmed. Can also drop the corresponding docs which talk about txn_matched.

Can you fix the above docs on conf_height which mentions txn_matched?

[tnull]

Done hope I'm not misinterpreting something here:

$ git diff-tree -U2 0f9fc8a 84073d
diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index ee557eb7..8fbfa215 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -552,7 +552,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// preimage after force-close.
///
- /// `conf_height` represents the height at which the transactions in `txn_matched` were
- /// confirmed. This does not need to equal the current blockchain tip height, which should be
- /// provided via `cur_height`, however it must never be higher than `cur_height`.
+ /// `conf_height` represents the minimal height at which the request could get confirmed. This
+ /// does not need to equal the current blockchain tip height, which should be provided via
+ /// `cur_height`, however it must never be higher than `cur_height`.
pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(&mut self,
requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B,

[TheBlueMatt]

Basically LGTM, feel free to squash from my PoV, but will need re-review from multiple reviewers here.

[tnull]

Squashed commits.

[wpaulino]

LGTM

[jkczyz]

Just some nits.

[tnull]

Addressed nits, squashed again (and accidentally rebased on main in the process... Sorry 😢 )

[tnull]

Recovered the most recent base before the accident. So the latest squash diff is:

> git diff-tree -U2 1a9fec5 1a80eac
diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index aa1116ae..14b5a298 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -555,10 +555,11 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// does not need to equal the current blockchain tip height, which should be provided via
/// `cur_height`, however it must never be higher than `cur_height`.
-	pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(&mut self,
- requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B,
- fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
- where B::Target: BroadcasterInterface,
- F::Target: FeeEstimator,
- L::Target: Logger,
+	pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(
+ &mut self, requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32,
+ broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+	) where
+ B::Target: BroadcasterInterface,
+ F::Target: FeeEstimator,
+ L::Target: Logger,
{
log_debug!(logger, "Updating claims view at height {} with {} claim requests", cur_height, requests.len());
@@ -654,10 +655,11 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// confirmed. This does not need to equal the current blockchain tip height, which should be
/// provided via `cur_height`, however it must never be higher than `cur_height`.
-	pub(crate) fn update_claims_view_from_matched_txn<B: Deref, F: Deref, L: Deref>(&mut self,
- txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash, cur_height: u32,
- broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
- where B::Target: BroadcasterInterface,
- F::Target: FeeEstimator,
- L::Target: Logger,
+	pub(crate) fn update_claims_view_from_matched_txn<B: Deref, F: Deref, L: Deref>(
+ &mut self, txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash,
+ cur_height: u32, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+	) where
+ B::Target: BroadcasterInterface,
+ F::Target: FeeEstimator,
+ L::Target: Logger,
{
log_debug!(logger, "Updating claims view at height {} with {} matched transactions in block {}", cur_height, txn_matched.len(), conf_height);
diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs
index 6d4fe16f..1e26d339 100644
--- a/lightning/src/ln/reorg_tests.rs
+++ b/lightning/src/ln/reorg_tests.rs
@@ -285,7 +285,6 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
assert_eq!(relevant_txids.len(), 1);
let block_hash_opt = relevant_txids[0].1;
- assert!(block_hash_opt.is_some());
let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
- assert_eq!(block_hash_opt.unwrap(), expected_hash);
+ assert_eq!(block_hash_opt, Some(expected_hash));
let txid = relevant_txids[0].0;
assert_eq!(txid, chan.3.txid());
@@ -364,7 +363,6 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
assert_eq!(relevant_txids.len(), 1);
let block_hash_opt = relevant_txids[0].1;
- assert!(block_hash_opt.is_some());
let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
- assert_eq!(block_hash_opt.unwrap(), expected_hash);
+ assert_eq!(block_hash_opt, Some(expected_hash));
let txid = relevant_txids[0].0;
assert_eq!(txid, chan.3.txid());

[TheBlueMatt]

FWIW, in the future, the update_claims_view split could be in a separate commit. Not worth trying to split it now that its done, though.

[tnull]

FWIW, in the future, the update_claims_view split could be in a separate commit. Not worth trying to split it now that its done, though.

Alright, split it out again.

[TheBlueMatt]

Two doc nits, feel free to ignore unless/until you have more review comments from others.

[tnull]

Addressed nits and squashed again, CI should be happy now.

[jkczyz]

Addressed nits and squashed again, CI should be happy now.

Didn't we go with two commits earlier?

[TheBlueMatt]

Yea, we did split it before, but not sure its worth bothering to hold this up more for that - the total diff is only +129 −60 so its not like its big enough that its hard to read.

[tnull]

Previously, Confirm::get_relevant_txids() only returned a list of transactions that have to be monitored for reorganization out of the chain. This interface however required a double bookkeeping: while we internally keep track of the best block, height, etc, it would also require the user to keep track which transaction was previously confirmed in which block and to take actions based on any change, e.g, to reconfirm them when the block would be reorged-out and the transactions had been reconfirmed in another block.

Here, we track the confirmation block hash internally and return it via Confirm::get_relevant_txids() to the user, which alleviates the requirement for double bookkeeping: the user can now simply check whether the given transactions are still confirmed via the given blocks, and take action if they are not.

[tnull]

Marked draft for now, will switch after concept ACK.

[codecov-commenter]

Codecov Report

Base: 90.78% // Head: 91.23% // Increases project coverage by +0.44% 🎉

Coverage data is based on head (de9cd52) compared to base (e55e0d5).
Patch coverage: 89.39% of modified lines in pull request are covered.

❗ Current head de9cd52 differs from pull request most recent head 9685d6c. Consider uploading reports for the commit 9685d6c to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1796 +/- ##
==========================================
+ Coverage 90.78% 91.23% +0.44% 
==========================================
Files 87 89 +2 Lines 47604 50973 +3369 Branches 47604 50973 +3369 ==========================================
+ Hits 43218 46503 +3285 - Misses 4386 4470 +84 

Impacted Files	Coverage Δ
lightning/src/chain/chainmonitor.rs	97.78% <ø> (ø)
lightning/src/chain/mod.rs	68.18% <ø> (ø)
lightning/src/chain/channelmonitor.rs	90.69% <80.64%> (+0.04%)	⬆️
lightning/src/chain/onchaintx.rs	95.08% <90.00%> (+0.07%)	⬆️
lightning/src/ln/channel.rs	88.71% <100.00%> (+<0.01%)	⬆️
lightning/src/ln/channelmanager.rs	88.85% <100.00%> (+3.44%)	⬆️
lightning/src/ln/reorg_tests.rs	100.00% <100.00%> (ø)
lightning-net-tokio/src/lib.rs	76.73% <0.00%> (-0.31%)	⬇️
lightning/src/ln/functional_tests.rs	96.95% <0.00%> (-0.11%)	⬇️
lightning/src/lib.rs	100.00% <0.00%> (ø)
... and 12 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]

Marked draft for now, will switch after concept ACK.

Just discussed this with Matt in Discord, I'll count this as concept ACK.

[arik-so]

Would it be helpful to also return the confirmation height, so a user could trivially verify if a transaction has been dropped by knowing the depth of recent reorgs?

[tnull]

Would it be helpful to also return the confirmation height, so a user could trivially verify if a transaction has been dropped by knowing the depth of recent reorgs?

Not sure, as the height to compare it to is always due to change meaning it may be ambiguous what 'recent reorgs' are. So including height might invite users to do some comparisons that are likely gonna be race-y again?

[wpaulino]

LGTM, feel free to squash.

[TheBlueMatt]

This really needs some kind of testing. It should be pretty straightforward to just add relevant assertions in some of the existing reorg tests, I hope.

[tnull]

This really needs some kind of testing. It should be pretty straightforward to just add relevant assertions in some of the existing reorg tests, I hope.

Now added a test that we're indeed getting the expected confirmation block hash with c937807. Let me know if it's fine to squash.

[TheBlueMatt]

I know its not really your fault, but man this is confusing - update_claims_view takes block hash and height parameters for "what block/height this tx was included in" but is often called without any txn and dummy (current block) values. Can we split the function in two and have the part that operates on txn_matched be separate? Should be a straightforward change and would make this PR much more clearly correct.

[tnull]

Now split the method into update_claims_view_from_requests and update_claims_view_from_matched_txn in 0f9fc8a. I also DRYed the onchain event processing that would always run into process_onchain_events_awaiting_threshold_conf.

[TheBlueMatt]

Should drop the conf_height - nothing is being confirmed. Can also drop the corresponding docs which talk about txn_matched.

Can you fix the above docs on conf_height which mentions txn_matched?

[tnull]

Done hope I'm not misinterpreting something here:

$ git diff-tree -U2 0f9fc8a 84073d
diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index ee557eb7..8fbfa215 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -552,7 +552,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// preimage after force-close.
///
- /// `conf_height` represents the height at which the transactions in `txn_matched` were
- /// confirmed. This does not need to equal the current blockchain tip height, which should be
- /// provided via `cur_height`, however it must never be higher than `cur_height`.
+ /// `conf_height` represents the minimal height at which the request could get confirmed. This
+ /// does not need to equal the current blockchain tip height, which should be provided via
+ /// `cur_height`, however it must never be higher than `cur_height`.
pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(&mut self,
requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B,

[TheBlueMatt]

Basically LGTM, feel free to squash from my PoV, but will need re-review from multiple reviewers here.

[tnull]

Squashed commits.

[wpaulino]

LGTM

[jkczyz]

Just some nits.

[tnull]

Addressed nits, squashed again (and accidentally rebased on main in the process... Sorry 😢 )

[tnull]

Recovered the most recent base before the accident. So the latest squash diff is:

> git diff-tree -U2 1a9fec5 1a80eac
diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index aa1116ae..14b5a298 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -555,10 +555,11 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// does not need to equal the current blockchain tip height, which should be provided via
/// `cur_height`, however it must never be higher than `cur_height`.
-	pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(&mut self,
- requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B,
- fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
- where B::Target: BroadcasterInterface,
- F::Target: FeeEstimator,
- L::Target: Logger,
+	pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(
+ &mut self, requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32,
+ broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+	) where
+ B::Target: BroadcasterInterface,
+ F::Target: FeeEstimator,
+ L::Target: Logger,
{
log_debug!(logger, "Updating claims view at height {} with {} claim requests", cur_height, requests.len());
@@ -654,10 +655,11 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// confirmed. This does not need to equal the current blockchain tip height, which should be
/// provided via `cur_height`, however it must never be higher than `cur_height`.
-	pub(crate) fn update_claims_view_from_matched_txn<B: Deref, F: Deref, L: Deref>(&mut self,
- txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash, cur_height: u32,
- broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
- where B::Target: BroadcasterInterface,
- F::Target: FeeEstimator,
- L::Target: Logger,
+	pub(crate) fn update_claims_view_from_matched_txn<B: Deref, F: Deref, L: Deref>(
+ &mut self, txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash,
+ cur_height: u32, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+	) where
+ B::Target: BroadcasterInterface,
+ F::Target: FeeEstimator,
+ L::Target: Logger,
{
log_debug!(logger, "Updating claims view at height {} with {} matched transactions in block {}", cur_height, txn_matched.len(), conf_height);
diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs
index 6d4fe16f..1e26d339 100644
--- a/lightning/src/ln/reorg_tests.rs
+++ b/lightning/src/ln/reorg_tests.rs
@@ -285,7 +285,6 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
assert_eq!(relevant_txids.len(), 1);
let block_hash_opt = relevant_txids[0].1;
- assert!(block_hash_opt.is_some());
let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
- assert_eq!(block_hash_opt.unwrap(), expected_hash);
+ assert_eq!(block_hash_opt, Some(expected_hash));
let txid = relevant_txids[0].0;
assert_eq!(txid, chan.3.txid());
@@ -364,7 +363,6 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
assert_eq!(relevant_txids.len(), 1);
let block_hash_opt = relevant_txids[0].1;
- assert!(block_hash_opt.is_some());
let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
- assert_eq!(block_hash_opt.unwrap(), expected_hash);
+ assert_eq!(block_hash_opt, Some(expected_hash));
let txid = relevant_txids[0].0;
assert_eq!(txid, chan.3.txid());

[TheBlueMatt]

FWIW, in the future, the update_claims_view split could be in a separate commit. Not worth trying to split it now that its done, though.

[tnull]

FWIW, in the future, the update_claims_view split could be in a separate commit. Not worth trying to split it now that its done, though.

Alright, split it out again.

[TheBlueMatt]

Two doc nits, feel free to ignore unless/until you have more review comments from others.

[tnull]

Addressed nits and squashed again, CI should be happy now.

[jkczyz]

Addressed nits and squashed again, CI should be happy now.

Didn't we go with two commits earlier?

[TheBlueMatt]

Yea, we did split it before, but not sure its worth bothering to hold this up more for that - the total diff is only +129 −60 so its not like its big enough that its hard to read.

[tnull]

Previously, Confirm::get_relevant_txids() only returned a list of transactions that have to be monitored for reorganization out of the chain. This interface however required a double bookkeeping: while we internally keep track of the best block, height, etc, it would also require the user to keep track which transaction was previously confirmed in which block and to take actions based on any change, e.g, to reconfirm them when the block would be reorged-out and the transactions had been reconfirmed in another block.

Here, we track the confirmation block hash internally and return it via Confirm::get_relevant_txids() to the user, which alleviates the requirement for double bookkeeping: the user can now simply check whether the given transactions are still confirmed via the given blocks, and take action if they are not.

[tnull]

Marked draft for now, will switch after concept ACK.

[codecov-commenter]

Codecov Report

Base: 90.78% // Head: 91.23% // Increases project coverage by +0.44% 🎉

Coverage data is based on head (de9cd52) compared to base (e55e0d5).
Patch coverage: 89.39% of modified lines in pull request are covered.

❗ Current head de9cd52 differs from pull request most recent head 9685d6c. Consider uploading reports for the commit 9685d6c to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1796 +/- ##
==========================================
+ Coverage 90.78% 91.23% +0.44% 
==========================================
Files 87 89 +2 Lines 47604 50973 +3369 Branches 47604 50973 +3369 ==========================================
+ Hits 43218 46503 +3285 - Misses 4386 4470 +84 

Impacted Files	Coverage Δ
lightning/src/chain/chainmonitor.rs	97.78% <ø> (ø)
lightning/src/chain/mod.rs	68.18% <ø> (ø)
lightning/src/chain/channelmonitor.rs	90.69% <80.64%> (+0.04%)	⬆️
lightning/src/chain/onchaintx.rs	95.08% <90.00%> (+0.07%)	⬆️
lightning/src/ln/channel.rs	88.71% <100.00%> (+<0.01%)	⬆️
lightning/src/ln/channelmanager.rs	88.85% <100.00%> (+3.44%)	⬆️
lightning/src/ln/reorg_tests.rs	100.00% <100.00%> (ø)
lightning-net-tokio/src/lib.rs	76.73% <0.00%> (-0.31%)	⬇️
lightning/src/ln/functional_tests.rs	96.95% <0.00%> (-0.11%)	⬇️
lightning/src/lib.rs	100.00% <0.00%> (ø)
... and 12 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]

Marked draft for now, will switch after concept ACK.

Just discussed this with Matt in Discord, I'll count this as concept ACK.

[arik-so]

Would it be helpful to also return the confirmation height, so a user could trivially verify if a transaction has been dropped by knowing the depth of recent reorgs?

[tnull]

Would it be helpful to also return the confirmation height, so a user could trivially verify if a transaction has been dropped by knowing the depth of recent reorgs?

Not sure, as the height to compare it to is always due to change meaning it may be ambiguous what 'recent reorgs' are. So including height might invite users to do some comparisons that are likely gonna be race-y again?

[wpaulino]

LGTM, feel free to squash.

[TheBlueMatt]

This really needs some kind of testing. It should be pretty straightforward to just add relevant assertions in some of the existing reorg tests, I hope.

[tnull]

This really needs some kind of testing. It should be pretty straightforward to just add relevant assertions in some of the existing reorg tests, I hope.

Now added a test that we're indeed getting the expected confirmation block hash with c937807. Let me know if it's fine to squash.

[TheBlueMatt]

I know its not really your fault, but man this is confusing - update_claims_view takes block hash and height parameters for "what block/height this tx was included in" but is often called without any txn and dummy (current block) values. Can we split the function in two and have the part that operates on txn_matched be separate? Should be a straightforward change and would make this PR much more clearly correct.

[tnull]

Now split the method into update_claims_view_from_requests and update_claims_view_from_matched_txn in 0f9fc8a. I also DRYed the onchain event processing that would always run into process_onchain_events_awaiting_threshold_conf.

[TheBlueMatt]

Should drop the conf_height - nothing is being confirmed. Can also drop the corresponding docs which talk about txn_matched.

Can you fix the above docs on conf_height which mentions txn_matched?

[tnull]

Done hope I'm not misinterpreting something here:

$ git diff-tree -U2 0f9fc8a 84073d
diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index ee557eb7..8fbfa215 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -552,7 +552,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// preimage after force-close.
///
- /// `conf_height` represents the height at which the transactions in `txn_matched` were
- /// confirmed. This does not need to equal the current blockchain tip height, which should be
- /// provided via `cur_height`, however it must never be higher than `cur_height`.
+ /// `conf_height` represents the minimal height at which the request could get confirmed. This
+ /// does not need to equal the current blockchain tip height, which should be provided via
+ /// `cur_height`, however it must never be higher than `cur_height`.
pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(&mut self,
requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B,

[TheBlueMatt]

Basically LGTM, feel free to squash from my PoV, but will need re-review from multiple reviewers here.

[tnull]

Squashed commits.

[wpaulino]

LGTM

[jkczyz]

Just some nits.

[tnull]

Addressed nits, squashed again (and accidentally rebased on main in the process... Sorry 😢 )

[tnull]

Recovered the most recent base before the accident. So the latest squash diff is:

> git diff-tree -U2 1a9fec5 1a80eac
diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index aa1116ae..14b5a298 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -555,10 +555,11 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// does not need to equal the current blockchain tip height, which should be provided via
/// `cur_height`, however it must never be higher than `cur_height`.
-	pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(&mut self,
- requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B,
- fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
- where B::Target: BroadcasterInterface,
- F::Target: FeeEstimator,
- L::Target: Logger,
+	pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(
+ &mut self, requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32,
+ broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+	) where
+ B::Target: BroadcasterInterface,
+ F::Target: FeeEstimator,
+ L::Target: Logger,
{
log_debug!(logger, "Updating claims view at height {} with {} claim requests", cur_height, requests.len());
@@ -654,10 +655,11 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// confirmed. This does not need to equal the current blockchain tip height, which should be
/// provided via `cur_height`, however it must never be higher than `cur_height`.
-	pub(crate) fn update_claims_view_from_matched_txn<B: Deref, F: Deref, L: Deref>(&mut self,
- txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash, cur_height: u32,
- broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
- where B::Target: BroadcasterInterface,
- F::Target: FeeEstimator,
- L::Target: Logger,
+	pub(crate) fn update_claims_view_from_matched_txn<B: Deref, F: Deref, L: Deref>(
+ &mut self, txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash,
+ cur_height: u32, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+	) where
+ B::Target: BroadcasterInterface,
+ F::Target: FeeEstimator,
+ L::Target: Logger,
{
log_debug!(logger, "Updating claims view at height {} with {} matched transactions in block {}", cur_height, txn_matched.len(), conf_height);
diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs
index 6d4fe16f..1e26d339 100644
--- a/lightning/src/ln/reorg_tests.rs
+++ b/lightning/src/ln/reorg_tests.rs
@@ -285,7 +285,6 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
assert_eq!(relevant_txids.len(), 1);
let block_hash_opt = relevant_txids[0].1;
- assert!(block_hash_opt.is_some());
let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
- assert_eq!(block_hash_opt.unwrap(), expected_hash);
+ assert_eq!(block_hash_opt, Some(expected_hash));
let txid = relevant_txids[0].0;
assert_eq!(txid, chan.3.txid());
@@ -364,7 +363,6 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
assert_eq!(relevant_txids.len(), 1);
let block_hash_opt = relevant_txids[0].1;
- assert!(block_hash_opt.is_some());
let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
- assert_eq!(block_hash_opt.unwrap(), expected_hash);
+ assert_eq!(block_hash_opt, Some(expected_hash));
let txid = relevant_txids[0].0;
assert_eq!(txid, chan.3.txid());

[TheBlueMatt]

FWIW, in the future, the update_claims_view split could be in a separate commit. Not worth trying to split it now that its done, though.

[tnull]

FWIW, in the future, the update_claims_view split could be in a separate commit. Not worth trying to split it now that its done, though.

Alright, split it out again.

[TheBlueMatt]

Two doc nits, feel free to ignore unless/until you have more review comments from others.

[tnull]

Addressed nits and squashed again, CI should be happy now.

[jkczyz]

Addressed nits and squashed again, CI should be happy now.

Didn't we go with two commits earlier?

[TheBlueMatt]

Yea, we did split it before, but not sure its worth bothering to hold this up more for that - the total diff is only +129 −60 so its not like its big enough that its hard to read.

[tnull]

Previously, Confirm::get_relevant_txids() only returned a list of transactions that have to be monitored for reorganization out of the chain. This interface however required a double bookkeeping: while we internally keep track of the best block, height, etc, it would also require the user to keep track which transaction was previously confirmed in which block and to take actions based on any change, e.g, to reconfirm them when the block would be reorged-out and the transactions had been reconfirmed in another block.

Here, we track the confirmation block hash internally and return it via Confirm::get_relevant_txids() to the user, which alleviates the requirement for double bookkeeping: the user can now simply check whether the given transactions are still confirmed via the given blocks, and take action if they are not.

[tnull]

Marked draft for now, will switch after concept ACK.

[codecov-commenter]

Codecov Report

Base: 90.78% // Head: 91.23% // Increases project coverage by +0.44% 🎉

Coverage data is based on head (de9cd52) compared to base (e55e0d5).
Patch coverage: 89.39% of modified lines in pull request are covered.

❗ Current head de9cd52 differs from pull request most recent head 9685d6c. Consider uploading reports for the commit 9685d6c to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1796 +/- ##
==========================================
+ Coverage 90.78% 91.23% +0.44% 
==========================================
Files 87 89 +2 Lines 47604 50973 +3369 Branches 47604 50973 +3369 ==========================================
+ Hits 43218 46503 +3285 - Misses 4386 4470 +84 

Impacted Files	Coverage Δ
lightning/src/chain/chainmonitor.rs	97.78% <ø> (ø)
lightning/src/chain/mod.rs	68.18% <ø> (ø)
lightning/src/chain/channelmonitor.rs	90.69% <80.64%> (+0.04%)	⬆️
lightning/src/chain/onchaintx.rs	95.08% <90.00%> (+0.07%)	⬆️
lightning/src/ln/channel.rs	88.71% <100.00%> (+<0.01%)	⬆️
lightning/src/ln/channelmanager.rs	88.85% <100.00%> (+3.44%)	⬆️
lightning/src/ln/reorg_tests.rs	100.00% <100.00%> (ø)
lightning-net-tokio/src/lib.rs	76.73% <0.00%> (-0.31%)	⬇️
lightning/src/ln/functional_tests.rs	96.95% <0.00%> (-0.11%)	⬇️
lightning/src/lib.rs	100.00% <0.00%> (ø)
... and 12 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]

Marked draft for now, will switch after concept ACK.

Just discussed this with Matt in Discord, I'll count this as concept ACK.

[arik-so]

Would it be helpful to also return the confirmation height, so a user could trivially verify if a transaction has been dropped by knowing the depth of recent reorgs?

[tnull]

Would it be helpful to also return the confirmation height, so a user could trivially verify if a transaction has been dropped by knowing the depth of recent reorgs?

Not sure, as the height to compare it to is always due to change meaning it may be ambiguous what 'recent reorgs' are. So including height might invite users to do some comparisons that are likely gonna be race-y again?

[wpaulino]

LGTM, feel free to squash.

[TheBlueMatt]

This really needs some kind of testing. It should be pretty straightforward to just add relevant assertions in some of the existing reorg tests, I hope.

[tnull]

This really needs some kind of testing. It should be pretty straightforward to just add relevant assertions in some of the existing reorg tests, I hope.

Now added a test that we're indeed getting the expected confirmation block hash with c937807. Let me know if it's fine to squash.

[TheBlueMatt]

I know its not really your fault, but man this is confusing - update_claims_view takes block hash and height parameters for "what block/height this tx was included in" but is often called without any txn and dummy (current block) values. Can we split the function in two and have the part that operates on txn_matched be separate? Should be a straightforward change and would make this PR much more clearly correct.

[tnull]

Now split the method into update_claims_view_from_requests and update_claims_view_from_matched_txn in 0f9fc8a. I also DRYed the onchain event processing that would always run into process_onchain_events_awaiting_threshold_conf.

[TheBlueMatt]

Should drop the conf_height - nothing is being confirmed. Can also drop the corresponding docs which talk about txn_matched.

Can you fix the above docs on conf_height which mentions txn_matched?

[tnull]

Done hope I'm not misinterpreting something here:

$ git diff-tree -U2 0f9fc8a 84073d
diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index ee557eb7..8fbfa215 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -552,7 +552,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// preimage after force-close.
///
- /// `conf_height` represents the height at which the transactions in `txn_matched` were
- /// confirmed. This does not need to equal the current blockchain tip height, which should be
- /// provided via `cur_height`, however it must never be higher than `cur_height`.
+ /// `conf_height` represents the minimal height at which the request could get confirmed. This
+ /// does not need to equal the current blockchain tip height, which should be provided via
+ /// `cur_height`, however it must never be higher than `cur_height`.
pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(&mut self,
requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B,

[TheBlueMatt]

Basically LGTM, feel free to squash from my PoV, but will need re-review from multiple reviewers here.

[tnull]

Squashed commits.

[wpaulino]

LGTM

[jkczyz]

Just some nits.

[tnull]

Addressed nits, squashed again (and accidentally rebased on main in the process... Sorry 😢 )

[tnull]

Recovered the most recent base before the accident. So the latest squash diff is:

> git diff-tree -U2 1a9fec5 1a80eac
diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index aa1116ae..14b5a298 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -555,10 +555,11 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// does not need to equal the current blockchain tip height, which should be provided via
/// `cur_height`, however it must never be higher than `cur_height`.
-	pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(&mut self,
- requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B,
- fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
- where B::Target: BroadcasterInterface,
- F::Target: FeeEstimator,
- L::Target: Logger,
+	pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(
+ &mut self, requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32,
+ broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+	) where
+ B::Target: BroadcasterInterface,
+ F::Target: FeeEstimator,
+ L::Target: Logger,
{
log_debug!(logger, "Updating claims view at height {} with {} claim requests", cur_height, requests.len());
@@ -654,10 +655,11 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// confirmed. This does not need to equal the current blockchain tip height, which should be
/// provided via `cur_height`, however it must never be higher than `cur_height`.
-	pub(crate) fn update_claims_view_from_matched_txn<B: Deref, F: Deref, L: Deref>(&mut self,
- txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash, cur_height: u32,
- broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
- where B::Target: BroadcasterInterface,
- F::Target: FeeEstimator,
- L::Target: Logger,
+	pub(crate) fn update_claims_view_from_matched_txn<B: Deref, F: Deref, L: Deref>(
+ &mut self, txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash,
+ cur_height: u32, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+	) where
+ B::Target: BroadcasterInterface,
+ F::Target: FeeEstimator,
+ L::Target: Logger,
{
log_debug!(logger, "Updating claims view at height {} with {} matched transactions in block {}", cur_height, txn_matched.len(), conf_height);
diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs
index 6d4fe16f..1e26d339 100644
--- a/lightning/src/ln/reorg_tests.rs
+++ b/lightning/src/ln/reorg_tests.rs
@@ -285,7 +285,6 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
assert_eq!(relevant_txids.len(), 1);
let block_hash_opt = relevant_txids[0].1;
- assert!(block_hash_opt.is_some());
let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
- assert_eq!(block_hash_opt.unwrap(), expected_hash);
+ assert_eq!(block_hash_opt, Some(expected_hash));
let txid = relevant_txids[0].0;
assert_eq!(txid, chan.3.txid());
@@ -364,7 +363,6 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
assert_eq!(relevant_txids.len(), 1);
let block_hash_opt = relevant_txids[0].1;
- assert!(block_hash_opt.is_some());
let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
- assert_eq!(block_hash_opt.unwrap(), expected_hash);
+ assert_eq!(block_hash_opt, Some(expected_hash));
let txid = relevant_txids[0].0;
assert_eq!(txid, chan.3.txid());

[TheBlueMatt]

FWIW, in the future, the update_claims_view split could be in a separate commit. Not worth trying to split it now that its done, though.

[tnull]

FWIW, in the future, the update_claims_view split could be in a separate commit. Not worth trying to split it now that its done, though.

Alright, split it out again.

[TheBlueMatt]

Two doc nits, feel free to ignore unless/until you have more review comments from others.

[tnull]

Addressed nits and squashed again, CI should be happy now.

[jkczyz]

Addressed nits and squashed again, CI should be happy now.

Didn't we go with two commits earlier?

[TheBlueMatt]

Yea, we did split it before, but not sure its worth bothering to hold this up more for that - the total diff is only +129 −60 so its not like its big enough that its hard to read.

[tnull]

Previously, Confirm::get_relevant_txids() only returned a list of transactions that have to be monitored for reorganization out of the chain. This interface however required a double bookkeeping: while we internally keep track of the best block, height, etc, it would also require the user to keep track which transaction was previously confirmed in which block and to take actions based on any change, e.g, to reconfirm them when the block would be reorged-out and the transactions had been reconfirmed in another block.

Here, we track the confirmation block hash internally and return it via Confirm::get_relevant_txids() to the user, which alleviates the requirement for double bookkeeping: the user can now simply check whether the given transactions are still confirmed via the given blocks, and take action if they are not.

[tnull]

Marked draft for now, will switch after concept ACK.

[codecov-commenter]

Codecov Report

Base: 90.78% // Head: 91.23% // Increases project coverage by +0.44% 🎉

Coverage data is based on head (de9cd52) compared to base (e55e0d5).
Patch coverage: 89.39% of modified lines in pull request are covered.

❗ Current head de9cd52 differs from pull request most recent head 9685d6c. Consider uploading reports for the commit 9685d6c to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1796 +/- ##
==========================================
+ Coverage 90.78% 91.23% +0.44% 
==========================================
Files 87 89 +2 Lines 47604 50973 +3369 Branches 47604 50973 +3369 ==========================================
+ Hits 43218 46503 +3285 - Misses 4386 4470 +84 

Impacted Files	Coverage Δ
lightning/src/chain/chainmonitor.rs	97.78% <ø> (ø)
lightning/src/chain/mod.rs	68.18% <ø> (ø)
lightning/src/chain/channelmonitor.rs	90.69% <80.64%> (+0.04%)	⬆️
lightning/src/chain/onchaintx.rs	95.08% <90.00%> (+0.07%)	⬆️
lightning/src/ln/channel.rs	88.71% <100.00%> (+<0.01%)	⬆️
lightning/src/ln/channelmanager.rs	88.85% <100.00%> (+3.44%)	⬆️
lightning/src/ln/reorg_tests.rs	100.00% <100.00%> (ø)
lightning-net-tokio/src/lib.rs	76.73% <0.00%> (-0.31%)	⬇️
lightning/src/ln/functional_tests.rs	96.95% <0.00%> (-0.11%)	⬇️
lightning/src/lib.rs	100.00% <0.00%> (ø)
... and 12 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]

Marked draft for now, will switch after concept ACK.

Just discussed this with Matt in Discord, I'll count this as concept ACK.

[arik-so]

Would it be helpful to also return the confirmation height, so a user could trivially verify if a transaction has been dropped by knowing the depth of recent reorgs?

[tnull]

Would it be helpful to also return the confirmation height, so a user could trivially verify if a transaction has been dropped by knowing the depth of recent reorgs?

Not sure, as the height to compare it to is always due to change meaning it may be ambiguous what 'recent reorgs' are. So including height might invite users to do some comparisons that are likely gonna be race-y again?

[wpaulino]

LGTM, feel free to squash.

[TheBlueMatt]

This really needs some kind of testing. It should be pretty straightforward to just add relevant assertions in some of the existing reorg tests, I hope.

[tnull]

This really needs some kind of testing. It should be pretty straightforward to just add relevant assertions in some of the existing reorg tests, I hope.

Now added a test that we're indeed getting the expected confirmation block hash with c937807. Let me know if it's fine to squash.

[TheBlueMatt]

I know its not really your fault, but man this is confusing - update_claims_view takes block hash and height parameters for "what block/height this tx was included in" but is often called without any txn and dummy (current block) values. Can we split the function in two and have the part that operates on txn_matched be separate? Should be a straightforward change and would make this PR much more clearly correct.

[tnull]

Now split the method into update_claims_view_from_requests and update_claims_view_from_matched_txn in 0f9fc8a. I also DRYed the onchain event processing that would always run into process_onchain_events_awaiting_threshold_conf.

[TheBlueMatt]

Should drop the conf_height - nothing is being confirmed. Can also drop the corresponding docs which talk about txn_matched.

Can you fix the above docs on conf_height which mentions txn_matched?

[tnull]

Done hope I'm not misinterpreting something here:

$ git diff-tree -U2 0f9fc8a 84073d
diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index ee557eb7..8fbfa215 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -552,7 +552,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// preimage after force-close.
///
- /// `conf_height` represents the height at which the transactions in `txn_matched` were
- /// confirmed. This does not need to equal the current blockchain tip height, which should be
- /// provided via `cur_height`, however it must never be higher than `cur_height`.
+ /// `conf_height` represents the minimal height at which the request could get confirmed. This
+ /// does not need to equal the current blockchain tip height, which should be provided via
+ /// `cur_height`, however it must never be higher than `cur_height`.
pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(&mut self,
requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B,

[TheBlueMatt]

Basically LGTM, feel free to squash from my PoV, but will need re-review from multiple reviewers here.

[tnull]

Squashed commits.

[wpaulino]

LGTM

[jkczyz]

Just some nits.

[tnull]

Addressed nits, squashed again (and accidentally rebased on main in the process... Sorry 😢 )

[tnull]

Recovered the most recent base before the accident. So the latest squash diff is:

> git diff-tree -U2 1a9fec5 1a80eac
diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index aa1116ae..14b5a298 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -555,10 +555,11 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// does not need to equal the current blockchain tip height, which should be provided via
/// `cur_height`, however it must never be higher than `cur_height`.
-	pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(&mut self,
- requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B,
- fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
- where B::Target: BroadcasterInterface,
- F::Target: FeeEstimator,
- L::Target: Logger,
+	pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(
+ &mut self, requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32,
+ broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+	) where
+ B::Target: BroadcasterInterface,
+ F::Target: FeeEstimator,
+ L::Target: Logger,
{
log_debug!(logger, "Updating claims view at height {} with {} claim requests", cur_height, requests.len());
@@ -654,10 +655,11 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// confirmed. This does not need to equal the current blockchain tip height, which should be
/// provided via `cur_height`, however it must never be higher than `cur_height`.
-	pub(crate) fn update_claims_view_from_matched_txn<B: Deref, F: Deref, L: Deref>(&mut self,
- txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash, cur_height: u32,
- broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
- where B::Target: BroadcasterInterface,
- F::Target: FeeEstimator,
- L::Target: Logger,
+	pub(crate) fn update_claims_view_from_matched_txn<B: Deref, F: Deref, L: Deref>(
+ &mut self, txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash,
+ cur_height: u32, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+	) where
+ B::Target: BroadcasterInterface,
+ F::Target: FeeEstimator,
+ L::Target: Logger,
{
log_debug!(logger, "Updating claims view at height {} with {} matched transactions in block {}", cur_height, txn_matched.len(), conf_height);
diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs
index 6d4fe16f..1e26d339 100644
--- a/lightning/src/ln/reorg_tests.rs
+++ b/lightning/src/ln/reorg_tests.rs
@@ -285,7 +285,6 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
assert_eq!(relevant_txids.len(), 1);
let block_hash_opt = relevant_txids[0].1;
- assert!(block_hash_opt.is_some());
let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
- assert_eq!(block_hash_opt.unwrap(), expected_hash);
+ assert_eq!(block_hash_opt, Some(expected_hash));
let txid = relevant_txids[0].0;
assert_eq!(txid, chan.3.txid());
@@ -364,7 +363,6 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
assert_eq!(relevant_txids.len(), 1);
let block_hash_opt = relevant_txids[0].1;
- assert!(block_hash_opt.is_some());
let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
- assert_eq!(block_hash_opt.unwrap(), expected_hash);
+ assert_eq!(block_hash_opt, Some(expected_hash));
let txid = relevant_txids[0].0;
assert_eq!(txid, chan.3.txid());

[TheBlueMatt]

FWIW, in the future, the update_claims_view split could be in a separate commit. Not worth trying to split it now that its done, though.

[tnull]

FWIW, in the future, the update_claims_view split could be in a separate commit. Not worth trying to split it now that its done, though.

Alright, split it out again.

[TheBlueMatt]

Two doc nits, feel free to ignore unless/until you have more review comments from others.

[tnull]

Addressed nits and squashed again, CI should be happy now.

[jkczyz]

Addressed nits and squashed again, CI should be happy now.

Didn't we go with two commits earlier?

[TheBlueMatt]

Yea, we did split it before, but not sure its worth bothering to hold this up more for that - the total diff is only +129 −60 so its not like its big enough that its hard to read.

[tnull]

Previously, Confirm::get_relevant_txids() only returned a list of transactions that have to be monitored for reorganization out of the chain. This interface however required a double bookkeeping: while we internally keep track of the best block, height, etc, it would also require the user to keep track which transaction was previously confirmed in which block and to take actions based on any change, e.g, to reconfirm them when the block would be reorged-out and the transactions had been reconfirmed in another block.

Here, we track the confirmation block hash internally and return it via Confirm::get_relevant_txids() to the user, which alleviates the requirement for double bookkeeping: the user can now simply check whether the given transactions are still confirmed via the given blocks, and take action if they are not.

[tnull]

Marked draft for now, will switch after concept ACK.

[codecov-commenter]

Codecov Report

Base: 90.78% // Head: 91.23% // Increases project coverage by +0.44% 🎉

Coverage data is based on head (de9cd52) compared to base (e55e0d5).
Patch coverage: 89.39% of modified lines in pull request are covered.

❗ Current head de9cd52 differs from pull request most recent head 9685d6c. Consider uploading reports for the commit 9685d6c to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1796 +/- ##
==========================================
+ Coverage 90.78% 91.23% +0.44% 
==========================================
Files 87 89 +2 Lines 47604 50973 +3369 Branches 47604 50973 +3369 ==========================================
+ Hits 43218 46503 +3285 - Misses 4386 4470 +84 

Impacted Files	Coverage Δ
lightning/src/chain/chainmonitor.rs	97.78% <ø> (ø)
lightning/src/chain/mod.rs	68.18% <ø> (ø)
lightning/src/chain/channelmonitor.rs	90.69% <80.64%> (+0.04%)	⬆️
lightning/src/chain/onchaintx.rs	95.08% <90.00%> (+0.07%)	⬆️
lightning/src/ln/channel.rs	88.71% <100.00%> (+<0.01%)	⬆️
lightning/src/ln/channelmanager.rs	88.85% <100.00%> (+3.44%)	⬆️
lightning/src/ln/reorg_tests.rs	100.00% <100.00%> (ø)
lightning-net-tokio/src/lib.rs	76.73% <0.00%> (-0.31%)	⬇️
lightning/src/ln/functional_tests.rs	96.95% <0.00%> (-0.11%)	⬇️
lightning/src/lib.rs	100.00% <0.00%> (ø)
... and 12 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]

Marked draft for now, will switch after concept ACK.

Just discussed this with Matt in Discord, I'll count this as concept ACK.

[arik-so]

Would it be helpful to also return the confirmation height, so a user could trivially verify if a transaction has been dropped by knowing the depth of recent reorgs?

[tnull]

Would it be helpful to also return the confirmation height, so a user could trivially verify if a transaction has been dropped by knowing the depth of recent reorgs?

Not sure, as the height to compare it to is always due to change meaning it may be ambiguous what 'recent reorgs' are. So including height might invite users to do some comparisons that are likely gonna be race-y again?

[wpaulino]

LGTM, feel free to squash.

[TheBlueMatt]

This really needs some kind of testing. It should be pretty straightforward to just add relevant assertions in some of the existing reorg tests, I hope.

[tnull]

This really needs some kind of testing. It should be pretty straightforward to just add relevant assertions in some of the existing reorg tests, I hope.

Now added a test that we're indeed getting the expected confirmation block hash with c937807. Let me know if it's fine to squash.

[TheBlueMatt]

I know its not really your fault, but man this is confusing - update_claims_view takes block hash and height parameters for "what block/height this tx was included in" but is often called without any txn and dummy (current block) values. Can we split the function in two and have the part that operates on txn_matched be separate? Should be a straightforward change and would make this PR much more clearly correct.

[tnull]

Now split the method into update_claims_view_from_requests and update_claims_view_from_matched_txn in 0f9fc8a. I also DRYed the onchain event processing that would always run into process_onchain_events_awaiting_threshold_conf.

[TheBlueMatt]

Should drop the conf_height - nothing is being confirmed. Can also drop the corresponding docs which talk about txn_matched.

Can you fix the above docs on conf_height which mentions txn_matched?

[tnull]

Done hope I'm not misinterpreting something here:

$ git diff-tree -U2 0f9fc8a 84073d
diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index ee557eb7..8fbfa215 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -552,7 +552,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// preimage after force-close.
///
- /// `conf_height` represents the height at which the transactions in `txn_matched` were
- /// confirmed. This does not need to equal the current blockchain tip height, which should be
- /// provided via `cur_height`, however it must never be higher than `cur_height`.
+ /// `conf_height` represents the minimal height at which the request could get confirmed. This
+ /// does not need to equal the current blockchain tip height, which should be provided via
+ /// `cur_height`, however it must never be higher than `cur_height`.
pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(&mut self,
requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B,

[TheBlueMatt]

Basically LGTM, feel free to squash from my PoV, but will need re-review from multiple reviewers here.

[tnull]

Squashed commits.

[wpaulino]

LGTM

[jkczyz]

Just some nits.

[tnull]

Addressed nits, squashed again (and accidentally rebased on main in the process... Sorry 😢 )

[tnull]

Recovered the most recent base before the accident. So the latest squash diff is:

> git diff-tree -U2 1a9fec5 1a80eac
diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index aa1116ae..14b5a298 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -555,10 +555,11 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// does not need to equal the current blockchain tip height, which should be provided via
/// `cur_height`, however it must never be higher than `cur_height`.
-	pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(&mut self,
- requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B,
- fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
- where B::Target: BroadcasterInterface,
- F::Target: FeeEstimator,
- L::Target: Logger,
+	pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(
+ &mut self, requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32,
+ broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+	) where
+ B::Target: BroadcasterInterface,
+ F::Target: FeeEstimator,
+ L::Target: Logger,
{
log_debug!(logger, "Updating claims view at height {} with {} claim requests", cur_height, requests.len());
@@ -654,10 +655,11 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// confirmed. This does not need to equal the current blockchain tip height, which should be
/// provided via `cur_height`, however it must never be higher than `cur_height`.
-	pub(crate) fn update_claims_view_from_matched_txn<B: Deref, F: Deref, L: Deref>(&mut self,
- txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash, cur_height: u32,
- broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
- where B::Target: BroadcasterInterface,
- F::Target: FeeEstimator,
- L::Target: Logger,
+	pub(crate) fn update_claims_view_from_matched_txn<B: Deref, F: Deref, L: Deref>(
+ &mut self, txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash,
+ cur_height: u32, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+	) where
+ B::Target: BroadcasterInterface,
+ F::Target: FeeEstimator,
+ L::Target: Logger,
{
log_debug!(logger, "Updating claims view at height {} with {} matched transactions in block {}", cur_height, txn_matched.len(), conf_height);
diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs
index 6d4fe16f..1e26d339 100644
--- a/lightning/src/ln/reorg_tests.rs
+++ b/lightning/src/ln/reorg_tests.rs
@@ -285,7 +285,6 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
assert_eq!(relevant_txids.len(), 1);
let block_hash_opt = relevant_txids[0].1;
- assert!(block_hash_opt.is_some());
let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
- assert_eq!(block_hash_opt.unwrap(), expected_hash);
+ assert_eq!(block_hash_opt, Some(expected_hash));
let txid = relevant_txids[0].0;
assert_eq!(txid, chan.3.txid());
@@ -364,7 +363,6 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
assert_eq!(relevant_txids.len(), 1);
let block_hash_opt = relevant_txids[0].1;
- assert!(block_hash_opt.is_some());
let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
- assert_eq!(block_hash_opt.unwrap(), expected_hash);
+ assert_eq!(block_hash_opt, Some(expected_hash));
let txid = relevant_txids[0].0;
assert_eq!(txid, chan.3.txid());

[TheBlueMatt]

FWIW, in the future, the update_claims_view split could be in a separate commit. Not worth trying to split it now that its done, though.

[tnull]

FWIW, in the future, the update_claims_view split could be in a separate commit. Not worth trying to split it now that its done, though.

Alright, split it out again.

[TheBlueMatt]

Two doc nits, feel free to ignore unless/until you have more review comments from others.

[tnull]

Addressed nits and squashed again, CI should be happy now.

[jkczyz]

Addressed nits and squashed again, CI should be happy now.

Didn't we go with two commits earlier?

[TheBlueMatt]

Yea, we did split it before, but not sure its worth bothering to hold this up more for that - the total diff is only +129 −60 so its not like its big enough that its hard to read.

[tnull]

Previously, Confirm::get_relevant_txids() only returned a list of transactions that have to be monitored for reorganization out of the chain. This interface however required a double bookkeeping: while we internally keep track of the best block, height, etc, it would also require the user to keep track which transaction was previously confirmed in which block and to take actions based on any change, e.g, to reconfirm them when the block would be reorged-out and the transactions had been reconfirmed in another block.

Here, we track the confirmation block hash internally and return it via Confirm::get_relevant_txids() to the user, which alleviates the requirement for double bookkeeping: the user can now simply check whether the given transactions are still confirmed via the given blocks, and take action if they are not.

[tnull]

Marked draft for now, will switch after concept ACK.

[codecov-commenter]

Codecov Report

Base: 90.78% // Head: 91.23% // Increases project coverage by +0.44% 🎉

Coverage data is based on head (de9cd52) compared to base (e55e0d5).
Patch coverage: 89.39% of modified lines in pull request are covered.

❗ Current head de9cd52 differs from pull request most recent head 9685d6c. Consider uploading reports for the commit 9685d6c to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1796 +/- ##
==========================================
+ Coverage 90.78% 91.23% +0.44% 
==========================================
Files 87 89 +2 Lines 47604 50973 +3369 Branches 47604 50973 +3369 ==========================================
+ Hits 43218 46503 +3285 - Misses 4386 4470 +84 

Impacted Files	Coverage Δ
lightning/src/chain/chainmonitor.rs	97.78% <ø> (ø)
lightning/src/chain/mod.rs	68.18% <ø> (ø)
lightning/src/chain/channelmonitor.rs	90.69% <80.64%> (+0.04%)	⬆️
lightning/src/chain/onchaintx.rs	95.08% <90.00%> (+0.07%)	⬆️
lightning/src/ln/channel.rs	88.71% <100.00%> (+<0.01%)	⬆️
lightning/src/ln/channelmanager.rs	88.85% <100.00%> (+3.44%)	⬆️
lightning/src/ln/reorg_tests.rs	100.00% <100.00%> (ø)
lightning-net-tokio/src/lib.rs	76.73% <0.00%> (-0.31%)	⬇️
lightning/src/ln/functional_tests.rs	96.95% <0.00%> (-0.11%)	⬇️
lightning/src/lib.rs	100.00% <0.00%> (ø)
... and 12 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]

Marked draft for now, will switch after concept ACK.

Just discussed this with Matt in Discord, I'll count this as concept ACK.

[arik-so]

Would it be helpful to also return the confirmation height, so a user could trivially verify if a transaction has been dropped by knowing the depth of recent reorgs?

[tnull]

Would it be helpful to also return the confirmation height, so a user could trivially verify if a transaction has been dropped by knowing the depth of recent reorgs?

Not sure, as the height to compare it to is always due to change meaning it may be ambiguous what 'recent reorgs' are. So including height might invite users to do some comparisons that are likely gonna be race-y again?

[wpaulino]

LGTM, feel free to squash.

[TheBlueMatt]

This really needs some kind of testing. It should be pretty straightforward to just add relevant assertions in some of the existing reorg tests, I hope.

[tnull]

This really needs some kind of testing. It should be pretty straightforward to just add relevant assertions in some of the existing reorg tests, I hope.

Now added a test that we're indeed getting the expected confirmation block hash with c937807. Let me know if it's fine to squash.

[TheBlueMatt]

I know its not really your fault, but man this is confusing - update_claims_view takes block hash and height parameters for "what block/height this tx was included in" but is often called without any txn and dummy (current block) values. Can we split the function in two and have the part that operates on txn_matched be separate? Should be a straightforward change and would make this PR much more clearly correct.

[tnull]

Now split the method into update_claims_view_from_requests and update_claims_view_from_matched_txn in 0f9fc8a. I also DRYed the onchain event processing that would always run into process_onchain_events_awaiting_threshold_conf.

[TheBlueMatt]

Should drop the conf_height - nothing is being confirmed. Can also drop the corresponding docs which talk about txn_matched.

Can you fix the above docs on conf_height which mentions txn_matched?

[tnull]

Done hope I'm not misinterpreting something here:

$ git diff-tree -U2 0f9fc8a 84073d
diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index ee557eb7..8fbfa215 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -552,7 +552,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// preimage after force-close.
///
- /// `conf_height` represents the height at which the transactions in `txn_matched` were
- /// confirmed. This does not need to equal the current blockchain tip height, which should be
- /// provided via `cur_height`, however it must never be higher than `cur_height`.
+ /// `conf_height` represents the minimal height at which the request could get confirmed. This
+ /// does not need to equal the current blockchain tip height, which should be provided via
+ /// `cur_height`, however it must never be higher than `cur_height`.
pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(&mut self,
requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B,

[TheBlueMatt]

Basically LGTM, feel free to squash from my PoV, but will need re-review from multiple reviewers here.

[tnull]

Squashed commits.

[wpaulino]

LGTM

[jkczyz]

Just some nits.

[tnull]

Addressed nits, squashed again (and accidentally rebased on main in the process... Sorry 😢 )

[tnull]

Recovered the most recent base before the accident. So the latest squash diff is:

> git diff-tree -U2 1a9fec5 1a80eac
diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs
index aa1116ae..14b5a298 100644
--- a/lightning/src/chain/onchaintx.rs
+++ b/lightning/src/chain/onchaintx.rs
@@ -555,10 +555,11 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// does not need to equal the current blockchain tip height, which should be provided via
/// `cur_height`, however it must never be higher than `cur_height`.
-	pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(&mut self,
- requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B,
- fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
- where B::Target: BroadcasterInterface,
- F::Target: FeeEstimator,
- L::Target: Logger,
+	pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(
+ &mut self, requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32,
+ broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+	) where
+ B::Target: BroadcasterInterface,
+ F::Target: FeeEstimator,
+ L::Target: Logger,
{
log_debug!(logger, "Updating claims view at height {} with {} claim requests", cur_height, requests.len());
@@ -654,10 +655,11 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// confirmed. This does not need to equal the current blockchain tip height, which should be
/// provided via `cur_height`, however it must never be higher than `cur_height`.
-	pub(crate) fn update_claims_view_from_matched_txn<B: Deref, F: Deref, L: Deref>(&mut self,
- txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash, cur_height: u32,
- broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
- where B::Target: BroadcasterInterface,
- F::Target: FeeEstimator,
- L::Target: Logger,
+	pub(crate) fn update_claims_view_from_matched_txn<B: Deref, F: Deref, L: Deref>(
+ &mut self, txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash,
+ cur_height: u32, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
+	) where
+ B::Target: BroadcasterInterface,
+ F::Target: FeeEstimator,
+ L::Target: Logger,
{
log_debug!(logger, "Updating claims view at height {} with {} matched transactions in block {}", cur_height, txn_matched.len(), conf_height);
diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs
index 6d4fe16f..1e26d339 100644
--- a/lightning/src/ln/reorg_tests.rs
+++ b/lightning/src/ln/reorg_tests.rs
@@ -285,7 +285,6 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
assert_eq!(relevant_txids.len(), 1);
let block_hash_opt = relevant_txids[0].1;
- assert!(block_hash_opt.is_some());
let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
- assert_eq!(block_hash_opt.unwrap(), expected_hash);
+ assert_eq!(block_hash_opt, Some(expected_hash));
let txid = relevant_txids[0].0;
assert_eq!(txid, chan.3.txid());
@@ -364,7 +363,6 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
assert_eq!(relevant_txids.len(), 1);
let block_hash_opt = relevant_txids[0].1;
- assert!(block_hash_opt.is_some());
let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
- assert_eq!(block_hash_opt.unwrap(), expected_hash);
+ assert_eq!(block_hash_opt, Some(expected_hash));
let txid = relevant_txids[0].0;
assert_eq!(txid, chan.3.txid());

[TheBlueMatt]

FWIW, in the future, the update_claims_view split could be in a separate commit. Not worth trying to split it now that its done, though.

[tnull]

FWIW, in the future, the update_claims_view split could be in a separate commit. Not worth trying to split it now that its done, though.

Alright, split it out again.

[TheBlueMatt]

Two doc nits, feel free to ignore unless/until you have more review comments from others.

[tnull]

Addressed nits and squashed again, CI should be happy now.

[jkczyz]

Addressed nits and squashed again, CI should be happy now.

Didn't we go with two commits earlier?

[TheBlueMatt]

Yea, we did split it before, but not sure its worth bothering to hold this up more for that - the total diff is only +129 −60 so its not like its big enough that its hard to read.