Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix KeychainTxOutIndex::lookahead_to_target #1349

Merged

Conversation

evanlinjin
Copy link
Member

@evanlinjin evanlinjin commented Feb 16, 2024

Description

This method was not used (so it was untested) and it was not working. This fixes it.

The old implementation used .next_store_index which returned the keychain's last index stored in .inner (which include lookahead spks). This is WRONG because .replenish_lookahead needs the difference from last revealed.

Changelog notice

Fix KeychainTxOutIndex::lookahead_to_target

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

Bugfixes:

  • I've added tests to reproduce the issue which are now passing

@evanlinjin evanlinjin self-assigned this Feb 16, 2024
@evanlinjin evanlinjin added the bug Something isn't working label Feb 16, 2024
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but going through replenish_lookahead I realized it could just use self.next_index instead of let next_reveal_index = self.last_revealed.get(&descriptor_id).map_or(0, |v| *v + 1);. Can you fix? Or I can open an issue if you prefer!

crates/chain/tests/test_keychain_txout_index.rs Outdated Show resolved Hide resolved
crates/chain/tests/test_keychain_txout_index.rs Outdated Show resolved Hide resolved
crates/chain/tests/test_keychain_txout_index.rs Outdated Show resolved Hide resolved
@evanlinjin
Copy link
Member Author

Unrelated, but going through replenish_lookahead I realized it could just use self.next_index instead of let next_reveal_index = self.last_revealed.get(&descriptor_id).map_or(0, |v| *v + 1);. Can you fix?

I don't mind adding to this PR (as it is small). However, I'm not sure what we are trying to fix here?

@LLFourn
Copy link
Contributor

LLFourn commented Feb 19, 2024

Did we consider removing this method? Do we believe there is still a good reason to do lookaheads outside of the lookahead setting?

@evanlinjin
Copy link
Member Author

Did we consider removing this method? Do we believe there is still a good reason to do lookaheads outside of the lookahead setting?

I'm currently using it for something 😅 (that's how I found the bug). To be fair, what I'm doing is pretty niche but I don't think it hurts to keep it?

@nondiremanuel nondiremanuel added this to the 1.0.0-alpha milestone Feb 27, 2024
@evanlinjin
Copy link
Member Author

This is ready for review!

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b290b29

The test framework was more complicated to review than the change! but good coverage.

@notmandatory notmandatory merged commit c3544c9 into bitcoindevkit:master Mar 2, 2024
12 checks passed
@notmandatory notmandatory mentioned this pull request Mar 2, 2024
23 tasks
@notmandatory notmandatory mentioned this pull request Mar 26, 2024
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants