Fix: Wallet sync may decrement address index by evanlinjin · Pull Request #653 · bitcoindevkit/bdk

evanlinjin · 2022-07-03T06:43:46Z

Description

It is critical to ensure Wallet::get_address with AddressIndex::new always returns a new and unused address.

This bug seems to be Electrum-specific. The fix is to check address index updates to ensure that newly suggested indexes are not smaller than indexes already in database.

Notes to the reviewers

I have written new tests in /testutils/blockchain_tests.rs that tests all Blockchain implementations.

Checklists

All Submissions:

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

Bugfixes:

* [ ] This pull request breaks the existing API

I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

rajarshimaitra

Concept ACK..

I verified that the test is only failing for Electrum, but not for RPC or Esplora.. Maybe worth exploring further..

I have a code suggestion for the same checking logic.. It seems we can get the check done in much sorter codes.

rajarshimaitra

tACK ac5f0af

Thanks for the quick update..

@rajarshimaitra

This bug seems to be Electrum-specific. The fix is to check the proposed changes against the current state of the database. Ensure newly suggested indexes are not smaller than indexes already in database. Changes: * Check index updates before they are applied to database during Electrum Blockchain sync (Thank you @rajarshimaitra for providing an elegant solution). Tests added: * bdk_blockchain_tests!::test_sync_address_index_should_not_decrement * bdk_blockchain_tests!::test_sync_address_index_should_increment These tests ensure there will be no unexpected address reuse when grabbing a new address via `Wallet::get_address` with `AddressIndex::New`. Other changes: * Tweak `rpc.rs` so that clippy is happy.

afilini · 2022-07-11T10:02:11Z

ACK af6bde3

I'll wait for the CI to finish and then I'll merge this.

evanlinjin · 2022-07-03T06:43:46Z

Description

Fixes#649

It is critical to ensure Wallet::get_address with AddressIndex::new always returns a new and unused address.

This bug seems to be Electrum-specific. The fix is to check address index updates to ensure that newly suggested indexes are not smaller than indexes already in database.

Notes to the reviewers

I have written new tests in /testutils/blockchain_tests.rs that tests all Blockchain implementations.

Checklists

All Submissions:

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

Bugfixes:

* [ ] This pull request breaks the existing API

I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

rajarshimaitra

Concept ACK..

I verified that the test is only failing for Electrum, but not for RPC or Esplora.. Maybe worth exploring further..

I have a code suggestion for the same checking logic.. It seems we can get the check done in much sorter codes.

rajarshimaitra

tACK ac5f0af

Thanks for the quick update..

@rajarshimaitra

This bug seems to be Electrum-specific. The fix is to check the proposed changes against the current state of the database. Ensure newly suggested indexes are not smaller than indexes already in database. Changes: * Check index updates before they are applied to database during Electrum Blockchain sync (Thank you @rajarshimaitra for providing an elegant solution). Tests added: * bdk_blockchain_tests!::test_sync_address_index_should_not_decrement * bdk_blockchain_tests!::test_sync_address_index_should_increment These tests ensure there will be no unexpected address reuse when grabbing a new address via `Wallet::get_address` with `AddressIndex::New`. Other changes: * Tweak `rpc.rs` so that clippy is happy.

afilini · 2022-07-11T10:02:11Z

ACK af6bde3

I'll wait for the CI to finish and then I'll merge this.

evanlinjin · 2022-07-03T06:43:46Z

Description

Fixes#649

It is critical to ensure Wallet::get_address with AddressIndex::new always returns a new and unused address.

This bug seems to be Electrum-specific. The fix is to check address index updates to ensure that newly suggested indexes are not smaller than indexes already in database.

Notes to the reviewers

I have written new tests in /testutils/blockchain_tests.rs that tests all Blockchain implementations.

Checklists

All Submissions:

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

Bugfixes:

* [ ] This pull request breaks the existing API

I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

rajarshimaitra

Concept ACK..

I verified that the test is only failing for Electrum, but not for RPC or Esplora.. Maybe worth exploring further..

I have a code suggestion for the same checking logic.. It seems we can get the check done in much sorter codes.

rajarshimaitra

tACK ac5f0af

Thanks for the quick update..

@rajarshimaitra

This bug seems to be Electrum-specific. The fix is to check the proposed changes against the current state of the database. Ensure newly suggested indexes are not smaller than indexes already in database. Changes: * Check index updates before they are applied to database during Electrum Blockchain sync (Thank you @rajarshimaitra for providing an elegant solution). Tests added: * bdk_blockchain_tests!::test_sync_address_index_should_not_decrement * bdk_blockchain_tests!::test_sync_address_index_should_increment These tests ensure there will be no unexpected address reuse when grabbing a new address via `Wallet::get_address` with `AddressIndex::New`. Other changes: * Tweak `rpc.rs` so that clippy is happy.

afilini · 2022-07-11T10:02:11Z

ACK af6bde3

I'll wait for the CI to finish and then I'll merge this.

evanlinjin · 2022-07-03T06:43:46Z

Description

Fixes#649

It is critical to ensure Wallet::get_address with AddressIndex::new always returns a new and unused address.

This bug seems to be Electrum-specific. The fix is to check address index updates to ensure that newly suggested indexes are not smaller than indexes already in database.

Notes to the reviewers

I have written new tests in /testutils/blockchain_tests.rs that tests all Blockchain implementations.

Checklists

All Submissions:

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

Bugfixes:

* [ ] This pull request breaks the existing API

I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

rajarshimaitra

Concept ACK..

I verified that the test is only failing for Electrum, but not for RPC or Esplora.. Maybe worth exploring further..

I have a code suggestion for the same checking logic.. It seems we can get the check done in much sorter codes.

rajarshimaitra

tACK ac5f0af

Thanks for the quick update..

@rajarshimaitra

This bug seems to be Electrum-specific. The fix is to check the proposed changes against the current state of the database. Ensure newly suggested indexes are not smaller than indexes already in database. Changes: * Check index updates before they are applied to database during Electrum Blockchain sync (Thank you @rajarshimaitra for providing an elegant solution). Tests added: * bdk_blockchain_tests!::test_sync_address_index_should_not_decrement * bdk_blockchain_tests!::test_sync_address_index_should_increment These tests ensure there will be no unexpected address reuse when grabbing a new address via `Wallet::get_address` with `AddressIndex::New`. Other changes: * Tweak `rpc.rs` so that clippy is happy.

afilini · 2022-07-11T10:02:11Z

ACK af6bde3

I'll wait for the CI to finish and then I'll merge this.

evanlinjin · 2022-07-03T06:43:46Z

Description

Fixes#649

It is critical to ensure Wallet::get_address with AddressIndex::new always returns a new and unused address.

This bug seems to be Electrum-specific. The fix is to check address index updates to ensure that newly suggested indexes are not smaller than indexes already in database.

Notes to the reviewers

I have written new tests in /testutils/blockchain_tests.rs that tests all Blockchain implementations.

Checklists

All Submissions:

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

Bugfixes:

* [ ] This pull request breaks the existing API

I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

rajarshimaitra

Concept ACK..

I verified that the test is only failing for Electrum, but not for RPC or Esplora.. Maybe worth exploring further..

I have a code suggestion for the same checking logic.. It seems we can get the check done in much sorter codes.

rajarshimaitra

tACK ac5f0af

Thanks for the quick update..

@rajarshimaitra

This bug seems to be Electrum-specific. The fix is to check the proposed changes against the current state of the database. Ensure newly suggested indexes are not smaller than indexes already in database. Changes: * Check index updates before they are applied to database during Electrum Blockchain sync (Thank you @rajarshimaitra for providing an elegant solution). Tests added: * bdk_blockchain_tests!::test_sync_address_index_should_not_decrement * bdk_blockchain_tests!::test_sync_address_index_should_increment These tests ensure there will be no unexpected address reuse when grabbing a new address via `Wallet::get_address` with `AddressIndex::New`. Other changes: * Tweak `rpc.rs` so that clippy is happy.

afilini · 2022-07-11T10:02:11Z

ACK af6bde3

I'll wait for the CI to finish and then I'll merge this.

evanlinjin · 2022-07-03T06:43:46Z

Description

Fixes#649

It is critical to ensure Wallet::get_address with AddressIndex::new always returns a new and unused address.

This bug seems to be Electrum-specific. The fix is to check address index updates to ensure that newly suggested indexes are not smaller than indexes already in database.

Notes to the reviewers

I have written new tests in /testutils/blockchain_tests.rs that tests all Blockchain implementations.

Checklists

All Submissions:

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

Bugfixes:

* [ ] This pull request breaks the existing API

I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

rajarshimaitra

Concept ACK..

I verified that the test is only failing for Electrum, but not for RPC or Esplora.. Maybe worth exploring further..

I have a code suggestion for the same checking logic.. It seems we can get the check done in much sorter codes.

rajarshimaitra

tACK ac5f0af

Thanks for the quick update..

@rajarshimaitra

This bug seems to be Electrum-specific. The fix is to check the proposed changes against the current state of the database. Ensure newly suggested indexes are not smaller than indexes already in database. Changes: * Check index updates before they are applied to database during Electrum Blockchain sync (Thank you @rajarshimaitra for providing an elegant solution). Tests added: * bdk_blockchain_tests!::test_sync_address_index_should_not_decrement * bdk_blockchain_tests!::test_sync_address_index_should_increment These tests ensure there will be no unexpected address reuse when grabbing a new address via `Wallet::get_address` with `AddressIndex::New`. Other changes: * Tweak `rpc.rs` so that clippy is happy.

afilini · 2022-07-11T10:02:11Z

ACK af6bde3

I'll wait for the CI to finish and then I'll merge this.

evanlinjin · 2022-07-03T06:43:46Z

Description

Fixes#649

It is critical to ensure Wallet::get_address with AddressIndex::new always returns a new and unused address.

This bug seems to be Electrum-specific. The fix is to check address index updates to ensure that newly suggested indexes are not smaller than indexes already in database.

Notes to the reviewers

I have written new tests in /testutils/blockchain_tests.rs that tests all Blockchain implementations.

Checklists

All Submissions:

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

Bugfixes:

* [ ] This pull request breaks the existing API

I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

rajarshimaitra

Concept ACK..

I verified that the test is only failing for Electrum, but not for RPC or Esplora.. Maybe worth exploring further..

I have a code suggestion for the same checking logic.. It seems we can get the check done in much sorter codes.

rajarshimaitra

tACK ac5f0af

Thanks for the quick update..

@rajarshimaitra

This bug seems to be Electrum-specific. The fix is to check the proposed changes against the current state of the database. Ensure newly suggested indexes are not smaller than indexes already in database. Changes: * Check index updates before they are applied to database during Electrum Blockchain sync (Thank you @rajarshimaitra for providing an elegant solution). Tests added: * bdk_blockchain_tests!::test_sync_address_index_should_not_decrement * bdk_blockchain_tests!::test_sync_address_index_should_increment These tests ensure there will be no unexpected address reuse when grabbing a new address via `Wallet::get_address` with `AddressIndex::New`. Other changes: * Tweak `rpc.rs` so that clippy is happy.

afilini · 2022-07-11T10:02:11Z

ACK af6bde3

I'll wait for the CI to finish and then I'll merge this.

evanlinjin · 2022-07-03T06:43:46Z

Description

Fixes#649

It is critical to ensure Wallet::get_address with AddressIndex::new always returns a new and unused address.

This bug seems to be Electrum-specific. The fix is to check address index updates to ensure that newly suggested indexes are not smaller than indexes already in database.

Notes to the reviewers

I have written new tests in /testutils/blockchain_tests.rs that tests all Blockchain implementations.

Checklists

All Submissions:

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

Bugfixes:

* [ ] This pull request breaks the existing API

I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

rajarshimaitra

Concept ACK..

I verified that the test is only failing for Electrum, but not for RPC or Esplora.. Maybe worth exploring further..

