[wallet] Enable support for single descriptor wallets

[ValuedMammal]

The change descriptor is made optional, making this an effective reversion of #1390 and enables creating wallets with a single descriptor.

fixes #1511

Notes to the reviewers

PR 1390 also removed an error case ChangePolicyDescriptor and this can possibly be added back. In the case the wallet only has a single descriptor we allow any utxos to fund a tx that aren't specifically marked unspendable regardless of the change spend policy.

Changelog notice

• Added method Wallet::create_single that expects a single D: IntoWalletDescriptor as input and enables building a wallet with no internal keychain.

Checklists

All Submissions:

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

New Features:

• This pull request breaks the existing API
• I've added tests for the new feature
• I've added docs for the new feature

[stevenroose]

Much love for this one 😉

[ValuedMammal]

What should be the function signature and behavior of Wallet::public_descriptor ?

[thunderbiscuit]

Thanks for the input Evan about the new/load taking only and all that's required to build, and further methods that provide setter/validators to be optional.

While we're on the topic and refactoring of the builder I figured I would just ask here instead of opening a new issue:

Q1

Most of the fields on the LoadParams type are "checkers/validators" (for example check_network, check_genesis_hash, check_descriptor), but the methods on the type are simply network, descriptor, and genesis_hash. This makes it unintuitive when reading the docs and the examples, because you're left wondering whether the network is persisted at all or if you need to set it correctly while using the load builder, or whether you might in fact be able to change it between loads. My suggestion would be to use this PR to fix the name on the validator methods to be either in the style of validate_network or check_network for all 4 methods. Let me know everyone if I've missed something or if there is a better way or reason to keep them unclear.

Q2

I see that the lookahead is a setter, but even after reading the docs I'm not sure what it's setting exactly because it just sounds like a sort of stop gap but not a stop gap. See the docs here on lookahead and the more explicit lookahead in KeychainTxOutIndex.

When an user first recovers a wallet (i.e. from a recovery phrase and/or descriptor), we will NOT have knowledge of which script pubkeys are revealed. So when we index a transaction or txout (using index_tx/index_txout) we scan the txouts against script pubkeys derived above the last revealed index. These additionally-derived script pubkeys are called the lookahead.

Don't we scan up to the stop gap? I know this from experience when looking at the Esplora logs on full scan and syncs (for example a stop gap of 3 with a lookahead of 100 will query the Esplora instance for only 4 addresses), so not sure if maybe it refers to something else?

The KeychainTxOutIndex is constructed with the lookahead and cannot be altered.

Wait so is it persisted then? Or by that do we mean it can't be altered per session but altered on every load of the wallet?

Q3

Related to (Q2), I know (think?) the lookahead is not persisted, but is there a danger in having two different lookahead between sessions? What if I set a lookahead of 3 when I create a wallet, and 300 the next time I load the wallet? Or the opposite?

Sorry I know it's a lot but I did not have time to review #1514 and I'm just now after the week in Nashville confident enough to ask my questions and comment on the shape of the API. Cheers!

[ValuedMammal]

Don't we scan up to the stop gap?

The docs describing the "lookahead" pertain specifically to the correct functioning of an Indexer when indexing transactions. Some number of lookahead SPKs are necessary for the indexer to recognize when a tx is relevant to its tracked SPKs. The stop-gap used during a full scan depends on there being a lookahead, in other words having a large stop-gap won't help if you haven't stored any lookaheads.

edit: see below

is there a danger in having two different lookahead between sessions?

I think it's a valid question..

[notmandatory]

What should be the function signature and behavior of Wallet::public_descriptor ?

Since single descriptor wallets are the advanced user/unusual case I think any Wallet methods that require a keychain_kind like Wallet::public_descriptor should panic if called with KeychainKind::Internal. This behavior should be documented on the Wallet::new_single_descriptor so the user knows it is their responsibility to only use KeychainKind::External. If this seems too harsh maybe we should hide the Wallet::new_single_descriptor option behind a non-default feature flag.

[thunderbiscuit]

Some number of lookahead SPKs are necessary for the indexer to recognize when a tx is relevant to its tracked SPKs. The stop-gap used during a full scan depends on there being a lookahead, in other words having a large stop-gap won't help if you haven't stored any lookaheads.

That sort of makes it sound like it could be a pretty good footgun then no? Big stop gap, tiny lookahead, your wallet not finding your txs? I have not, however, been able to reproduce this locally, so I'm still unclear about what this lookahead is doing, and because it's part of the public API of the Wallet type I want to make sure I really get it. Here's what I have tried on regtest, and looking at the logs for my esplora instance:

• Stop gap of 3, lookahead of 100. The wallet syncs 6 addresses total (3 per keychain). The lookahead does not impact the spks the wallet attempts to sync.
• Stop gap of 25, lookahead of 7. The wallet syncs 50 addresses, (lookahead didn't seem to be a problem, again not impacting the syncing process)
• Send coins to address 19, set the lookahead to 7 and the stop gap to 25. The coins are seen and wallet shows correct balance. Lookahead below the stop gap did not cause any problems.

More notes on this: the docs here say:

Without a lookahead the index will miss outputs you own when processing transactions whose output script pubkeys lie beyond the last revealed index. In certain situations, such as when performing an initial scan of the blockchain during wallet import, it may be uncertain or unknown what the index of the last revealed script pubkey actually is.

Again this makes it sound close to the concept of the stop gap, but I assume it's different somehow or a more generalized version of it that can be used in the deeper logic of the KeychainIndex? That's as far as the API docs go. I'll try to look at where it's actually used in the chain crate tomorrow and report back.

[ValuedMammal]

@thunderbiscuit Ah, you're right, the lookahead doesn't appear to have a role during full scan for esplora because we get the SPKs to sync by calling all_unbounded_spk_iters

For sync it's also not relevant because we only sync SPKs that are already revealed.

In short, the lookahead defines a number of scripts to derive ahead of time without revealing them. If you do a full scan with the default lookahead of 25 and persist the wallet, then on subsequent loads those SPKs are still stored in the index and there shouldn't be any harm in setting a new lookahead.

edit: Nope that's also wrong. we don't persist derived SPKs, only the value of last_revealed. So yeah we probably need more testing.

One way to illustrate the effect of the lookahead is to create a wallet with the default value of 25. Then peek at address index 25. The method Wallet::is_mine will return false because internally the address is not yet derived (remembering that SPKs are 0-indexed).

For context one of the reasons we chose to make the lookahead configurable is because for a short time we had it set to 1000 and users reported unbearably long load times. Beyond that, I don't think it's something users should generally care about other than to know it exists

[ValuedMammal]

Would it be easier to just bring back the map_keychain functionality as a way to avoid panics?

[evanlinjin]

@ValuedMammal I think I agree with you here. What I am suggesting requires too much work. What you suggest works good enough to make the users happy.

[notmandatory]

Would it be easier to just bring back the map_keychain functionality as a way to avoid panics?

After sleeping on it I also support avoiding the panics by just pulling from the External keychain when ever the Internal one is requested, which is what the TxBuilder is going to do. We just need to document it (and the TxBuilder "change" related functions won't work anymore).

I can also see that @evanlinjin's suggested builder change of making new() take the External and a different function to set the Internal isn't too big a breaking change. This way also sets us up for supporting multipath descriptors (BIP-389).

We should finalize these decisions first thing in tomorrow's dev call so @ValuedMammal can finish up this PR.

[ValuedMammal]

I didn't touch the method names for LoadParams as noted in #1533 (comment). I think the docs are clear enough but I'm open to more discussion. Appreciate everyone's input

[ValuedMammal]

Attempting to load a persisted wallet with no change desc caused an error when reading from sqlite - currently investigating

Pushed a fix in d4fbe66

[thunderbiscuit]

As requested over the dev call my comment above about the method names on the load builder does not need to be addressed here, and instead can be reviewed in a separate PR: #1537.

[ValuedMammal]

Followup question: when loading a single descriptor wallet changeset.change_descriptor will be None. In this case if the user calls descriptor on LoadParams passing KeychainKind::Internal, should we

• add a change descriptor to the wallet
• error because it doesn't match what was loaded
• or silently ignore the param?

[evanlinjin]

Followup question: when loading a single descriptor wallet changeset.change_descriptor will be None. In this case if the user calls descriptor on LoadParams passing KeychainKind::Internal, should we

* add a change descriptor to the wallet

* error because it doesn't match what was loaded

* or silently ignore the param?

I think we should error since we are meant to check the loaded changeset's descriptor against the provided descriptor.

Also refer to my comment #1537 (comment) which would (in my opinion) make our API communicate the "checking" part more clearer.

[notmandatory]

I also think we should error if LoadParams doesn't exactly match what's in the loaded file, including if the change_descriptor value in file not matching LoadParams as Some or None.

[evanlinjin]

I also think we should error if LoadParams doesn't exactly match what's in the loaded file, including if the change_descriptor value in file not matching LoadParams as Some or None.

I think we need to rethink the LoadParams constructor API if we want to do this. What if the caller does not want to check anything? What if the caller only wants to check the external descriptor (because they don't know what the internal descriptor is)?

pub fn check_descriptor<D>(mut self, keychain: KeychainKind, exp: Option<D>, extract_keys: bool) -> Self {
    todo!()
}

I think an API like this makes sense for what you are proposing. exp is what we expect the loaded changeset should have.

[ValuedMammal]

@evanlinjin I added your suggestion, happy to make further tweaks if anything is missing.

Edit: still needs work

[ValuedMammal]

I changed the extract_keys bool to a separate method on LoadParams that ensures we either extract keys for both keychains or for none of them

[notmandatory]

ACK 3951110

This turned out bigger than I expected but should be a good solution for the projects that need it.

[evanlinjin]

ACK 13e7008

[cryptoquick]

Nice! This looks great.