fetch_index can mutate the database and increment the index

[nickfarrow]

wallet.fetch_index(KeychainKind) can mutate the database which can lead to unexpected results.

For example, the two following snippets give different addresses with a fresh wallet:

dbg!(wallet.get_address(New).unwrap().to_string());

wallet.fetch_index(KeychainKind::External).unwrap();
dbg!(wallet.get_address(New).unwrap().to_string());

In the 2nd case, since no index exists yet in the database, fetch_index follows the else path which fetches and increments the index:

fn fetch_index(&self, keychain: KeychainKind) -> Result<u32, Error> {
    let (descriptor, keychain) = self._get_descriptor_for_keychain(keychain);
    let index = match descriptor.is_deriveable() {
        false => Some(0),
        true => self.database.borrow_mut().get_last_index(keychain)?,
    };

    if let Some(i) = index {
        Ok(i)
    } else {
        // HERE
        self.fetch_and_increment_index(keychain)
    }
}

Perhaps fetch_index should just return None for fresh wallets?

Would replacing fetch_index with fetch_next_derivation_index fix this?

Thoughts/other solutions?

[LLFourn]

This seems like quite a severe bug to me. Very easy to design something that skips the first address. My weak preference is to do the next_derivation_index solution since that's what I did in bdk_core.

FWIW the approach in bdk_core is to simply not have this keychain index. Rather you can simply look at the last address you've derived. It doesn't make sense to derive more than you need in bdk_core. If you are deriving addresses because you are in a watch only mode that's fine but then it doesn't make sense to "get a new address" because you should not be giving out addresses at all.

[evanlinjin]

Imo the fetch index/get address logic is super confusing. It can do with some cleanup.

[notmandatory]

@nickfarrow have you seen this issue with 1.0.0-alpha? If it's fixed in 1.0.0-alpha I'd like to close this issue.