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.

Sign up for GitHub

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

Jump to bottom

fix electrum conftime_req filter for needs_block_height #1782

Merged
merged 9 commits into from
Jan 8, 2025
Merged

fix electrum conftime_req filter for needs_block_height #1782

merged 9 commits into from
Jan 8, 2025

Conversation

zoedberg
Copy link

Description

This PR fixes a bug introduced in 04994e4#diff-57bf66f87897e694c5ebfdfe0fd366774dda30b43eab93c1a0fdc802d0eb8c8dR171

In that commit block_times.get(height).is_none() was converted to block_times.contains_key(height), inverting the code logic.

Checklists

All Submissions:

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

@ValuedMammal
Copy link
Contributor

LGTM

oleonardolima
oleonardolima reviewed Dec 18, 2024
View reviewed changes
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

utACK 8d190ba

@ValuedMammal ValuedMammal added the bug Something isn't working label Dec 23, 2024
@notmandatory notmandatory force-pushed the fix_electrum_conftime_req_filter branch from f1dac61 to bbd0658 Compare January 6, 2025 17:00
@notmandatory notmandatory force-pushed the fix_electrum_conftime_req_filter branch 2 times, most recently from 4537c51 to 81a1433 Compare January 6, 2025 17:41
@notmandatory notmandatory force-pushed the fix_electrum_conftime_req_filter branch 4 times, most recently from 6d04fe5 to 3ad96b5 Compare January 6, 2025 18:45
@notmandatory notmandatory force-pushed the fix_electrum_conftime_req_filter branch from 5b4f186 to 20100aa Compare January 6, 2025 20:20
@notmandatory
Copy link
Member

notmandatory commented Jan 6, 2025 •
edited
Loading

I had to make some small fixes to get CI to complete, see 4aa8fba...20100aa.

notmandatory
notmandatory approved these changes Jan 6, 2025
View reviewed changes
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 20100aa

@zoedberg thank you for catching and fixing this! How did you find it?

@zoedberg
Copy link
Author

zoedberg commented Jan 7, 2025

thank you for catching and fixing this! How did you find it?

I've just updated rgb-lib to bdk 0.30.0 and almost all its tests started failing. Precisely the call wallet.sync(bdk_blockchain, SyncOptions { progress: None }) was returning a Generic("electrum server misbehaving") error. At that point I've inspected bdk's code, checked the points where the error could have been raised, then checked the blame to identify the latest commit that modified those lines and finally inspecting that commit I've found the incorrect change (04994e4#diff-57bf66f87897e694c5ebfdfe0fd366774dda30b43eab93c1a0fdc802d0eb8c8dR171).

oleonardolima
oleonardolima reviewed Jan 7, 2025
View reviewed changes
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

utACK 20100aa

@notmandatory any specific reason to downgrade the runners to ubuntu-20.04?

nymius
nymius reviewed Jan 7, 2025
View reviewed changes
Copy link
Contributor

@nymius nymius left a comment

Choose a reason for hiding this comment

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

ACK 20100aa

@notmandatory any specific reason to downgrade the runners to ubuntu-20.04?

Curious about this too

@notmandatory notmandatory mentioned this pull request Jan 8, 2025
Merged
3 tasks
@notmandatory
Copy link
Member

@oleonardolima and @nymius I had to downgrade the runner to the ubuntu-20.04 for two issues:

  1. the lastest version of python breaks the old pyo3 needed for hwi
  2. something with the latest c compiler breaks the old rocksdb used in the old cbf impl

I don't see any reason to fix these two things to work with the latest ubuntu since they're rarely used.

@notmandatory notmandatory merged commit 68c78e4 into bitcoindevkit:release/0.29 Jan 8, 2025
39 checks passed
notmandatory added a commit that referenced this pull request Jan 8, 2025
@notmandatory
Merge #1791: Update bdk 0.30.x README docs to deprecate the bdk library
9df9827 
6d2e243 release: bump version to 0.30.1 (Steve Myers)
e52c911 docs: update README to deprecate the bdk lib (Steve Myers)

Pull request description:

  ### Description

  This is needed to direct new and existing users to upgrade to the new `bdk_wallet`1.0 library.

  I also used this PR to bump the version and changelog to v0.30.1 so we're ready to publish the fix in #1782.

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

ACKs for top commit:
  thunderbiscuit:
    ACK 6d2e243.

Tree-SHA512: b5d5eb8ae2df719e18430a7629970f0ec06e023d07a7ff1ea03bb7fb43b5bcb27d5975473cc2c1a310afe16b64263551ce2d25c2a44a0302cfdacd9a41f597d7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nymius nymius nymius left review comments

@oleonardolima oleonardolima oleonardolima left review comments

@notmandatory notmandatory notmandatory approved these changes

Assignees

@zoedberg zoedberg

Labels
bug Something isn't working
Projects
BDK Maintenance
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zoedberg @ValuedMammal @notmandatory @oleonardolima @nymius