Fix: Hang when `ElectrumBlockchainConfig::stop_gap == 0` by evanlinjin · Pull Request #652 · bitcoindevkit/bdk

evanlinjin · 2022-07-03T05:00:20Z

Ensure chunk_size is > 0 during wallet sync.
Slight refactoring for better readability.
Add test: test_electrum_blockchain_factory_sync_with_stop_gaps

Description

Wallet::sync hangs indefinitely when syncing with Electrum with stop_gap set as 0.

The culprit is having chunk_size set as stop_gap. A zero value results in syncing not being able to progress.

Fixes#651

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

LLFourn · 2022-07-04T05:34:07Z

Interesting bug. I'm not sure about the semantics of stop_gap = 0? Maybe the correct thing is to do nothing. If I am going to stop after I find a gap of 0 then I should just stop right away no? (you start off with a gap of 0).

LLFourn · 2022-07-04T06:14:48Z

ACK. See #652 (comment)

chunk_size was set wrongly before. Thanks!

afilini

Concept ACK, a couple of things:

did you check if this happens with esplora as well? The two blockchains are very similar in terms of architecture and code
once you are done fixing the various comments, could you squash the commits into the first one?

afilini · 2022-07-04T09:39:49Z

+ // [1] actual_gap: Range size of address indexes without a balance
+ // [2] addrs_before: Range size of address indexes (before gap) which contains a balance
+ // [3] addrs_after: Range size of address indexes (after gap) which contains a balance
+ let test_vectors: Vec<[u64; 4]> = vec![


Wouldn't it be better to use usize here? I see that you have to cast these values in a few places

It seems that if we do this, other variables will need casting... Should we leave it as is?

Yeah at a quick glance I thought using usize would be fine, but if you need to cast anyway there's no reason to change it

evanlinjin · 2022-07-04T12:56:41Z

did you check if this happens with esplora as well? The two blockchains are very similar in terms of architecture and code

@afilini I've made a trait that can be reused for testing both esplora and electrum. I've also made it expandable, so in the future we can implement more tests in it. Let me know whether these changes make sense!

* Ensure chunk_size is > 0 during wallet sync. * Slight refactoring for better readability. * Add test: test_electrum_blockchain_factory_sync_with_stop_gaps

evanlinjin · 2022-07-04T13:56:05Z

once you are done fixing the various comments, could you squash the commits into the first one?

@afilini I feel like it's better to keep it to two commits. One if for the actual fix, the other is introducing a new testing trait that can be shared between different blockchain implementations.

What do you think?

@afilini

This is a continuation of the bitcoindevkit#651 fix. We should also check whether the same bug affects esplora as noted by @afilini. To achieve this, I've introduced a `ConfigurableBlockchainTester` trait that can test multiple blockchain implementations. * Introduce `ConfigurableBlockchainTester` trait. * Use the aforementioned trait to also test esplora. * Change the electrum test to also use the new trait. * Fix some complaints by clippy in ureq.rs file (why is CI not seeing this?). * Refactor some code.

evanlinjin · 2022-07-04T23:59:01Z

@afilini Should this fix also be part of Release 0.20.0 Feature Freeze?

Ensuring that the Blockchain implementations respect the stop_gap parameter can potentially be important for avoiding address reuse.

The test I have included checks for this.

bdk/src/testutils/configurable_blockchain_tests.rs

Lines 131 to 135 in 612da16

assert!(
 wallet_balance >= min_balance,
"wallet balance is smaller than expected: {}",
 details 
);

afilini · 2022-07-05T10:16:29Z

@afilini I feel like it's better to keep it to two commits. One if for the actual fix, the other is introducing a new testing trait that can be shared between different blockchain implementations.

Yeah two commits works for me, as long as they are self-contained (they should both compile and run cargo test with no errors) and they have a good commit message

@afilini Should this fix also be part of Release 0.20.0 Feature Freeze?

I don't think this is as critical as that other one, but if this is ready in time yes, this is also important

afilini

ACK 612da16

evanlinjin · 2022-07-03T05:00:20Z

Ensure chunk_size is > 0 during wallet sync.
Slight refactoring for better readability.
Add test: test_electrum_blockchain_factory_sync_with_stop_gaps

Description

Wallet::sync hangs indefinitely when syncing with Electrum with stop_gap set as 0.

The culprit is having chunk_size set as stop_gap. A zero value results in syncing not being able to progress.

Fixes#651

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

LLFourn · 2022-07-04T05:34:07Z

Interesting bug. I'm not sure about the semantics of stop_gap = 0? Maybe the correct thing is to do nothing. If I am going to stop after I find a gap of 0 then I should just stop right away no? (you start off with a gap of 0).

LLFourn · 2022-07-04T06:14:48Z

ACK. See #652 (comment)

chunk_size was set wrongly before. Thanks!

afilini

Concept ACK, a couple of things:

did you check if this happens with esplora as well? The two blockchains are very similar in terms of architecture and code
once you are done fixing the various comments, could you squash the commits into the first one?

afilini · 2022-07-04T09:39:49Z

+ // [1] actual_gap: Range size of address indexes without a balance
+ // [2] addrs_before: Range size of address indexes (before gap) which contains a balance
+ // [3] addrs_after: Range size of address indexes (after gap) which contains a balance
+ let test_vectors: Vec<[u64; 4]> = vec![


Wouldn't it be better to use usize here? I see that you have to cast these values in a few places

It seems that if we do this, other variables will need casting... Should we leave it as is?

Yeah at a quick glance I thought using usize would be fine, but if you need to cast anyway there's no reason to change it

evanlinjin · 2022-07-04T12:56:41Z

did you check if this happens with esplora as well? The two blockchains are very similar in terms of architecture and code

@afilini I've made a trait that can be reused for testing both esplora and electrum. I've also made it expandable, so in the future we can implement more tests in it. Let me know whether these changes make sense!

* Ensure chunk_size is > 0 during wallet sync. * Slight refactoring for better readability. * Add test: test_electrum_blockchain_factory_sync_with_stop_gaps

evanlinjin · 2022-07-04T13:56:05Z

once you are done fixing the various comments, could you squash the commits into the first one?

@afilini I feel like it's better to keep it to two commits. One if for the actual fix, the other is introducing a new testing trait that can be shared between different blockchain implementations.

What do you think?

@afilini

This is a continuation of the bitcoindevkit#651 fix. We should also check whether the same bug affects esplora as noted by @afilini. To achieve this, I've introduced a `ConfigurableBlockchainTester` trait that can test multiple blockchain implementations. * Introduce `ConfigurableBlockchainTester` trait. * Use the aforementioned trait to also test esplora. * Change the electrum test to also use the new trait. * Fix some complaints by clippy in ureq.rs file (why is CI not seeing this?). * Refactor some code.

evanlinjin · 2022-07-04T23:59:01Z

@afilini Should this fix also be part of Release 0.20.0 Feature Freeze?

Ensuring that the Blockchain implementations respect the stop_gap parameter can potentially be important for avoiding address reuse.

The test I have included checks for this.

bdk/src/testutils/configurable_blockchain_tests.rs

Lines 131 to 135 in 612da16

assert!(
 wallet_balance >= min_balance,
"wallet balance is smaller than expected: {}",
 details 
);

afilini · 2022-07-05T10:16:29Z

@afilini I feel like it's better to keep it to two commits. One if for the actual fix, the other is introducing a new testing trait that can be shared between different blockchain implementations.

Yeah two commits works for me, as long as they are self-contained (they should both compile and run cargo test with no errors) and they have a good commit message

@afilini Should this fix also be part of Release 0.20.0 Feature Freeze?

I don't think this is as critical as that other one, but if this is ready in time yes, this is also important

afilini

ACK 612da16

evanlinjin · 2022-07-03T05:00:20Z

Ensure chunk_size is > 0 during wallet sync.
Slight refactoring for better readability.
Add test: test_electrum_blockchain_factory_sync_with_stop_gaps

Description

Wallet::sync hangs indefinitely when syncing with Electrum with stop_gap set as 0.

The culprit is having chunk_size set as stop_gap. A zero value results in syncing not being able to progress.

Fixes#651

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

LLFourn · 2022-07-04T05:34:07Z

Interesting bug. I'm not sure about the semantics of stop_gap = 0? Maybe the correct thing is to do nothing. If I am going to stop after I find a gap of 0 then I should just stop right away no? (you start off with a gap of 0).

LLFourn · 2022-07-04T06:14:48Z

ACK. See #652 (comment)

chunk_size was set wrongly before. Thanks!

afilini

Concept ACK, a couple of things:

did you check if this happens with esplora as well? The two blockchains are very similar in terms of architecture and code
once you are done fixing the various comments, could you squash the commits into the first one?

afilini · 2022-07-04T09:39:49Z

+ // [1] actual_gap: Range size of address indexes without a balance
+ // [2] addrs_before: Range size of address indexes (before gap) which contains a balance
+ // [3] addrs_after: Range size of address indexes (after gap) which contains a balance
+ let test_vectors: Vec<[u64; 4]> = vec![


Wouldn't it be better to use usize here? I see that you have to cast these values in a few places

It seems that if we do this, other variables will need casting... Should we leave it as is?

Yeah at a quick glance I thought using usize would be fine, but if you need to cast anyway there's no reason to change it

evanlinjin · 2022-07-04T12:56:41Z

did you check if this happens with esplora as well? The two blockchains are very similar in terms of architecture and code

@afilini I've made a trait that can be reused for testing both esplora and electrum. I've also made it expandable, so in the future we can implement more tests in it. Let me know whether these changes make sense!

* Ensure chunk_size is > 0 during wallet sync. * Slight refactoring for better readability. * Add test: test_electrum_blockchain_factory_sync_with_stop_gaps

evanlinjin · 2022-07-04T13:56:05Z

once you are done fixing the various comments, could you squash the commits into the first one?

@afilini I feel like it's better to keep it to two commits. One if for the actual fix, the other is introducing a new testing trait that can be shared between different blockchain implementations.

What do you think?

@afilini

This is a continuation of the bitcoindevkit#651 fix. We should also check whether the same bug affects esplora as noted by @afilini. To achieve this, I've introduced a `ConfigurableBlockchainTester` trait that can test multiple blockchain implementations. * Introduce `ConfigurableBlockchainTester` trait. * Use the aforementioned trait to also test esplora. * Change the electrum test to also use the new trait. * Fix some complaints by clippy in ureq.rs file (why is CI not seeing this?). * Refactor some code.

evanlinjin · 2022-07-04T23:59:01Z

@afilini Should this fix also be part of Release 0.20.0 Feature Freeze?

Ensuring that the Blockchain implementations respect the stop_gap parameter can potentially be important for avoiding address reuse.

The test I have included checks for this.

bdk/src/testutils/configurable_blockchain_tests.rs

Lines 131 to 135 in 612da16

assert!(
 wallet_balance >= min_balance,
"wallet balance is smaller than expected: {}",
 details 
);

afilini · 2022-07-05T10:16:29Z

@afilini I feel like it's better to keep it to two commits. One if for the actual fix, the other is introducing a new testing trait that can be shared between different blockchain implementations.

Yeah two commits works for me, as long as they are self-contained (they should both compile and run cargo test with no errors) and they have a good commit message

@afilini Should this fix also be part of Release 0.20.0 Feature Freeze?

I don't think this is as critical as that other one, but if this is ready in time yes, this is also important

afilini

ACK 612da16

evanlinjin · 2022-07-03T05:00:20Z

Ensure chunk_size is > 0 during wallet sync.
Slight refactoring for better readability.
Add test: test_electrum_blockchain_factory_sync_with_stop_gaps

Description

Wallet::sync hangs indefinitely when syncing with Electrum with stop_gap set as 0.

The culprit is having chunk_size set as stop_gap. A zero value results in syncing not being able to progress.

Fixes#651

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

LLFourn · 2022-07-04T05:34:07Z

Interesting bug. I'm not sure about the semantics of stop_gap = 0? Maybe the correct thing is to do nothing. If I am going to stop after I find a gap of 0 then I should just stop right away no? (you start off with a gap of 0).

LLFourn · 2022-07-04T06:14:48Z

ACK. See #652 (comment)

chunk_size was set wrongly before. Thanks!

afilini

Concept ACK, a couple of things:

did you check if this happens with esplora as well? The two blockchains are very similar in terms of architecture and code
once you are done fixing the various comments, could you squash the commits into the first one?

afilini · 2022-07-04T09:39:49Z

+ // [1] actual_gap: Range size of address indexes without a balance
+ // [2] addrs_before: Range size of address indexes (before gap) which contains a balance
+ // [3] addrs_after: Range size of address indexes (after gap) which contains a balance
+ let test_vectors: Vec<[u64; 4]> = vec![


Wouldn't it be better to use usize here? I see that you have to cast these values in a few places

It seems that if we do this, other variables will need casting... Should we leave it as is?

Yeah at a quick glance I thought using usize would be fine, but if you need to cast anyway there's no reason to change it

evanlinjin · 2022-07-04T12:56:41Z

did you check if this happens with esplora as well? The two blockchains are very similar in terms of architecture and code

@afilini I've made a trait that can be reused for testing both esplora and electrum. I've also made it expandable, so in the future we can implement more tests in it. Let me know whether these changes make sense!

* Ensure chunk_size is > 0 during wallet sync. * Slight refactoring for better readability. * Add test: test_electrum_blockchain_factory_sync_with_stop_gaps

evanlinjin · 2022-07-04T13:56:05Z

once you are done fixing the various comments, could you squash the commits into the first one?

@afilini I feel like it's better to keep it to two commits. One if for the actual fix, the other is introducing a new testing trait that can be shared between different blockchain implementations.

What do you think?

@afilini

This is a continuation of the bitcoindevkit#651 fix. We should also check whether the same bug affects esplora as noted by @afilini. To achieve this, I've introduced a `ConfigurableBlockchainTester` trait that can test multiple blockchain implementations. * Introduce `ConfigurableBlockchainTester` trait. * Use the aforementioned trait to also test esplora. * Change the electrum test to also use the new trait. * Fix some complaints by clippy in ureq.rs file (why is CI not seeing this?). * Refactor some code.

evanlinjin · 2022-07-04T23:59:01Z

@afilini Should this fix also be part of Release 0.20.0 Feature Freeze?

Ensuring that the Blockchain implementations respect the stop_gap parameter can potentially be important for avoiding address reuse.

The test I have included checks for this.

bdk/src/testutils/configurable_blockchain_tests.rs

Lines 131 to 135 in 612da16

assert!(
 wallet_balance >= min_balance,
"wallet balance is smaller than expected: {}",
 details 
);

afilini · 2022-07-05T10:16:29Z

@afilini I feel like it's better to keep it to two commits. One if for the actual fix, the other is introducing a new testing trait that can be shared between different blockchain implementations.

Yeah two commits works for me, as long as they are self-contained (they should both compile and run cargo test with no errors) and they have a good commit message

@afilini Should this fix also be part of Release 0.20.0 Feature Freeze?

I don't think this is as critical as that other one, but if this is ready in time yes, this is also important

afilini

ACK 612da16

evanlinjin · 2022-07-03T05:00:20Z

Ensure chunk_size is > 0 during wallet sync.
Slight refactoring for better readability.
Add test: test_electrum_blockchain_factory_sync_with_stop_gaps

Description

Wallet::sync hangs indefinitely when syncing with Electrum with stop_gap set as 0.

The culprit is having chunk_size set as stop_gap. A zero value results in syncing not being able to progress.

Fixes#651

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

LLFourn · 2022-07-04T05:34:07Z

Interesting bug. I'm not sure about the semantics of stop_gap = 0? Maybe the correct thing is to do nothing. If I am going to stop after I find a gap of 0 then I should just stop right away no? (you start off with a gap of 0).

LLFourn · 2022-07-04T06:14:48Z

ACK. See #652 (comment)

chunk_size was set wrongly before. Thanks!

afilini

Concept ACK, a couple of things:

did you check if this happens with esplora as well? The two blockchains are very similar in terms of architecture and code
once you are done fixing the various comments, could you squash the commits into the first one?

afilini · 2022-07-04T09:39:49Z

+ // [1] actual_gap: Range size of address indexes without a balance
+ // [2] addrs_before: Range size of address indexes (before gap) which contains a balance
+ // [3] addrs_after: Range size of address indexes (after gap) which contains a balance
+ let test_vectors: Vec<[u64; 4]> = vec![


Wouldn't it be better to use usize here? I see that you have to cast these values in a few places

It seems that if we do this, other variables will need casting... Should we leave it as is?

Yeah at a quick glance I thought using usize would be fine, but if you need to cast anyway there's no reason to change it

evanlinjin · 2022-07-04T12:56:41Z

did you check if this happens with esplora as well? The two blockchains are very similar in terms of architecture and code

@afilini I've made a trait that can be reused for testing both esplora and electrum. I've also made it expandable, so in the future we can implement more tests in it. Let me know whether these changes make sense!

* Ensure chunk_size is > 0 during wallet sync. * Slight refactoring for better readability. * Add test: test_electrum_blockchain_factory_sync_with_stop_gaps

evanlinjin · 2022-07-04T13:56:05Z

once you are done fixing the various comments, could you squash the commits into the first one?

@afilini I feel like it's better to keep it to two commits. One if for the actual fix, the other is introducing a new testing trait that can be shared between different blockchain implementations.

What do you think?

@afilini

This is a continuation of the bitcoindevkit#651 fix. We should also check whether the same bug affects esplora as noted by @afilini. To achieve this, I've introduced a `ConfigurableBlockchainTester` trait that can test multiple blockchain implementations. * Introduce `ConfigurableBlockchainTester` trait. * Use the aforementioned trait to also test esplora. * Change the electrum test to also use the new trait. * Fix some complaints by clippy in ureq.rs file (why is CI not seeing this?). * Refactor some code.

evanlinjin · 2022-07-04T23:59:01Z

@afilini Should this fix also be part of Release 0.20.0 Feature Freeze?

Ensuring that the Blockchain implementations respect the stop_gap parameter can potentially be important for avoiding address reuse.

The test I have included checks for this.

bdk/src/testutils/configurable_blockchain_tests.rs

Lines 131 to 135 in 612da16

assert!(
 wallet_balance >= min_balance,
"wallet balance is smaller than expected: {}",
 details 
);

afilini · 2022-07-05T10:16:29Z

@afilini I feel like it's better to keep it to two commits. One if for the actual fix, the other is introducing a new testing trait that can be shared between different blockchain implementations.

Yeah two commits works for me, as long as they are self-contained (they should both compile and run cargo test with no errors) and they have a good commit message

@afilini Should this fix also be part of Release 0.20.0 Feature Freeze?

I don't think this is as critical as that other one, but if this is ready in time yes, this is also important

afilini

ACK 612da16

evanlinjin · 2022-07-03T05:00:20Z

Ensure chunk_size is > 0 during wallet sync.
Slight refactoring for better readability.
Add test: test_electrum_blockchain_factory_sync_with_stop_gaps

Description

Wallet::sync hangs indefinitely when syncing with Electrum with stop_gap set as 0.

The culprit is having chunk_size set as stop_gap. A zero value results in syncing not being able to progress.

Fixes#651

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

LLFourn · 2022-07-04T05:34:07Z

Interesting bug. I'm not sure about the semantics of stop_gap = 0? Maybe the correct thing is to do nothing. If I am going to stop after I find a gap of 0 then I should just stop right away no? (you start off with a gap of 0).

LLFourn · 2022-07-04T06:14:48Z

ACK. See #652 (comment)

chunk_size was set wrongly before. Thanks!

afilini

Concept ACK, a couple of things:

did you check if this happens with esplora as well? The two blockchains are very similar in terms of architecture and code
once you are done fixing the various comments, could you squash the commits into the first one?

afilini · 2022-07-04T09:39:49Z

+ // [1] actual_gap: Range size of address indexes without a balance
+ // [2] addrs_before: Range size of address indexes (before gap) which contains a balance
+ // [3] addrs_after: Range size of address indexes (after gap) which contains a balance
+ let test_vectors: Vec<[u64; 4]> = vec![


Wouldn't it be better to use usize here? I see that you have to cast these values in a few places

It seems that if we do this, other variables will need casting... Should we leave it as is?

Yeah at a quick glance I thought using usize would be fine, but if you need to cast anyway there's no reason to change it

evanlinjin · 2022-07-04T12:56:41Z

did you check if this happens with esplora as well? The two blockchains are very similar in terms of architecture and code

@afilini I've made a trait that can be reused for testing both esplora and electrum. I've also made it expandable, so in the future we can implement more tests in it. Let me know whether these changes make sense!

* Ensure chunk_size is > 0 during wallet sync. * Slight refactoring for better readability. * Add test: test_electrum_blockchain_factory_sync_with_stop_gaps

evanlinjin · 2022-07-04T13:56:05Z

once you are done fixing the various comments, could you squash the commits into the first one?

@afilini I feel like it's better to keep it to two commits. One if for the actual fix, the other is introducing a new testing trait that can be shared between different blockchain implementations.

What do you think?

@afilini

This is a continuation of the bitcoindevkit#651 fix. We should also check whether the same bug affects esplora as noted by @afilini. To achieve this, I've introduced a `ConfigurableBlockchainTester` trait that can test multiple blockchain implementations. * Introduce `ConfigurableBlockchainTester` trait. * Use the aforementioned trait to also test esplora. * Change the electrum test to also use the new trait. * Fix some complaints by clippy in ureq.rs file (why is CI not seeing this?). * Refactor some code.

evanlinjin · 2022-07-04T23:59:01Z

@afilini Should this fix also be part of Release 0.20.0 Feature Freeze?

Ensuring that the Blockchain implementations respect the stop_gap parameter can potentially be important for avoiding address reuse.

The test I have included checks for this.

bdk/src/testutils/configurable_blockchain_tests.rs

Lines 131 to 135 in 612da16

assert!(
 wallet_balance >= min_balance,
"wallet balance is smaller than expected: {}",
 details 
);

afilini · 2022-07-05T10:16:29Z

@afilini I feel like it's better to keep it to two commits. One if for the actual fix, the other is introducing a new testing trait that can be shared between different blockchain implementations.

Yeah two commits works for me, as long as they are self-contained (they should both compile and run cargo test with no errors) and they have a good commit message

@afilini Should this fix also be part of Release 0.20.0 Feature Freeze?

I don't think this is as critical as that other one, but if this is ready in time yes, this is also important

afilini

ACK 612da16

evanlinjin · 2022-07-03T05:00:20Z

Ensure chunk_size is > 0 during wallet sync.
Slight refactoring for better readability.
Add test: test_electrum_blockchain_factory_sync_with_stop_gaps

Description

Wallet::sync hangs indefinitely when syncing with Electrum with stop_gap set as 0.

The culprit is having chunk_size set as stop_gap. A zero value results in syncing not being able to progress.

Fixes#651

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

LLFourn · 2022-07-04T05:34:07Z

Interesting bug. I'm not sure about the semantics of stop_gap = 0? Maybe the correct thing is to do nothing. If I am going to stop after I find a gap of 0 then I should just stop right away no? (you start off with a gap of 0).

LLFourn · 2022-07-04T06:14:48Z

ACK. See #652 (comment)

chunk_size was set wrongly before. Thanks!

afilini

Concept ACK, a couple of things:

did you check if this happens with esplora as well? The two blockchains are very similar in terms of architecture and code
once you are done fixing the various comments, could you squash the commits into the first one?

afilini · 2022-07-04T09:39:49Z

+ // [1] actual_gap: Range size of address indexes without a balance
+ // [2] addrs_before: Range size of address indexes (before gap) which contains a balance
+ // [3] addrs_after: Range size of address indexes (after gap) which contains a balance
+ let test_vectors: Vec<[u64; 4]> = vec![


Wouldn't it be better to use usize here? I see that you have to cast these values in a few places

It seems that if we do this, other variables will need casting... Should we leave it as is?

Yeah at a quick glance I thought using usize would be fine, but if you need to cast anyway there's no reason to change it

evanlinjin · 2022-07-04T12:56:41Z

did you check if this happens with esplora as well? The two blockchains are very similar in terms of architecture and code

@afilini I've made a trait that can be reused for testing both esplora and electrum. I've also made it expandable, so in the future we can implement more tests in it. Let me know whether these changes make sense!

* Ensure chunk_size is > 0 during wallet sync. * Slight refactoring for better readability. * Add test: test_electrum_blockchain_factory_sync_with_stop_gaps

evanlinjin · 2022-07-04T13:56:05Z

once you are done fixing the various comments, could you squash the commits into the first one?

@afilini I feel like it's better to keep it to two commits. One if for the actual fix, the other is introducing a new testing trait that can be shared between different blockchain implementations.

What do you think?

@afilini

This is a continuation of the bitcoindevkit#651 fix. We should also check whether the same bug affects esplora as noted by @afilini. To achieve this, I've introduced a `ConfigurableBlockchainTester` trait that can test multiple blockchain implementations. * Introduce `ConfigurableBlockchainTester` trait. * Use the aforementioned trait to also test esplora. * Change the electrum test to also use the new trait. * Fix some complaints by clippy in ureq.rs file (why is CI not seeing this?). * Refactor some code.

evanlinjin · 2022-07-04T23:59:01Z

@afilini Should this fix also be part of Release 0.20.0 Feature Freeze?

Ensuring that the Blockchain implementations respect the stop_gap parameter can potentially be important for avoiding address reuse.

The test I have included checks for this.

bdk/src/testutils/configurable_blockchain_tests.rs

Lines 131 to 135 in 612da16

assert!(
 wallet_balance >= min_balance,
"wallet balance is smaller than expected: {}",
 details 
);

afilini · 2022-07-05T10:16:29Z

@afilini I feel like it's better to keep it to two commits. One if for the actual fix, the other is introducing a new testing trait that can be shared between different blockchain implementations.

Yeah two commits works for me, as long as they are self-contained (they should both compile and run cargo test with no errors) and they have a good commit message

@afilini Should this fix also be part of Release 0.20.0 Feature Freeze?

I don't think this is as critical as that other one, but if this is ready in time yes, this is also important

afilini

ACK 612da16

evanlinjin · 2022-07-03T05:00:20Z

Ensure chunk_size is > 0 during wallet sync.
Slight refactoring for better readability.
Add test: test_electrum_blockchain_factory_sync_with_stop_gaps

Description

Wallet::sync hangs indefinitely when syncing with Electrum with stop_gap set as 0.

The culprit is having chunk_size set as stop_gap. A zero value results in syncing not being able to progress.

Fixes#651

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

LLFourn · 2022-07-04T05:34:07Z

Interesting bug. I'm not sure about the semantics of stop_gap = 0? Maybe the correct thing is to do nothing. If I am going to stop after I find a gap of 0 then I should just stop right away no? (you start off with a gap of 0).

LLFourn · 2022-07-04T06:14:48Z

ACK. See #652 (comment)

chunk_size was set wrongly before. Thanks!

afilini

Concept ACK, a couple of things:

did you check if this happens with esplora as well? The two blockchains are very similar in terms of architecture and code
once you are done fixing the various comments, could you squash the commits into the first one?

afilini · 2022-07-04T09:39:49Z

+ // [1] actual_gap: Range size of address indexes without a balance
+ // [2] addrs_before: Range size of address indexes (before gap) which contains a balance
+ // [3] addrs_after: Range size of address indexes (after gap) which contains a balance
+ let test_vectors: Vec<[u64; 4]> = vec![


Wouldn't it be better to use usize here? I see that you have to cast these values in a few places

It seems that if we do this, other variables will need casting... Should we leave it as is?

Yeah at a quick glance I thought using usize would be fine, but if you need to cast anyway there's no reason to change it

evanlinjin · 2022-07-04T12:56:41Z

did you check if this happens with esplora as well? The two blockchains are very similar in terms of architecture and code

@afilini I've made a trait that can be reused for testing both esplora and electrum. I've also made it expandable, so in the future we can implement more tests in it. Let me know whether these changes make sense!

* Ensure chunk_size is > 0 during wallet sync. * Slight refactoring for better readability. * Add test: test_electrum_blockchain_factory_sync_with_stop_gaps

evanlinjin · 2022-07-04T13:56:05Z

once you are done fixing the various comments, could you squash the commits into the first one?

@afilini I feel like it's better to keep it to two commits. One if for the actual fix, the other is introducing a new testing trait that can be shared between different blockchain implementations.

What do you think?

@afilini

This is a continuation of the bitcoindevkit#651 fix. We should also check whether the same bug affects esplora as noted by @afilini. To achieve this, I've introduced a `ConfigurableBlockchainTester` trait that can test multiple blockchain implementations. * Introduce `ConfigurableBlockchainTester` trait. * Use the aforementioned trait to also test esplora. * Change the electrum test to also use the new trait. * Fix some complaints by clippy in ureq.rs file (why is CI not seeing this?). * Refactor some code.

evanlinjin · 2022-07-04T23:59:01Z

@afilini Should this fix also be part of Release 0.20.0 Feature Freeze?

Ensuring that the Blockchain implementations respect the stop_gap parameter can potentially be important for avoiding address reuse.

The test I have included checks for this.

bdk/src/testutils/configurable_blockchain_tests.rs

Lines 131 to 135 in 612da16

assert!(
 wallet_balance >= min_balance,
"wallet balance is smaller than expected: {}",
 details 
);

afilini · 2022-07-05T10:16:29Z

@afilini I feel like it's better to keep it to two commits. One if for the actual fix, the other is introducing a new testing trait that can be shared between different blockchain implementations.

Yeah two commits works for me, as long as they are self-contained (they should both compile and run cargo test with no errors) and they have a good commit message

@afilini Should this fix also be part of Release 0.20.0 Feature Freeze?

I don't think this is as critical as that other one, but if this is ready in time yes, this is also important

afilini

ACK 612da16

evanlinjin changed the title ~~Fix hang when ElectrumBlockchainConfig::stop_gap == 0~~Fix: hang when ElectrumBlockchainConfig::stop_gap == 0Jul 3, 2022

evanlinjin changed the title ~~Fix: hang when ElectrumBlockchainConfig::stop_gap == 0~~Fix: Hang when ElectrumBlockchainConfig::stop_gap == 0Jul 3, 2022

This comment was marked as resolved.
Sign in to view

LLFourn reviewed Jul 4, 2022
View reviewed changes

Comment threadsrc/blockchain/electrum.rs Outdated

LLFourn reviewed Jul 4, 2022
View reviewed changes

Comment threadsrc/blockchain/electrum.rs Outdated

evanlinjin force-pushed the fix-electrum-stop-gap-0 branch from 629604f to 475b005Compare July 4, 2022 06:46

afilini reviewed Jul 4, 2022
View reviewed changes

evanlinjin force-pushed the fix-electrum-stop-gap-0 branch 3 times, most recently from 924b320 to 35a49fdCompare July 4, 2022 13:45

Fix hang when ElectrumBlockchainConfig::stop_gap == 0
8a5f89e
* Ensure chunk_size is > 0 during wallet sync. * Slight refactoring for better readability. * Add test: test_electrum_blockchain_factory_sync_with_stop_gaps

evanlinjin force-pushed the fix-electrum-stop-gap-0 branch from 35a49fd to 3533afaCompare July 4, 2022 13:53

evanlinjin requested review from LLFourn and afilini July 4, 2022 13:56

notmandatory assigned evanlinjin Jul 4, 2022

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

evanlinjin force-pushed the fix-electrum-stop-gap-0 branch from 3533afa to a884a63Compare July 4, 2022 23:43

evanlinjin force-pushed the fix-electrum-stop-gap-0 branch from a884a63 to 612da16Compare July 4, 2022 23:53

afilini approved these changes Jul 5, 2022
View reviewed changes

afilini merged commit 0e92820 into bitcoindevkit:masterJul 5, 2022

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

notmandatory added this to the Release 0.20.0 Feature Freeze milestone Jul 5, 2022