I have a code suggestion for the same checking logic.. It seems we can get the check done in much sorter codes.

rajarshimaitra

tACK ac5f0af

Thanks for the quick update..

@rajarshimaitra

This bug seems to be Electrum-specific. The fix is to check the proposed changes against the current state of the database. Ensure newly suggested indexes are not smaller than indexes already in database. Changes: * Check index updates before they are applied to database during Electrum Blockchain sync (Thank you @rajarshimaitra for providing an elegant solution). Tests added: * bdk_blockchain_tests!::test_sync_address_index_should_not_decrement * bdk_blockchain_tests!::test_sync_address_index_should_increment These tests ensure there will be no unexpected address reuse when grabbing a new address via `Wallet::get_address` with `AddressIndex::New`. Other changes: * Tweak `rpc.rs` so that clippy is happy.

afilini · 2022-07-11T10:02:11Z

ACK af6bde3

I'll wait for the CI to finish and then I'll merge this.

evanlinjin force-pushed the fix-electrum-sync-index-decrement branch 2 times, most recently from 49cae10 to f7ec271Compare July 3, 2022 06:53

evanlinjin marked this pull request as draft July 3, 2022 07:38

evanlinjin force-pushed the fix-electrum-sync-index-decrement branch 2 times, most recently from 07fa29e to 06c76a5Compare July 3, 2022 11:18

evanlinjin marked this pull request as ready for review July 3, 2022 12:07

evanlinjin mentioned this pull request Jul 3, 2022
Syncing causes get_address index to decrement #649
Closed

afilini added this to the Release 0.20.0 Feature Freeze milestone Jul 4, 2022

evanlinjin force-pushed the fix-electrum-sync-index-decrement branch 2 times, most recently from 1123954 to f4975d1Compare July 4, 2022 16:26

notmandatory assigned evanlinjin Jul 4, 2022

notmandatory added the bug Something isn't working label Jul 4, 2022

evanlinjin force-pushed the fix-electrum-sync-index-decrement branch 4 times, most recently from d09efa9 to a8f470fCompare July 5, 2022 11:28

notmandatory mentioned this pull request Jul 5, 2022
W27 BDK Library Team Call bitcoindevkit/.github#12
Closed
24 tasks

afilini reviewed Jul 6, 2022
View reviewed changes

Comment threadsrc/blockchain/script_sync.rs Outdated

evanlinjin force-pushed the fix-electrum-sync-index-decrement branch 2 times, most recently from dcddd61 to 6f6d923Compare July 6, 2022 19:21

evanlinjin requested a review from afilini July 6, 2022 19:24

notmandatory reviewed Jul 6, 2022
View reviewed changes

Comment threadsrc/testutils/blockchain_tests.rs Outdated

evanlinjin force-pushed the fix-electrum-sync-index-decrement branch 3 times, most recently from b4b2ad9 to edf5ab7Compare July 6, 2022 19:51

rajarshimaitra suggested changes Jul 7, 2022
View reviewed changes

Comment threadsrc/blockchain/script_sync.rs Outdated

evanlinjin force-pushed the fix-electrum-sync-index-decrement branch from edf5ab7 to ac5f0afCompare July 7, 2022 18:56

evanlinjin requested review from notmandatory and rajarshimaitra July 7, 2022 19:00

evanlinjin mentioned this pull request Jul 7, 2022
ElectrumBlockchain requires a special check to not decrement index on sync. bitcoindevkit/bdk_wallet#181
Closed

rajarshimaitra approved these changes Jul 8, 2022
View reviewed changes

evanlinjin changed the base branch from master to release/0.20.0July 11, 2022 09:33

evanlinjin force-pushed the fix-electrum-sync-index-decrement branch from ac5f0af to 6f0ed1bCompare July 11, 2022 09:37

evanlinjin force-pushed the fix-electrum-sync-index-decrement branch from 6f0ed1b to af6bde3Compare July 11, 2022 09:52

afilini merged commit 5561057 into bitcoindevkit:release/0.20.0Jul 11, 2022