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

test(electrum): Test sync in reorg and no-reorg situations #1535

Merged
merged 2 commits into from
Aug 12, 2024

Conversation

LagginTimes
Copy link
Contributor

@LagginTimes LagginTimes commented Aug 3, 2024

Closes #1125.

Description

Add new test for bdk_electrum to make sure previously unconfirmed transactions get confirmed again in both reorg and no-reorg situations.

Changelog notice

  • Added wait_until_electrum_sees_txid method to TestEnv.
  • wait_until_electrum_sees_block now has a Duration input for timeout.
  • Removed exponential polling delay in wait_until_electrum_sees_block.
  • Added test_sync to bdk_electrum to make sure previously unconfirmed transactions get confirmed in both reorg and no-org situations.

Checklists

All Submissions:

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

@LagginTimes LagginTimes force-pushed the electrum_sync_test branch 2 times, most recently from a15f6f3 to 01f5a0f Compare August 3, 2024 15:20
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Good work with this. Moving forward, I recommend improving the docs and adding more clarity to the testing logic. I also included a bunch of nits for you to consider including.

crates/electrum/tests/test_electrum.rs Outdated Show resolved Hide resolved
crates/electrum/tests/test_electrum.rs Outdated Show resolved Hide resolved
crates/electrum/tests/test_electrum.rs Outdated Show resolved Hide resolved
crates/electrum/tests/test_electrum.rs Outdated Show resolved Hide resolved
crates/electrum/tests/test_electrum.rs Outdated Show resolved Hide resolved
crates/electrum/tests/test_electrum.rs Outdated Show resolved Hide resolved
crates/electrum/tests/test_electrum.rs Outdated Show resolved Hide resolved
crates/electrum/tests/test_electrum.rs Outdated Show resolved Hide resolved
crates/electrum/tests/test_electrum.rs Outdated Show resolved Hide resolved
crates/electrum/tests/test_electrum.rs Show resolved Hide resolved
@LagginTimes LagginTimes force-pushed the electrum_sync_test branch 4 times, most recently from a47b802 to c080123 Compare August 7, 2024 09:18
@evanlinjin
Copy link
Member

The commit also adds a method to the TestEnv (which is not documented). Please either have this change as a separate commit, or mention the change in the commit.

crates/testenv/src/lib.rs Outdated Show resolved Hide resolved
crates/testenv/src/lib.rs Outdated Show resolved Hide resolved
@LagginTimes LagginTimes force-pushed the electrum_sync_test branch 4 times, most recently from f274e4c to ff3f5da Compare August 8, 2024 14:19
@notmandatory notmandatory added this to the 1.0.0-beta milestone Aug 8, 2024
@LagginTimes LagginTimes force-pushed the electrum_sync_test branch 2 times, most recently from 360a78c to 73e34ec Compare August 9, 2024 07:18
Add test for `bdk_electrum` to make sure previously unconfirmed
transactions get confirmed again in both reorg and no-reorg
situations.
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

LGTM. Once these are addressed and changelog is added to PR description, I am happy to merge.

crates/electrum/tests/test_electrum.rs Outdated Show resolved Hide resolved
crates/electrum/tests/test_electrum.rs Show resolved Hide resolved
Added `wait_until_electrum_sees_txid` method to `TestEnv`. Both
`bdk_electrum` wait methods now have a `timeout` option. Removed
the exponential polling delay in lieu of a fixed delay inside the
`bdk_electrum` wait methods.
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

ACK 2c0bc45

@evanlinjin evanlinjin merged commit caef3f4 into bitcoindevkit:master Aug 12, 2024
12 checks passed

assert_eq!(
get_balance(&recv_chain, &recv_graph)?,
Balance {
trusted_pending: SEND_AMOUNT * depth as u64,
Copy link
Contributor

@ValuedMammal ValuedMammal Aug 13, 2024

Choose a reason for hiding this comment

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

I agree this line should be here. It made me wonder why after #1416 the assertion failed, but I think the difference here is that we're updating last-seen during sync_with_electrum as we should.

see related #1550

@notmandatory notmandatory mentioned this pull request Aug 25, 2024
31 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
4 participants