wallet: Properly support a wallet id

[achow101]

Adds a unique id for each wallet that is saved in a new "walletid" record. For compatibility, wallets using BDB will use the BDB generated id. All other wallets will have a randomly generated id if an id does not already exist.

Alternative to #20204

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #26008 (wallet: cache IsMine scriptPubKeys to improve performance of wallets with a lot of non-ranged descriptors by achow101)
• #25907 (wallet: rpc to add automatically generated descriptors by achow101)
• #25152 (refactor: Split util/system into exception, shell, and fs-specific files by Empact)
• #24914 (wallet: Load database records in a particular order by achow101)
• #23544 (rpc, wallet: addhdseed, infer seed when importing descriptor with xpub by Sjors)
• #23417 (wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager by achow101)
• #22341 (rpc: add getxpub by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[promag]

Concept ACK.

Can't we just set the id for sqlite wallets on creation? Or you want to handle wallets already created?

Edit: do we want to modify old wallets? CWallet could return BDB Id on the fly without persisting it. Later we would just persist it if the wallet is converted.

[achow101]

Can't we just set the id for sqlite wallets on creation? Or you want to handle wallets already created?

This needs to handle wallets already created.

Edit: do we want to modify old wallets? CWallet could return BDB Id on the fly without persisting it. Later we would just persist it if the wallet is converted.

It should be persisted as a wallet record. The BDB ID is purely because existing PRs use it as a wallet ID. But proper support for a wallet id means it needs to be a record.

Also, a record means that a wallet migrated with createfromdump will have the ID as a record correctly without having that command know about this stuff.

[promag]

@achow101 reasonable answers.

[luke-jr]

Is there a reason to limit it to 160-bit? Seems like to future-proof, 256-bit ids would be better. And even if we end up only generating 160-bit ids now, it would be nice to load longer ones so we don't have to break compatibility if we ever generate more...

[achow101]

The current BDB ID is 160 bits long. I don't think limiting to "just" 160 bits will be a problem. Why would we need to load longer ones?

[luke-jr]

Probably 160-bit is enough, but I can't predict the future. :)

[jonasschnelli]

utACK 234e467

[promag]

For compatibility, wallets using BDB will use the BDB generated id

@achow101 compatibility with what?

[luke-jr]

@promag With existing wallets. Right now, the only unique identifier they have is the BDB database id.

[luke-jr]

While I think a variable-length wallet id would be more future-proof, I don't know of a use case, and this doesn't strictly tie our hands in that regard yet, and it's a strict improvement, so utACK anyway.

[hebasto]

ACK 4b7b253, tested on Linux Mint 20 (x86_64) both bdb and sqlite wallets.

Verified that WALLETID key is written to a new sqlite wallet database.

[ryanofsky]

Reiterate weak concept NACK see #20204 (comment).

This change is a bad change. It's a perfect example of dumb, unreasoning, cargo-cult design (there is no design discussion here) copying a poorly thought BDB feature that directly lead to data corruption previously in #11429 and is a trap and footgun for the future. Merging this change now will encourage adding fragile assumptions to code (uniqueness of ids), and adding pointless and confusing restrictions around wallet usage (disallowing basic file operations like copies).

It's possible to put thought into design designs that affect data format. For example, There are explicit reasons subversion uses unique ids and git doesn't use them. There are advantages and disadvantages to different approaches.

In this case, if there is any reason to add wallets id later (maybe adding a rename detection heuristic, maybe trying to help out forensics or recovery tools), nothing prevents wallet ids from being added later. It seems the main things gained now by merging this PR would be:

• Extra code that no usage or test coverage
• Foot gun and expectation of future foot surgery

[laanwj]

As this is controversial and needs more discussion, I'm removing it from the 0.21 milestone. From discussion on IRC I get the idea this is a "nice to have" for some people so I don't agree there is any hurry on this.

[luke-jr]

This is a regression bugfix and a blocker for sqlite wallets. 0.21 should not be released without it.

[ryanofsky]

This is a regression bugfix and a blocker for sqlite wallets. 0.21 should not be released without it.

Can you give an example of something that would be broken, or a feature that would be more difficult to implement if this is not included in 0.21? This would be news to me and I don't know what it is referring to.

A UUID field (or any field that can ignored by previous releases) can be added at any time. Wallet code is intentionally written to ignore unrecognized rows and non-mandatory flags so things like this can be added.

We can disagree about whether features that depend on UUIDs are ideal or if there are better alternatives. But even if they are ideal and perfect in every way, there's no reason the UUID fields can't be added at the same time as features which use them.

[luke-jr]

Adding a UUID later would mean backups are missing it, and restoring from a backup could break features relying on it. Even if we ignore these risks, the UUID is itself a feature wallets have always had, that is now missing in sqlite wallets.

Adding it back now has literally zero downsides. Absolute worst case, we could just not use it.

Furthermore, not having it now also means features can't use it until those features are merged into Core, without risk of their wallet format diverging. Considering Core's history with refusing to be compatible with other software, that isn't a good risk to take, so I would actually have to disable such features in Knots when a sqlite wallet is used.

[ryanofsky]

Adding a UUID later would mean backups are missing it, and restoring from a backup could break features relying on it.

This isn't true. Unrecognized rows are not stripped from backups. The other comments about core and knots also do not seem to make sense. We shouldn't go back and forth on this endlessly, but I think the more specific and detailed you can be about problems you are concerned about, the more quickly we will be able to see the right thing to do.

[Sjors]

Being able to load and reference a wallet by ID is useful for external tools. E.g. Specter desktop identifies wallets by their path, which would break if a user renames or even moves it.

I would suggest adding a commit to this PR that exposes the wallet ID for getwalletinfo and allows using it with loadwallet and unloadwallet.

[promag]

I would suggest adding a commit to this PR that exposed the wallet ID for getwalletinfo and allows using it with loadwallet and unloadwallet.

@Sjors how could loadwallet load with the id?

[Sjors]

@promag it would look through all wallets in the path (like the GUI does when populating menu items). Though it depends on how much overhead there is in reading the ID out of the wallet files; that might be a bit much (we could document that if you have millions of wallet files for some reason, you should just load by file name).

[promag]

@Sjors IIRC listing wallets doesn't open the files. Actually wallet files are opened to read some magic bytes and infer the wallet file format.

But scanning all wallet files in the RPC seems fine to me.

[ryanofsky]

Sjors could you clarify what the broken case is: What steps does the user perform, and what is the unexpected behavior in this case and the expected behavior? It seems like it would be confusing if external software interacting with the bitcoin wallet tried to identify wallets differently than bitcoin wallet itself does. More generally, I think it would be confusing for any software to track wallets by IDs outside of user control, confuse backup wallets with the latest wallet, assume falsely that two wallets can't have same id, and be broken in various repeated ways from this assumption.

I do think it would be great to add a descriptive text field to wallet files (call it memo or comment or description and set it initially to the wallet name), which could help users identify wallets if they are copying around and renaming them.

I also think it would be great to list active descriptors in getwalletinfo (or just the active descriptor for the preferred output type, or the active master key for legacy wallets), so external software that knows about a particular descriptor or key can identify wallets it is able to work with.

[Sjors]

What steps does the user perform, and what is the unexpected behavior in this case and the expected behavior?

1. User configures a multisig wallet in Specter, which in turn calls createwallet and remembers the wallet by its path (e.g. specter/my_multisig)
2. User starts Bitcoin Core and Specter at some later date (which calls loadwallet):
   a) the wallet file wasn't moved or renamed: all works fine
   b) the wallet file was moved or renamed: Specter fails to load wallet, user has manually edit a config file to point Specter to the right wallet name (e.g. I ran into this because I didn't like its lower case naming convention)

The above is not end of the world stuff.

confuse backup wallets with the latest wallet, assume falsely that two wallets can't have same id

When calling loadwallet with an ID, we could either refuse or pick the most recent one if two are found.

external software that knows about a particular descriptor or key can identify wallets it is able to work with

I'm not opposed to this, but I don't know if external software should be tracking such detailed information. In case of Specter it does though, but all that redundancy is also a potential source of errors.

[ryanofsky]

What steps does the user perform, and what is the unexpected behavior in this case and the expected behavior?

1. User configures a multisig wallet in Specter, which in turn calls `createwallet` and remembers the wallet by its path (e.g. `specter/my_multisig`)

2. User starts Bitcoin Core and Specter at some later date (which calls `loadwallet`):
   a) the wallet file wasn't moved or renamed: all works fine
   b) the wallet file was moved or renamed: Specter fails to load wallet, user has manually edit a config file to point Specter to the right wallet name (e.g. I ran into this because I didn't like its lower case naming convention)

The above is not end of the world stuff.

Agreed nothing is end of the world stuff. But I think the proposed change to fix an understandable and already fixable renaming issue would create worse problems. Both our software and the external software would have to have new logic to decide how to respond if a wallet file is copied and two wallets have same id. If a user wants to reimport the same master key in a new wallet file (maybe they accidentally deleted the original, or thought the file had too much old transaction data, or a too many keys in the keypool), there is no way to get the new wallet file to work with external software because it tracking some uncontrollable UUID instead of the just opening the wallet file with the same name.

confuse backup wallets with the latest wallet, assume falsely that two wallets can't have same id

When calling loadwallet with an ID, we could either refuse or pick the most recent one if two are found.

Right, I'm saying adding this logic makes software more fragile, more complicated, and results in a worse UX.

external software that knows about a particular descriptor or key can identify wallets it is able to work with

I'm not opposed to this, but I don't know if external software should be tracking such detailed information. In case of Specter it does though, but all that redundancy is also a potential source of errors.

Yes, I'm sure there are situations where that would be the wrong thing to do, and I'm not familiar with the Specter. The case where it could be the right thing to do is if you have a key on a hardware wallet, and the software is managing that key, and it wants to find wallets on a bitcoind node associated with that key. But if the software is literally managing a particular bitcoin core wallet, then it should probably refer to that wallet the same way bitcoin core does.

[Sjors]

I agree there's something to be said for identifying wallets based on the descriptors (descriptor id) rather than the wallet id.

[ryanofsky]

I agree there's something to be said for identifying wallets based on the descriptors (descriptor id) rather than the wallet id.

From the description it seems like #23417 could be helpful for this, too, if it brings back the concept of an active master seed / key id.

[Sjors]

Not in the case of multisig wallets. The master seed at best refers to 1 of N master keys that make up a wallet. But a single master key may be used in multiple single / multi signature setups. So it doesn't make for a good unique identifier.