wallets created on master get corrupted when processed with v25

mzumsande commented at 1:37 am on June 20, 2023: member

Is there an existing issue for this?

I have searched the existing issues

Current behaviour

I randomly encountered this by loading a wallet created from master with an older version. I could trace it down to bd13dc2f46ea10302a928fcf0f53b7aed77ad260 (#26076), but I don’t know descriptor wallets enough to understand what’s going on (cc @Sjors @achow101).

Expected behaviour

bitcoind starts, or at least tells me to use a later version if #26076 was a breaking change instead of corrupting wallet.dat.

Steps to reproduce

(master, clean chain): createwallet -regtest "" (v25.0rc2): first bitcoind -regtest succeeds then stop and restart the node: The second bitcoind -regtest leads to an InitError: Error: Error: Unable to expand wallet descriptor from cache now, the wallet cannot be loaded by either version.

Relevant log output

No response

How did you obtain Bitcoin Core

Compiled from source

What version of Bitcoin Core are you using?

v25.0rc2 / master

Operating system and version

Ubuntu

Machine specifications

No response

mzumsande renamed this:
~~wallet: wallets created on master get corrupted when processed with v25~~
wallets created on master get corrupted when processed with v25
on Jun 20, 2023

MarcoFalke added this to the milestone 26.0 on Jun 20, 2023

MarcoFalke added the label Bug on Jun 20, 2023

MarcoFalke added the label Wallet on Jun 20, 2023

Sjors commented at 7:13 am on June 20, 2023: member

Does it also happen the other way around?

Nothing in that PR should change the key material, so the cache of derived keys should be the same too. I’ll investigate and add an explicit test for this case.

One place to look at would be the descriptor identifier, which is a hash of the string. We don’t touch the descriptor itself during upgrade, let alone during downgrade, but perhaps we accidentally broke how the identifier is calculated.

mzumsande commented at 2:31 pm on June 20, 2023: member

Does it also happen the other way around?

No, wallets created in 25.2 are loaded without problems in master for me.

furszy commented at 2:56 pm on June 20, 2023: member

@Sjors, the issue is indeed on the descriptor’s ID. The calculation differs between master and 25.2.

Can check it by applying this into the v25.2 branch and follow @mzumsande steps:

 0diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
 1--- a/src/wallet/walletdb.cpp	(revision 8996da626d68784aaaccd96d3459c805b578f759)
 2+++ b/src/wallet/walletdb.cpp	(date 1687271655072)
 3@@ -651,6 +651,9 @@
 4                 wss.m_descriptor_caches[id] = DescriptorCache();
 5             }
 6             pwallet->LoadDescriptorScriptPubKeyMan(id, desc);
 7+            const auto* spkm = pwallet->GetDescriptorScriptPubKeyMan(desc);
 8+            assert(id == spkm->GetID());
 9         } else if (strType == DBKeys::WALLETDESCRIPTORCACHE) {
10             bool parent = true;
11             uint256 desc_id;

It happens because the spkm writes the descriptor record after topping up the wallet.

The BIP32PubkeyProvider::ToString in v25.2 uses apostrophe (because it has no notion of the h) while in master the string is formatted with the h .

Because we cannot change the past, to stay compatible, we need to always use the apostrophe version for the spkm ID. Happy to hear yours thoughts before moving forward with the fix PR.

fanquake commented at 3:47 pm on June 20, 2023: member

Can someone clarify what version(s) we are talking about here. 25.2 doesn’t exist. There is currently “master”, 25.x, 25.0, 24.x, 24.1. etc

mzumsande commented at 3:48 pm on June 20, 2023: member

Sorry, I meant v25.0rc2

furszy commented at 3:53 pm on June 20, 2023: member

@mzumsande give #27920 a try please. It fixes the issue for me. Still, while in a first glance looks ok, will think more about it.

Sjors commented at 5:15 pm on June 20, 2023: member

Can’t we derive the ID from the literal string as stored in the wallet database?

Currently WalletDescriptor is parsed when read from the database and then ToString() is called when storing it. It seems less brittle if we add std::string m_raw_descriptor and hold on to the original there. Presumably that means ToString() is only used once at generation time.

This would however break if you downgrade ~and then upgrade again. That in turn could be fixed by detecting the error condition, recalculating the hash (and wiping or modifying the cache records).~

~In other words when loading a descriptor, if the hash is wrong, try replacing ' with h and see if that works. If so, modify the record.~ (<- I don’t think this works, but leaving the thought)

There’s a similar downgrade - upgrade breakage introduced by #26728, although it’s much harder to hit (you have to downgrade, encrypt and then upgrade). That PR introduces a new flag which is currently optional, but it’s an option (though not ideal) to make it mandatory. Thereby preventing a downgrade.

Sjors commented at 5:34 pm on June 20, 2023: member

@achow101 wrote in the PR:

Once we assign the id, we should just store it and remember what it is rather than regenerating it whenever we want it.

This seems better than my suggestion above to store the raw string.

furszy commented at 11:07 pm on June 20, 2023: member

achow101 wrote in the PR:

Once we assign the id, we should just store it and remember what it is rather than regenerating it whenever we want it.

This seems better than my suggestion above to store the raw string.

That would work for newer versions, not for the previous ones. We can’t do much there. Old releases will still read the descriptor, and calculate the id by hashing the apostrophe formatted descriptor string.

The main point here is that every single descriptor record in the wallet database is keyed by the descriptor id. So.. if we change the spkm id calculation in newer versions, old software versions will not be able to connect the descriptor entries: keys, caches, etc. with the descriptor itself. And, even worst, after finish the loading process, the wallet will store a new descriptor entry (because the wallet re-writes the descriptor post TopUp()), keyed by the hash of the apostrophe formatted descriptor string… which conflicts with the string inside the descriptor, which is what causes this unrecoverable state.

Overall, I don’t think that there is a good workaround for the downgrading topic. We cannot change the past, unless we make people migrate to a newer database format, or.. we store records duplicated in the db.. with the two possible ids.. but that would be the dirties thing ever.

So, I think that we will have to live with the apostrophe formatted descriptor string IDs for now. Which is not ideal, but.. it isn’t really an issue anyway.

Sjors commented at 10:26 am on June 22, 2023: member

Living with apostrophe is one option, but the other is to prevent downgrades. This can be done by making WALLET_FLAG_GLOBAL_HD_KEY mandatory, see https://github.com/bitcoin/bitcoin/pull/26728/files#r1081289252. That only works if that PR lands before the 26 branch-off. We can leave this PR open in case #26728 doesn’t make it in time.

The third option is to revert #26076 which would be my least favourite.

furszy commented at 1:16 pm on June 22, 2023: member

Living with apostrophe is one option, but the other is to prevent downgrades.

Haven’t seen #26728 yet but preventing downgrades on descriptors wallets sounds bad for me. Users already face enough difficulties with their legacy wallets and the migration process for now. We should try to avoid breaking the descriptors wallet compatibility now too.

Furthermore, I believe this isn’t a major problem really, we just had a bad design decision by making the software calculate the db key on-demand, but it is not the end of the world. We can stick to the current implementation and clearly state that descriptor IDs are calculated by hashing the apostrophe-formatted descriptor string. And, in the future, if we ever want to replace the db key for something better, we can take the opportunity to propose a better overall database format and implement the changes all at once.

Sjors commented at 1:23 pm on June 22, 2023: member

I’m still worried that something like this will happen again because it’s so brittle. E.g. if we change something in miniscript.

furszy commented at 2:58 pm on June 22, 2023: member

I’m still worried that something like this will happen again because it’s so brittle. E.g. if we change something in miniscript.

You mean changing the string representation of a miniscript output descriptor between releases will cause the same issue that we are having here with the hd path. Hmm, that opens a bigger can of worms. Good that we are now aware of it and changes there can be treated carefully.

Maybe not for the next release (because I think that we are not planning to introduce new miniscript changes for now) but.. the incompatibility idea started getting more traction on me too.. this might also suit well for a new record type and an automatic migration process.

Will keep thinking about it.

At least with #27920, and as long as we don’t introduce changes in the descriptor string representation, we gain time to figure out the best way to solve this.

luke-jr commented at 2:33 pm on June 24, 2023: member

Maybe we should consider it an error to load a wallet if the descriptors can’t all be round-tripped? (and consider it a bugfix for 25.x, while also trying to avoid triggering it for a while)

furszy commented at 2:39 pm on June 24, 2023: member

Maybe we should consider it an error to load a wallet if the descriptors can’t all be round-tripped? (and consider it a bugfix for 25.x, while also trying to avoid triggering it for a while)

Yeah, that is already done in #27920 (fc5565eb). It will prevent undesirable events from happening from now on.

Sjors commented at 2:46 pm on June 26, 2023: member

I’ll review the apostropocalypse fix soon, thanks.

achow101 closed this on Jul 4, 2023

wallets created on master get corrupted when processed with v25 #27915