esplora: fix incorrect gap limit check in blocking client #1225
Conversation
|
It's a bit surprising this bug wouldn't be caught by any test. I'll look into adding one. |
|
From a real quick comparison in looks like the changed code is part of |
Probably, but to be honest i'm pretty limited in time at the moment so i'm only writing a unit test to demonstrate the bug for the blocking client. I'd prefer if another contributor took care of fixing the async client. |
darosior
force-pushed
the
2311_fix_explora_gap_limit
branch
from
65e1d36
to
d5ecfd3
Compare
|
Pushed a unit test exercising this logic as a separate commit, so it can be cherry-picked on master to demonstrate the bug. If somebody wants to take care of fixing the async client they can reuse the same logic from this test, i think. |
|
Ok so actually porting the fix and the test to the async client was actually trivial, so i just did it here. The same goes for the async unit test: it can be cherry-picked on master to demonstrate the bug. |
I can see you've done this thanks. I think we are in the middle of setting up a proper test environment for this crate in #1171. I think you can leave @evanlinjin and @LagginTimes to polish of this PR after that. |
There was a problem hiding this comment.
Thanks for spotting this major bug and quickly implementing the fix with tests to go with. The changes here are strictly better than what we had before.
However, stop_gap is now implemented slightly incorrectly. If you do not have time to make these changes, I could do it for you!
|
Okay I think we both agree that |
The gap limit was checked such as if the last_index was not None but the last_active_index was, we'd consider having reached it. But the last_index is never None for this check. This effectively made it so the gap limit was always 1: if the first address isn't used last_active_index will be None and we'd return immediately. Fix this by avoiding error-prone Option comparisons and correctly checking we've reached the gap limit before breaking out of the loop.
darosior
force-pushed
the
2311_fix_explora_gap_limit
branch
from
9e1abe1
to
43aed38
Compare
|
Ok, just pushed a version which fixes the off-by-one when the last active index is |
|
Ok the discussion about what the gap limit should be is now happening in two threads. Let's centralize it here. I'm going to close both threads. First of all let me point out how this PR does not change existing behaviour for the case where there is a last active index. We might want to change that, but maybe let's keep this for another PR: one bug at a time heh? That said, whether it happens here or in a follow-up, Daniela raises a good point. If we define "gap limit" as "the number of inactive derivation indexes before we stop" then the current implementation is wrong: as demonstrated by the newly introduced test with a gap limit of 3 we scan up to the 4th derivation index. |
There was a problem hiding this comment.
First of all let me point out how this PR does not change existing behaviour for the case where there is a last active index. We might want to change that, but maybe let's keep this for another PR: one bug at a time heh?
Fair enough! I've opened #1227 for discussion about this - even if the current definition of stop_gap is correct, it should be clearly documented :)
ACK 43aed38
The gap limit was checked such as if the last_index was not None but the last_active_index was, we'd consider having reached it. But the last_index is never None for this check. This effectively made it so the gap limit was always 1: if the first address isn't used last_active_index will be None and we'd return immediately.
Fix this by avoiding error-prone Option comparisons and correctly checking we've reached the gap limit before breaking out of the loop.