original LastUnused and get all unused indexes#1
Conversation
PR feedback: * remove batch getting `n` unused addresses, just get them all (much simpler) * use next_back() and next() for last and first unused * test more cases for get_unused_address_indexes
nickfarrow
force-pushed
the
first-unused-feedback
branch
from
0fa62b4 to
43ae40e
Compare
nickfarrow
force-pushed
the
first-unused-feedback
branch
from
ad10d41 to
de727c8
Compare
|
Can we be certain that each address index will correspond to one script pubkey? If it is possible to reuse the same address index for different scripts (obviously bad idea but could it happen?), then this could cause misalignment at for the |
LLFourn
left a comment
LLFourn
left a comment
There was a problem hiding this comment.
I like the approach and API. A few small implementation fixups todo.
| index: *current_index as u32, | ||
| keychain, | ||
| }) | ||
| .map_err(|_| Error::ScriptDoesntHaveAddressForm) |
There was a problem hiding this comment.
I think we can just call peek_address here?
You could consider ust reverting this to the master version as its behavior is not meant to change with this PR (even if the logic is slightly redundant).
| for i in 0..=current_address_index { | ||
| if (i < script_pubkeys.len()) && txn_scripts.contains(&script_pubkeys[i]) { | ||
| } else { | ||
| scripts_not_used.push(i as u32); |
There was a problem hiding this comment.
this if is kind of awkward.Can't we just iterate over script_pubkeys.enumerate() and push i if not contains.
There was a problem hiding this comment.
So one reason i iterated over 0..=current_address_index was because the script pubkeys can be empty for a new database, but address index 0. Then script_pubkeys.enumerate() misses index 0 and returns nothing. I think we want to return [0] on a fresh wallet
I can add this manually after the enumerate but it's not so nice
if script_pubkeys.len() == 0 {
scripts_not_used.push(0);
}
Related to earlier comment, the other reason is it seems possible for more scripts in the database to be populated further than the current address index.
Can handle this upper limit with take(current_address_index + 1).
Also worried about having less scripts than the current_address_index, gaps possible?
There was a problem hiding this comment.
The take(current_address_index + 1) is OK because it never gives an exception even if the index is larger than the length of script_pubkeys.
There was a problem hiding this comment.
Ok now I get it. Yeah I would do the take thing. Are you sure that current_address_index is not the one after the one you are looking for i.e. you should be iterating .take(current_address_index) and not current_address_index + 1. i.e. the "current" address is really the "next address to use".
There was a problem hiding this comment.
So current_address_index is the index of the address last derived. When the current_address_index is 0 we need to take 1 address and check if it has been used
2 participants
PR feedback:
nunused addresses, just get them all (muchsimpler)