Repository navigation
fix(chain): return false for untracked indices in SpkTxOutIndex::is_used - #2445
bhuvan-somisetty wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2445 +/- ##
==========================================
+ Coverage 83.25% 83.30% +0.04%
==========================================
Files 31 31
Lines 6236 6236
Branches 293 294 +1
==========================================
+ Hits 5192 5195 +3
+ Misses 976 973 -3
Partials 68 68
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
yahia008
left a comment
There was a problem hiding this comment.
tACK aa79e14
I'm fairly new to this codebase, so take this as a tested-locally comment and not a
maintainer review.
I checked out the PR and ran the new tests; both pass, along with the existing
test_keychain_txout_index suite. The new check matches the guard already in unmark_used,
and the only caller of SpkTxOutIndex::is_used I found in the repo is
KeychainTxOutIndex::is_used, which forwards to it.
A few small things, not blocking:
- The MSRV job fails during dependency resolution on 1.85, and that doesn't look related
to this change. Master has updated the MSRV script since this branch was cut, so would a
rebase help? - The
KeychainTxOutIndex::is_useddoc could say "an unrevealed index" instead of "the spk
underkeychainandindex". - Since unknown indices now return
falsewhere they used to returntrue, would
"Changed" fit the changelog better than "Fixed"?
aa79e14 to
d1821e1
Compare
|
Thanks! Rebased on master, which should fix MSRV, updated the doc wording, and switched the changelog to "Changed". |
Previously, `SpkTxOutIndex::is_used` checked `!self.unused.contains(index)`. Because `self.unused` only tracks indices that were explicitly added to the index, any untracked index was absent from `self.unused`, causing `is_used` to evaluate to `true`. Check `self.spks.contains_key(index)` in `is_used` to ensure untracked indices return `false`. Also update documentation and add unit tests for `SpkTxOutIndex` and `KeychainTxOutIndex`. Closes bitcoindevkit#2444 Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
d1821e1 to
1cf692d
Compare
Description
Fixes an issue where
SpkTxOutIndex::is_usedreturnstruefor an index that was never tracked or added to the index (e.g. on an empty index or querying an unrevealed keychain derivation index inKeychainTxOutIndex::is_used).is_usedpreviously checked!self.unused.contains(index). Becauseself.unusedonly tracks indices that were explicitly added to the index, any index that has never been tracked is absent fromself.unused, causingis_usedto evaluate totrue.This PR updates
SpkTxOutIndex::is_usedto verifyself.spks.contains_key(index) && !self.unused.contains(index), matching the guard already present inunmark_used. It also adds documentation and unit tests for bothSpkTxOutIndexandKeychainTxOutIndex.Closes #2444.
Notes to the reviewers
SpkTxOutIndex::unmark_usedalready explicitly checksif !self.spks.contains_key(index) { return false; }. Applyingself.spks.contains_key(index)tois_usedbrings full symmetry between both methods and preserves the invariant that untracked indices are not considered used.test_spk_txout_index.rsandtest_keychain_txout_index.rs.Changelog notice
SpkTxOutIndex::is_usedandKeychainTxOutIndex::is_usedto returnfalsefor untracked or unrevealed indices instead oftrue.Checklists
All Submissions:
Bugfixes: