wallet: Properly support a wallet id

achow101 commented at 8:06 pm on October 20, 2020: member

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 added the label Wallet on Oct 20, 2020

DrahtBot commented at 1:20 am on October 21, 2020: contributor

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.

in src/wallet/wallet.cpp:4138 in 545c362e78 outdated

4580@@ -4581,3 +4581,20 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat
4581 
4582     return ret;
4583 }
4584+
4585+void CWallet::LoadWalletID(const uint160& id)
4586+{
4587+    wallet_id = id;

promag commented at 11:18 pm on October 21, 2020:

Assert it’s currently null? (set only once)

achow101 commented at 2:24 am on October 22, 2020:

Done

in src/wallet/wallet.cpp:4144 in 545c362e78 outdated

4587+    wallet_id = id;
4588+}
4589+
4590+const uint160& CWallet::GetWalletID()
4591+{
4592+    return wallet_id;

promag commented at 11:27 pm on October 21, 2020:

Assert it’s not null.

achow101 commented at 2:24 am on October 22, 2020:

Done

in src/wallet/bdb.cpp:368 in 545c362e78 outdated

363@@ -363,6 +364,13 @@ void BerkeleyDatabase::Open()
364 
365             m_db.reset(pdb_temp.release());
366 
367+            auto batch = MakeBatch();
368+            if (!batch->Exists(std::string("walletid"))) {

promag commented at 11:29 pm on October 21, 2020:

I think this is in the wrong place. From a quick look CWallet::LoadWallet seems a better place, like:

0    if (wallet_id.IsNull()) {
1        if (database->Format() == "dbd") {
2             // use BDB ID
3        } else {
4             // generate random id
5        }
6        // persist
7    }
8    return DBErrors::LOAD_OK;
9}

This would allow you to drop EnsureWalletIDWithDB. Only bad thing here is to compare agains “bdb”. Would be better to add Format() and FormatString().

achow101 commented at 2:24 am on October 22, 2020:

I’ve moved all of the logic into EnsureWalletIDWithDB.

in src/wallet/wallet.h:1290 in 545c362e78 outdated

1282@@ -1281,6 +1283,13 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
1283 
1284     //! Add a descriptor to the wallet, return a ScriptPubKeyMan & associated output type
1285     ScriptPubKeyMan* AddWalletDescriptor(WalletDescriptor& desc, const FlatSigningProvider& signing_provider, const std::string& label);
1286+
1287+    //! Set the wallet ID
1288+    void LoadWalletID(const uint160& id);
1289+    //! Get the wallet iD
1290+    const uint160& GetWalletID();

promag commented at 11:30 pm on October 21, 2020:

const.

achow101 commented at 2:30 am on October 22, 2020:

Done

promag commented at 11:35 pm on October 21, 2020: member

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 commented at 11:43 pm on October 21, 2020: member

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 commented at 11:45 pm on October 21, 2020: member

@achow101 reasonable answers.

achow101 force-pushed on Oct 22, 2020

in src/wallet/wallet.cpp:4135 in 234e4678c7 outdated

4580@@ -4581,3 +4581,31 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat
4581 
4582     return ret;
4583 }
4584+
4585+void CWallet::LoadWalletID(const uint160& id)

luke-jr commented at 12:33 pm on October 22, 2020:

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 commented at 3:48 pm on October 22, 2020:

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 commented at 5:16 pm on October 22, 2020:

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

in src/wallet/wallet.cpp:4779 in 234e4678c7 outdated

4600+
4601+    // No ID, so add one
4602+    auto bdb_db = dynamic_cast<BerkeleyDatabase*>(database.get());
4603+    if (bdb_db) {
4604+        // For BDB, use the BDB unique ID
4605+        memcpy(wallet_id.data(), bdb_db->env->m_fileids[bdb_db->strFile].value, wallet_id.size());

luke-jr commented at 12:35 pm on October 22, 2020:

Maybe an assert that wallet_id.size() == sizeof(bdb_db->env->m_fileids[bdb_db->strFile].value)? (or use the latter and assert that it’s <= wallet_id.size())

achow101 commented at 4:46 pm on October 22, 2020:

Done

luke-jr changes_requested

jonasschnelli commented at 12:42 pm on October 22, 2020: contributor

utACK 234e4678c774aa0021a45810acd9a0f3f5bf3b64

promag commented at 12:52 pm on October 22, 2020: member

For compatibility, wallets using BDB will use the BDB generated id @achow101 compatibility with what?

achow101 force-pushed on Oct 22, 2020

luke-jr commented at 5:15 pm on October 22, 2020: member

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

laanwj added this to the milestone 0.21.0 on Oct 22, 2020

luke-jr approved

luke-jr commented at 2:30 am on October 24, 2020: member

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 approved

hebasto commented at 12:08 pm on October 26, 2020: member

ACK 4b7b2532e42f10b8f695d3800bebff88b586550c, 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 commented at 1:26 pm on October 26, 2020: contributor

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 removed this from the milestone 0.21.0 on Oct 29, 2020

laanwj commented at 10:43 am on October 29, 2020: member

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 commented at 12:54 pm on October 29, 2020: member

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

ryanofsky commented at 1:09 pm on October 29, 2020: contributor

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 commented at 1:30 pm on October 29, 2020: member

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 commented at 1:39 pm on October 29, 2020: contributor

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.

luke-jr commented at 3:01 pm on October 29, 2020: member

The UUID isn’t simply unrecognised, it’s ENTIRELY MISSING in sqlite wallets right now.

ryanofsky commented at 3:23 pm on October 29, 2020: contributor

The UUID isn’t simply unrecognised, it’s ENTIRELY MISSING in sqlite wallets right now.

I know this. You know this. What I don’t know is what specific problem is fixed by merging this PR now that would not be fixed by merging it later (or perhaps by never relying on uuids at all, but I don’t want to broaden the topic if it is not necessary). I’m encouraging you to describe an example in some level of detail so we can actually think about it.

luke-jr commented at 4:10 pm on October 29, 2020: member

Merging it later won’t retroactively ensure wallets created by 0.21 and then copied share a common UUID (which is kinda the whole point of a UUID).

Merging it later also means features using the UUID won’t work on sqlite wallets in the meantime.

jonasschnelli added this to the milestone 0.21.0 on Oct 29, 2020

jonasschnelli removed this from the milestone 0.21.0 on Nov 5, 2020

jonasschnelli commented at 7:12 pm on November 5, 2020: contributor

Removed the 0.21 milestone again. This seems indeed be too controversial for a last minute 0.21 merge.

sipa commented at 7:31 pm on November 5, 2020: member

“It’s a feature of existing wallets, new wallets should have it too” is not a good argument. It’s a new wallet type, it will support whatever features we declare it to have.
Wallet IDs were originally introduced in BDB because of database environments being unable to open the same database multiple times. Restricting the ability to open multiple copies simultaneously is a necessity here, not a feature. There is no point in maintaining it if not required by the database layer.
However, I believe there are reasons why wallet ids are useful, independent of the argument above: Bitcoin Core wallets are intended to have a linear history. While we take lots of effort to make sure using a backup is won’t cause monetary loss, this isn’t perfect and doesn’t apply to all data in the wallet. Adding address labels, which may matter for accounting reasons, in one copy won’t make them show up in another. Creating two transactions from two versions of the same wallet ~simultaneously (or while one hasn’t caught up with the chain) may result in unintentionally creating conflicting transactions. Because of that, I think it makes conceptual sense to have an ID, and use that to warn (not reject) when opening multiple copies.
I don’t think the above is urgent, or necessary, and I think it should be treated as a new feature. It’s trivial to add IDs though, and having it earlier means features depending on it can be introduced later without needing a new backup.

Concept ACK, and I’m ok with having it in 0.21, but adding wallet IDs whenever a feature actually using them is introduced is fine by me as well.

achow101 commented at 7:40 pm on November 5, 2020: member

Given that descriptor (and thus sqlite) wallets are considered experimental, I think it’s find to miss the id for 0.21. I think it’s reasonable to ask users make a new backup when we no longer consider descriptor wallets to be experimental, and we can introduce the wallet id before that time.

DrahtBot added the label Needs rebase on Nov 9, 2020

achow101 force-pushed on Nov 9, 2020

DrahtBot removed the label Needs rebase on Nov 9, 2020

hebasto approved

hebasto commented at 7:08 pm on November 9, 2020: member

re-ACK 30a02e9d60af542003b333996e658f982ceb2d9a, only rebased since my previous review, verified with git range-diff master 4b7b253 30a02e9.

ryanofsky commented at 7:46 pm on December 15, 2020: contributor

re: #20205 (comment)

Because of that, I think it makes conceptual sense to have an ID, and use that to warn (not reject) when opening multiple copies.

Without knowing who the warning would be for or what it would say, it doesn’t seem clear to me how it would be useful. But assuming it is useful, wouldn’t the warning be more robust if it checked directly for duplicate seeds and descriptors, instead of going indirectly through this PR, adding a new random number to sqlite databases and checking for uniqueness based on that?

I still do stand by my NACK from #20205 (comment), and I don’t think this code should be merged until there is an actual, real life, properly designed feature which depends on it. I think it is possible to build useful heuristics (usage warnings, rename detection) with UUIDs. I also think it’s possible to build misfeatures that make software painful to work with (and even cause data corruption, see #11429) because of bad and fragile assumptions about uniqueness. So adding this dead code to the wallet which complicates the already messy database layer and exposes a new API which is asking to be misused does not seem like a win to me right now. The harms outweigh the benefits currently, because there aren’t benefits currently.

luke-jr commented at 7:59 pm on December 15, 2020: member

@ryanofsky There already is, why are you ignoring it? #19463

ryanofsky commented at 8:24 pm on December 15, 2020: contributor

@ryanofsky There already is, why are you ignoring it? #19463

I’m aware and not ignoring it. If #19463 is close to being merged and it depends on this PR, then this PR should be merged. But this isn’t the case now. #19463 isn’t close to being merged, and I don’t think it makes sense for #19463 to depend on this PR. I’m just objecting to this PR on its merits, and its harms and benefits right now.

DrahtBot added the label Needs rebase on May 19, 2021

achow101 force-pushed on May 19, 2021

DrahtBot removed the label Needs rebase on May 19, 2021

DrahtBot added the label Needs rebase on Jun 9, 2021

achow101 force-pushed on Jun 9, 2021

DrahtBot removed the label Needs rebase on Jun 9, 2021

hebasto commented at 7:39 pm on June 11, 2021: member

CI error:

0In file included from wallet/wallet.cpp:34:
1./wallet/bdb.h:27:10: fatal error: 'db_cxx.h' file not found
2#include <db_cxx.h>
3         ^~~~~~~~~~
41 error generated.

DrahtBot added the label Needs rebase on Sep 3, 2021

ghost commented at 11:39 am on September 3, 2021: none

Concept ACK

Can we add 0/1/2/3 for every ID based on chain the wallet was created in? 0-Mainnet 1-Testnet 2-Regtest 3-Signet

Chains for wallets can already be identified using genesis block and this is used in #14533, however genesis block can be same in few cases as mentioned in #14533 (comment), we can use this ID to differentiate for wallets created in future with 00/01/02/03.

achow101 force-pushed on Sep 3, 2021

DrahtBot removed the label Needs rebase on Sep 3, 2021

DrahtBot added the label Needs rebase on Sep 7, 2021

achow101 force-pushed on Sep 7, 2021

DrahtBot removed the label Needs rebase on Sep 7, 2021

DrahtBot added the label Needs rebase on Sep 26, 2021

achow101 force-pushed on Sep 27, 2021

DrahtBot removed the label Needs rebase on Sep 27, 2021

maflcko commented at 10:52 am on December 3, 2021: member

 0  CXX      wallet/libbitcoin_wallet_a-wallet.o
 1In file included from wallet/wallet.cpp:35:
 2./wallet/bdb.h:27:10: fatal error: 'db_cxx.h' file not found
 3#include <db_cxx.h>
 4         ^~~~~~~~~~
 51 error generated.
 6make[2]: *** [Makefile:10408: wallet/libbitcoin_wallet_a-wallet.o] Error 1
 7make[2]: *** Waiting for unfinished jobs....
 8make[2]: Leaving directory '/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
 9make[1]: *** [Makefile:16267: install-recursive] Error 1
10make[1]: Leaving directory '/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
11make: *** [Makefile:821: install-recursive] Error 1
12Build failure. Verbose build follows.
13Making install in src
14make[1]: Entering directory '/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
15make[2]: Entering directory '/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
16rm -f libbitcoin_server.a
17ar cr libbitcoin_server.a libbitcoin_server_a-addrdb.o libbitcoin_server_a-addrman.o libbitcoin_server_a-banman.o libbitcoin_server_a-blockencodings.o libbitcoin_server_a-blockfilter.o libbitcoin_server_a-chain.o consensus/libbitcoin_server_a-tx_verify.o libbitcoin_server_a-dbwrapper.o libbitcoin_server_a-deploymentstatus.o libbitcoin_server_a-flatfile.o libbitcoin_server_a-httprpc.o libbitcoin_server_a-httpserver.o libbitcoin_server_a-i2p.o index/libbitcoin_server_a-base.o index/libbitcoin_server_a-blockfilterindex.o index/libbitcoin_server_a-coinstatsindex.o index/libbitcoin_server_a-txindex.o libbitcoin_server_a-init.o libbitcoin_server_a-mapport.o libbitcoin_server_a-miner.o libbitcoin_server_a-net.o libbitcoin_server_a-net_processing.o node/libbitcoin_server_a-blockstorage.o node/libbitcoin_server_a-coin.o node/libbitcoin_server_a-coinstats.o node/libbitcoin_server_a-context.o node/libbitcoin_server_a-interfaces.o node/libbitcoin_server_a-psbt.o node/libbitcoin_server_a-transaction.o node/libbitcoin_server_a-ui_interface.o libbitcoin_server_a-noui.o policy/libbitcoin_server_a-fees.o policy/libbitcoin_server_a-packages.o policy/libbitcoin_server_a-rbf.o policy/libbitcoin_server_a-settings.o libbitcoin_server_a-pow.o libbitcoin_server_a-rest.o rpc/libbitcoin_server_a-blockchain.o rpc/libbitcoin_server_a-mining.o rpc/libbitcoin_server_a-misc.o rpc/libbitcoin_server_a-net.o rpc/libbitcoin_server_a-rawtransaction.o rpc/libbitcoin_server_a-server.o script/libbitcoin_server_a-sigcache.o libbitcoin_server_a-shutdown.o libbitcoin_server_a-signet.o libbitcoin_server_a-timedata.o libbitcoin_server_a-torcontrol.o libbitcoin_server_a-txdb.o libbitcoin_server_a-txmempool.o libbitcoin_server_a-txorphanage.o libbitcoin_server_a-txrequest.o libbitcoin_server_a-validation.o libbitcoin_server_a-validationinterface.o libbitcoin_server_a-versionbits.o  wallet/libbitcoin_server_a-init.o  
18ranlib libbitcoin_server.a
19/usr/bin/ccache clang++ -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config   -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I. -I./secp256k1/include  -isystem /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include -I./leveldb/include -I./leveldb/helpers/memenv -I./univalue/include -I/tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include -I/tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -DPROVIDE_FUZZ_MAIN_FUNCTION -fdebug-prefix-map=/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src=. -Wstack-protector -fstack-protector-all -fcf-protection=full   -Werror=gnu -Werror=vla -Werror=shadow-field -Werror=switch -Werror=thread-safety -Werror=range-loop-analysis -Werror=unused-variable -Werror=date-time -Werror=return-type -Werror=conditional-uninitialized -Werror=sign-compare -Werror=unreachable-code-loop-increment -Werror=mismatched-tags -Werror=implicit-fallthrough -Werror=documentation  -fsanitize=memory -fPIE -pipe -O2 -fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls -nostdinc++ -stdlib=libc++ -L/tmp/cirrus-ci-build/ci/scratch/msan/build/lib -lc++abi -I/tmp/cirrus-ci-build/ci/scratch/msan/build/include -I/tmp/cirrus-ci-build/ci/scratch/msan/build/include/c++/v1 -lpthread -Wl,-rpath,/tmp/cirrus-ci-build/ci/scratch/msan/build/lib -Wno-unused-command-line-argument -c -o wallet/libbitcoin_wallet_a-wallet.o `test -f 'wallet/wallet.cpp' || echo './'`wallet/wallet.cpp
20In file included from wallet/wallet.cpp:35:
21./wallet/bdb.h:27:10: fatal error: 'db_cxx.h' file not found
22#include <db_cxx.h>
23         ^~~~~~~~~~
241 error generated.
25make[2]: *** [Makefile:10408: wallet/libbitcoin_wallet_a-wallet.o] Error 1
26make[2]: Leaving directory '/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
27make[1]: *** [Makefile:16267: install-recursive] Error 1
28make[1]: Leaving directory '/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
29make: *** [Makefile:821: install-recursive] Error 1

achow101 force-pushed on Dec 8, 2021

achow101 force-pushed on Dec 17, 2021

DrahtBot added the label Needs rebase on Jan 11, 2022

achow101 force-pushed on Jan 11, 2022

DrahtBot removed the label Needs rebase on Jan 11, 2022

Sjors commented at 10:21 am on January 12, 2022: member

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 commented at 10:23 am on January 12, 2022: member

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 commented at 10:25 am on January 12, 2022: member

@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 commented at 10:28 am on January 12, 2022: member

@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 commented at 3:45 pm on January 12, 2022: contributor

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 commented at 4:05 pm on January 12, 2022: member

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

User configures a multisig wallet in Specter, which in turn calls createwallet and remembers the wallet by its path (e.g. specter/my_multisig)
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 commented at 4:31 pm on January 12, 2022: contributor

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 commented at 6:30 pm on January 12, 2022: member

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

ryanofsky commented at 6:34 pm on January 12, 2022: contributor

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 commented at 6:38 pm on January 12, 2022: member

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.

DrahtBot added the label Needs rebase on Mar 24, 2022

achow101 force-pushed on Mar 24, 2022

DrahtBot removed the label Needs rebase on Mar 24, 2022

DrahtBot added the label Needs rebase on Sep 1, 2022

achow101 force-pushed on Sep 1, 2022

bitcoin deleted a comment on Sep 1, 2022

DrahtBot removed the label Needs rebase on Sep 1, 2022

wallet: Properly support a wallet id

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.

4a3c22f7d8

achow101 force-pushed on Sep 20, 2022

achow101 closed this on Oct 12, 2022

bitcoin locked this on Oct 12, 2023

wallet: Properly support a wallet id #20205

Conflicts