fix: Set WatchedOutput::block_hash to Some
#4016
Conversation
|
👋 Thanks for assigning @wpaulino as a reviewer! |
Prabhat1308
force-pushed
the
pv/update_WatchedOutput
branch
from
24667d6 to
a2bd207
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4016 +/- ##
==========================================
- Coverage 88.85% 88.68% -0.17%
==========================================
Files 175 174 -1
Lines 127710 127633 -77
Branches 127710 127633 -77
==========================================
- Hits 113478 113197 -281
- Misses 11675 11957 +282
+ Partials 2557 2479 -78
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hmm, note that the original comment linked on #3915 was regarding ChainMonitor, not the places you're changing here. I also don't think we can just always plug-in the current block hash, but (depending on the context) rather need to track the earliest block hash where the respective output spent could have happened.
| use std::ops::Deref; | ||
|
|
||
| // Custom test filter to capture full WatchedOutput instances | ||
| struct TestFilter { |
There was a problem hiding this comment.
Rather than creating a new test util, you should be able to reuse crate::util::test_utils::TestChainSource.
| #[test] | ||
|
|
||
| fn test_watched_output_block_hash_persistence_after_restart() { | ||
| use crate::chain::WatchedOutput; |
There was a problem hiding this comment.
Please move any imports to the module-level.
| @@ -3298,3 +3298,80 @@ fn test_claim_event_never_handled() { | |||
| // `ChannelMonitorUpdate`. | |||
| check_added_monitors(&nodes[1], 2); | |||
| } | |||
|
|
|||
| #[test] | |||
|
|
|||
| @@ -1212,7 +1213,8 @@ where | |||
| ); | |||
| chain_source.register_tx(&funding_outpoint.txid, &funding_script); | |||
| chain_source.register_output(WatchedOutput { | |||
| block_hash: None, | |||
| block_hash: Some(current_block_hash), | |||
| // block_hash: None, | |||
There was a problem hiding this comment.
Please remove the commented-out lines here and below.
| mine_transactions(&nodes[1], &[]); | ||
| } | ||
|
|
||
| let expected_block_hash = nodes[1].best_block_hash(); |
There was a problem hiding this comment.
The expected block hash should be earliest hash the given output could potentially appear in. That means it would need to be the earliest block hash here i.e., the hash even before mining any additional blocks, no?
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
| @@ -2002,7 +2003,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> { | |||
| let outpoint = OutPoint { txid: *txid, index: *index as u16 }; | |||
| log_trace!(logger, "Registering outpoint {} with the filter to monitor spend", outpoint); | |||
| filter.register_output(WatchedOutput { | |||
| block_hash: None, | |||
| block_hash: Some(current_block_hash), | |||
There was a problem hiding this comment.
Since we're interested in the spend of an output, ideally this block hash is for the block the output was created in, or if it has not been created yet, the current block hash. This means we'll need to store it so we can reuse it here when this gets called on startup.
fixes #3915
adds test for #2119