Refactor TestEnv::wait_until_electrum_sees_block to be more concrete.
#1640
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
evanlinjin
force-pushed
the
fix/wait_until_electrum_sees_block
branch
from
85b35be
to
4684a43
Compare
Add inputs `block_height` and `block_hash` so that it is more concrete what exactly we are waiting for. Introduce `TestEnv::wait_until_electrum_tip_syncs_with_bitcoind`.
evanlinjin
force-pushed
the
fix/wait_until_electrum_sees_block
branch
from
4684a43
to
ce565ac
Compare
evanlinjin
commented
Oct 4, 2024
Comment on lines
+198
to
+204
| // NOTE: There is a reason why we use the subscribe endpoint specifically. You may think | ||
| // just polling Electrs via Electrum for a block header at height `block_height` and | ||
| // checking whether `block_hash` matches is enough. However, having the header polling call | ||
| // up to date does not necessarily mean the spk histories are up to date. On the other hand, | ||
| // getting a notification for a new block tip does mean that the confirmed spk histories | ||
| // are up to date up to and including the new notified tip. This is all due to the internal | ||
| // workings of Electrs. |
There was a problem hiding this comment.
I may need to word this better.
There was a problem hiding this comment.
How about:
// NOTE: We use the subscribe endpoint because polling Electrs for a block header at
// `block_height` and verifying the `block_hash` does not ensure that the spk histories
// are current. In contrast, getting a notification for a new block tip ensures that the
// confirmed spk histories are current, including the new notified tip. This is a result of
// the internal workings of Electrs.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Before, the signature of
TestEnv::wait_until_electrum_sees_blockonly took in adelay: Durationinput. It was not clear which block we were waiting for exactly. This PR adds 2 inputs to this method: ablock_heighttarget, and an optionalblock_hashmethod. Just having theblock_heightmethod waits until Electrs sees a block at that height, and all spk histories are up to date to that block. If the caller also includes ablock_hash, we wait until Electrs sees a block at the given height, also has that block hash.We also introduce a new method:
TestEnv::wait_until_electrum_tip_syncs_with_bitcoind. The method name is self-explanatory.Notes to the reviewers
It's a breaking change for
bdk_testenv. The API now makes more sense.Changelog notice
TestEnv::wait_until_electrum_sees_blockto have a height target, and an optional block hash target.TestEnv::wait_until_electrum_tip_syncs_with_bitcoind.Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features: