-
Notifications
You must be signed in to change notification settings - Fork 295
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(esplora): use saturating_add in update_tx_graph() #1110
fix(esplora): use saturating_add in update_tx_graph() #1110
Conversation
I'm confused. How can this possibly overflow in practice? |
I ran into this when I was testing sync with esplora but only checking unused spks, based on how the cli example works. I could be misunderstanding how that's supposed to be done, but I thought I could just give the esplora client my spks via |
b156772
to
c161a21
Compare
I was also able to repro this issue (using the original code) using the
|
crates/esplora/tests/blocking_ext.rs
Outdated
1, | ||
)?; | ||
|
||
assert_eq!(graph_update.full_txs().count(), 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 test that the txids match those returned from send_to_address
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There should be a way to consolidate blocking and async tests and TestEnv (probably with macros as Alekos originally did), I'll create an issue.
crates/esplora/Cargo.toml
Outdated
# This feature is used to run `cargo check` in our CI targeting wasm. It's not recommended | ||
# for libraries to explicitly include the "getrandom/js" feature, so we only do it when | ||
# necessary for running our CI. See: https://docs.rs/getrandom/0.2.8/getrandom/#webassembly-support | ||
dev-getrandom-wasm = ["getrandom/js"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this. Where do we need to use getrandom
at all here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like getrandom
is brought in via the electrsd
dev-dependency I added. One improvement I can make is to only include getrandom
as a dev-dependency since that's the only place it's needed.
# from esplora directory
$ cargo tree -i getrandom
getrandom v0.2.10
└── rand_core v0.6.4
├── rand v0.8.5
│ └── secp256k1 v0.27.0
│ └── bitcoin v0.30.1
│ ├── bdk_chain v0.5.0 (/Users/steve/git/notmandatory/bdk/crates/chain)
│ │ └── bdk_esplora v0.3.0 (/Users/steve/git/notmandatory/bdk/crates/esplora)
│ ├── bitcoincore-rpc-json v0.17.0
│ │ └── bitcoincore-rpc v0.17.0
│ │ └── bitcoind v0.33.1
│ │ └── electrsd v0.25.0
│ │ [dev-dependencies]
│ │ └── bdk_esplora v0.3.0 (/Users/steve/git/notmandatory/bdk/crates/esplora)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you don't need it at all since electrsd
doesn't run in WASM anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason cargo check
with the WASM target is still including getrandom
and throwing the error. I'll see if I can find some way to force it not to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah because it's compiling dev depends in cargo check right? so it's gonna try and compile electrsd for WASM. So you need to #[cfg(not(target_arch = "wasm32"))]
and not depend on the thing in the Cargo.toml if the arch is WASM.
Tbh I'm not sure why secp256k1 depends on rand
maybe we can disable a feature that prevents including rand? That would be much simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured out the issue. Using [target.'cfg(not(target_arch = "wasm32"))'.dev-dependencies]
in the crates/esplora/Cargo.toml
didn't work until I enabled resolver = "2"
in the top level workspace Cargo.toml
. I would have expected it to already be using the new resolver since all our crates are set to edition = "2021"
, but apparently the resolver also needs to be set for the whole workspace.
https://blog.rust-lang.org/2021/03/25/Rust-1.51.0.html#cargos-new-feature-resolver
4d3e367
to
26f90a6
Compare
47d09e6
to
a42fb3d
Compare
Rebased on master. |
a42fb3d
to
7be6f86
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @notmandatory. Just to be clear I fully understand why it fails. I missed that u32::MAX
was how we were disabling it.
ACK 7be6f86
@@ -38,7 +38,9 @@ jobs: | |||
cargo update -p flate2:1.0.27 --precise "1.0.26" | |||
cargo update -p reqwest --precise "0.11.18" | |||
cargo update -p h2 --precise "0.3.20" | |||
cargo update -p rustls-webpki --precise "0.100.1" | |||
cargo update -p rustls-webpki:0.100.3 --precise "0.100.1" | |||
cargo update -p rustls-webpki:0.101.5 --precise "0.101.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we depend on two slightly different versions of webpki?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like rustls-webpki
0.100.x is required by the rustls
version used by electrum-client
.
And rustls-webpki
0.101.x is required directly by the version of minreq
used by our dev-dependencies bitcoind
and electrsd
.
I think the ultimate solution for this will be to get all our dependencies to use the same major+minor version of rustls
.
This fixes overflow error when calling update_tx_graph() from update_tx_graph_without_keychain().
…rsion 2 The resolver version must be set at the workspace level. See: https://blog.rust-lang.org/2021/03/25/Rust-1.51.0.html#cargos-new-feature-resolver
7be6f86
to
bf9a425
Compare
Rebased and fixed two more ci MSRV issues. Had to pin |
FYI, after I merged this PR the audit job failed due to the new dev dependency on the |
Description
This fixes overflow error when calling update_tx_graph() from update_tx_graph_without_keychain().
Notes to the reviewers
You can reproduce the error by reverting 66a2bf5.
The tests could use some cleanup but get the job done for this PR.
Changelog notice
None
Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingBugfixes: