wallet: bugfix, always use apostrophe for spkm descriptor ID #27920

furszy · 2023-06-20T15:51:01Z

Aiming to fix #27915.

As we re-write the descriptor's db record every time that
the wallet is loaded (at TopUp time), if the spkm ID differs
from the one in db, the wallet will enter in an unrecoverable
corruption state (due to the storage of a descriptor with an ID
that is not linked to any other descriptor record in DB), and
no soft version will be able to open it anymore.

Because we cannot change the past, to stay compatible between
releases, we need to always use the apostrophe version for the
spkm IDs.

DrahtBot · 2023-06-20T15:51:03Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, Sjors
Concept ACK	ErikDeSmedt

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27255 (MiniTapscript: port Miniscript to Tapscript by darosior)
#26567 (Wallet: estimate the size of signed inputs using descriptors by darosior)
#25907 (wallet: rpc to add automatically generated descriptors by achow101)

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.

mzumsande

Can confirm that this fixes the problem from #27915, I'm not familiar with the wallet though, so I can't comment on the code changes.

achow101 · 2023-06-20T17:26:16Z

Having the id be dependent on ToString being a round trip seems kind of bleh. I think it would be better to change the id calculation to just use the string written to/read from the db.

Edit: Actually, how the id itself is generated shouldn't matter. It might as well be random. Once we assign the id, we should just store it and remember what it is rather than regenerating it whenever we want it. But that doesn't really solve this downgrading problem.

src/wallet/walletdb.cpp

src/script/descriptor.h

Sjors

One way to reduce the odds of this happening again with future descriptor changes is to hardcode the COMPAT identifier for all the test vectors in descriptor_tests.cpp.

src/wallet/walletdb.cpp

Sjors · 2023-06-22T15:05:24Z

I wrote a commit on top of this branch that moves GetID() into the Descriptor class and then adds test vectors for it. The function documentation clarifies that this ID is not part of BIP 380 and shouldn't be exposed to the user.

https://github.com/Sjors/bitcoin/tree/2023/06/desc_id_vectors

furszy · 2023-06-22T19:47:59Z

I wrote a commit on top of this branch that moves GetID() into the Descriptor class and then adds test vectors for it. The function documentation clarifies that this ID is not part of BIP 380 and shouldn't be exposed to the user.

https://github.com/Sjors/bitcoin/tree/2023/06/desc_id_vectors

You know, I initially wrote this with the GetID() method as well, check #27920 (comment).

Also, left you a comment on your commit. Not sure why added the GetID() method to the pub key provider, they are always returning an empty uint256 and they don't seems to be used anywhere (can remove them, compile the sources, run the descriptor_tests and it passes there).

Will go deeper over your commit. Thanks!

One way to reduce the odds of this happening again with future descriptor changes is to hardcode the COMPAT identifier for all the test vectors in descriptor_tests.cpp.

Good call 👌🏼.

furszy · 2023-06-23T20:26:46Z

Updated per feedback, thanks @Sjors.

Pulled your test commit and made some changes. While reviewing it, discovered that descriptors containing a key with an origin are actually incorrect. As the origin consists of a root xpub fingerprint and the key derivation path, this former needs to be formatted with an apostrophe as well. So, have updated the code and tests to address it. And also expanded the coverage for few other descriptors that were previously not included.

Then, to facilitate the verification of the test vectors, have created two isolated commits, ab3b700 and 9e1c3b4. These can be applied on top of the v25 branch. The cherry-pick process is not entirely clean, but once it's done, you can easily verify the correctness of the descriptors' IDs.
The tests must pass in v25 with these commits, just as they pass here with the bug fix.

To simplify the process of verifying these test vectors, have pushed a v25 branch that includes only those two commits: https://github.com/furszy/bitcoin-core/tree/25.x_verify_spkm_ids.

Also, have separated the descriptor ID function as a standalone function to avoid mixing it with the descriptor's interface.

Sjors · 2023-06-26T14:43:27Z

While reviewing it, discovered that descriptors containing a key with an origin are actually incorrect.

Glad the tests were useful :-)

The tests must pass in v25 with these commits.

Thanks, I was thinking of testing that as well, but couldn't figure out how to do so trivially with my own commit.

If the computed descriptor's ID doesn't match the wallet's DB spkm ID, return early from the loading process to prevent DB data from being modified in any post-loading procedure (e.g 'TopUp' updates the descriptor's data).

This allows us to verify the descriptor ID on the descriptors unit tests in different software versions without requiring to use the entire DescriptorScriptPubKeyMan machinery. Note: The unit test changes are introduced after the bugfix commit but this commit + the unit test commit can be cherry-picked on top of the v25 branch to verify IDs correctness. IDs must be the same for v25 and after the bugfix commit.

As we update the descriptor's db record every time that the wallet is loaded (at `TopUp` time), if the spkm ID differs from the one in db, the wallet will enter in an unrecoverable corruption state, and no soft version will be able to open it anymore. Because we cannot change the past, to stay compatible between releases, we need to always use the apostrophe version for the spkm IDs.

Tests vectors were calculated by running the same tests on v25. Which was the last release prior to introducing the diff in the descriptor's string representation ('h' format). Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>

Sjors · 2023-06-28T13:34:14Z

What happened to fa23292?

furszy · 2023-06-28T14:08:20Z

What happened to fa23292?

Vanished after #24914 merge :).

We no longer iterate over all the database records without ordering. Now the wallet loads each specific group of records separately (e.g. legacy records, descriptors records, txs, etc).

The reason behind fa23292 was that the upper level method LoadWallet was only treating few specific ReadKeyValue errors as corruption ones (we were adding each of them manually). And now, each specific section of the wallet loading process can return a corruption error alone (check 405b4d9).

ErikDeSmedt · 2023-06-28T17:12:50Z

ACK: I've tested the changes and the bug is resolved.

I've been struggling with back-porting the test-code to v25.0.
You might need to change some descriptors to use h instead of ' and I manually removed some checksums.

@Sjors : Thanks for helping me out with this

achow101 · 2023-06-28T18:57:45Z

ACK 5df988b

Sjors · 2023-07-03T19:30:08Z

tACK 5df988b

Also tested backporting the tests. Trick is to disable the norm_pub check (or manually replacing ' with h, changing the checksum where needed).

luke-jr · 2023-07-04T16:41:18Z

src/wallet/walletdb.cpp

+        // Prior to doing anything with this spkm, verify ID compatibility
+        if (id != pwallet->GetDescriptorScriptPubKeyMan(desc)->GetID()) {
+            strErr = "The descriptor ID calculated by the wallet differs from the one in DB";
+            return DBErrors::CORRUPT;


Why not UNKNOWN_DESCRIPTOR instead?

The descriptor isn't unknown. It's a corruption error as the id written in the database doesn't match the id that we calculate.

…escript…

luke-jr · 2023-07-04T16:45:29Z

Tag for backport?

fanquake · 2023-07-04T16:48:41Z

Not if this has (as I understand it) only been broken on master?

achow101 · 2023-07-04T16:52:38Z

Tag for backport?

No need, only affects master.

Sjors · 2023-07-04T18:04:48Z

If anyone on master is affected by this, we could perhaps add a command to wallet-tool that resets the descriptor ID and cache.

denavila · 2023-09-03T00:57:43Z

Some of my wallets are no longer opening after pulling master, and I bisected it to this commit.
For example when starting in -sigtest or -regtest mode I get the error "The wallet is corrupt" and Bitcoin Core exits.
This doesn't happen for -testnet or main net though.
If I understand correctly, this PR is fixing a bug for people who got their wallets corrupted, but in my case it's the opposite - I can still open the wallets with versions before this commit.
Is this an expected behavior?

sipa · 2023-09-03T04:20:19Z

@denavila My understanding is that if you used non-release code from before this PR, then this PR will be expected to break things for you. However if you only ever run released version, this PR prevents incompatibilities.

denavila · 2023-09-03T16:20:55Z

Yes, I was using non-release versions for these wallets.
I tried to restore the signet wallet from a backup, but that also hits the same "corruption" error. Is there any other way to migrate the signet wallet to the latest code?
It's signet, so it's not the end of the world if the coins are lost, but it would still be better if I could recover them.

furszy

Yes, I was using non-release versions for these wallets.
I tried to restore the signet wallet from a backup, but that also hits the same "corruption" error. Is there any other way to migrate the signet wallet to the latest code?
It's signet, so it's not the end of the world if the coins are lost, but it would still be better if I could recover them.

Probably the easiest workaround would be to revert this PR on your local repository. Compile the sources and start the app. Once you have the wallet loaded, export the descriptors by calling listdescriptors true (showing the private keys). Then, create a new wallet on top of latest master and import the exported descriptors there.

denavila · 2023-09-03T17:17:55Z

@furszy, thanks for the workaround! I got my signet wallet restored :-)

Sjors · 2023-09-04T14:38:01Z

Compile the sources and start the app.

~~Or even just run an earlier release and export the descriptors.~~ Glad that worked!

furszy · 2023-09-04T14:49:19Z

Compile the sources and start the app.

Or even just run an earlier release and export the descriptors. Glad that worked!

That wouldn't work. Remember that this fixed an unrecoverable corruption state
that was arising when the 'h' created wallet was open by an earlier release.

The previous release would overwrite the spkm ID with the non-'h' version of it.
Making all the db records tied to it fail to load on the next startup.

…r spkm descriptor ID BACKPORT NOTICE It includes only this commit: 97a965d refactor: extract descriptor ID calculation from spkm GetID() This allows us to verify the descriptor ID on the descriptors unit tests in different software versions without requiring to use the entire DescriptorScriptPubKeyMan machinery. Note: The unit test changes are introduced after the bugfix commit but this commit + the unit test commit can be cherry-picked on top of the v25 branch to verify IDs correctness. IDs must be the same for v25 and after the bugfix commit.

…29007 - to improve CI experience 2eadcf2 partial Merge bitcoin#29007: test: create deterministic addrman in the functional tests (stratospher) f95ca4e fix: unify with bitcoin: removed requirement of txindex=0 (Konstantin Akimov) bf46e7a partial Merge bitcoin#28799: wallet: cache descriptor ID to avoid repeated descriptor string creation (Andrew Chow) 05e5966 partial Merge bitcoin#27920: wallet: bugfix, always use apostrophe for spkm descriptor ID (furszy) Pull request description: ## Issue being fixed or feature implemented Lately our CI for tsan is flapping many functional tests and take long times. This PR has several important changes backported from the latest bitcoin's version to improve CI experience ## What was done? This PR has several backports that improved CI experience drastically. **Firstly, it aims to fix flapping test p2p_node_network_limited.py** For example: https://gitlab.com/dashpay/dash/-/jobs/7692635307 <details> <summary>p2p_node_network_limited.py | ✖ Failed | 28 s</summary> ``` test 2024-08-29T02:50:53.929000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/test_framework/test_framework.py", line 158, in main self.run_test() File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/p2p_node_network_limited.py", line 79, in run_test self.nodes[0].disconnect_p2ps() File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/test_framework/test_node.py", line 611, in disconnect_p2ps wait_until_helper(check_peers, timeout=5) File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/test_framework/util.py", line 262, in wait_until_helper raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout)) AssertionError: Predicate '''' def check_peers(): for p in self.getpeerinfo(): for p2p in self.p2ps: if p['subver'] == p2p.strSubVer: return False return True ''' not true after 5.0 seconds ``` </details> **Secondly, it improves performance of Descriptor wallets significantly for case of `tsan` CI**. It is tiny improvement for Release build and local runs, but some fucnctional tests run as fast as twice: https://gitlab.com/dashpay/dash/-/jobs/7694458953 https://gitlab.com/dashpay/dash/-/jobs/7665132625 ``` wallet_create_tx.py --descriptors | ✓ Passed | 236 s <-- new version wallet_create_tx.py --legacy-wallet | ✓ Passed | 108 s wallet_basic.py --descriptors | ✓ Passed | 135 s <---- new version wallet_basic.py --legacy-wallet | ✓ Passed | 97 s wallet_create_tx.py --descriptors | ✓ Passed | 456 s <-- old version wallet_create_tx.py --legacy-wallet | ✓ Passed | 98 s wallet_basic.py --descriptors | ✓ Passed | 189 s <--- old version wallet_basic.py --legacy-wallet | ✓ Passed | 131 s ``` See performance investigation here: #6226 ## How Has This Been Tested? Run unit/functional tests ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK 2eadcf2 UdjinM6: utACK 2eadcf2 Tree-SHA512: 127fbaa65c160aa95e2145a6b40d3811f7c42e36fbee9ce98a9ac021abd9cbe6edc7791870b331a54855ba891e3804885db7936ef212647b693f50f79a60d232

DrahtBot added the Wallet label Jun 20, 2023

furszy mentioned this pull request Jun 20, 2023

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

Closed

1 task

furszy force-pushed the 2023_wallet_fix_spkm_id_corruption branch from 6b51f24 to 3a38f7f Compare June 20, 2023 16:03

mzumsande reviewed Jun 20, 2023

View reviewed changes

furszy force-pushed the 2023_wallet_fix_spkm_id_corruption branch from 3a38f7f to 494fa79 Compare June 20, 2023 16:46

achow101 reviewed Jun 20, 2023

View reviewed changes

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

src/script/descriptor.h Outdated Show resolved Hide resolved

furszy force-pushed the 2023_wallet_fix_spkm_id_corruption branch from 494fa79 to 0f294a7 Compare June 20, 2023 22:39

Sjors reviewed Jun 22, 2023

View reviewed changes

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

furszy force-pushed the 2023_wallet_fix_spkm_id_corruption branch 4 times, most recently from 9e1c3b4 to 467b7fd Compare June 23, 2023 18:38

This was referenced Jun 23, 2023

MiniTapscript: port Miniscript to Tapscript #27255

Merged

Wallet: estimate the size of signed inputs using descriptors #26567

Merged

DrahtBot added the Needs rebase label Jun 28, 2023

furszy and others added 4 commits June 28, 2023 09:37

wallet: do not allow loading descriptor with an invalid ID

1d207e3

If the computed descriptor's ID doesn't match the wallet's DB spkm ID, return early from the loading process to prevent DB data from being modified in any post-loading procedure (e.g 'TopUp' updates the descriptor's data).

test: add coverage for descriptor ID

5df988b

Tests vectors were calculated by running the same tests on v25. Which was the last release prior to introducing the diff in the descriptor's string representation ('h' format). Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>

furszy force-pushed the 2023_wallet_fix_spkm_id_corruption branch from 467b7fd to 5df988b Compare June 28, 2023 12:40

DrahtBot removed the Needs rebase label Jun 28, 2023

furszy requested a review from Sjors July 3, 2023 14:16

DrahtBot removed the request for review from Sjors July 3, 2023 19:30

achow101 merged commit f08d914 into bitcoin:master Jul 4, 2023

furszy deleted the 2023_wallet_fix_spkm_id_corruption branch July 4, 2023 02:15

luke-jr reviewed Jul 4, 2023

View reviewed changes

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 4, 2023

Merge bitcoin#27920: wallet: bugfix, always use apostrophe for spkm d…

dd59d69

…escript…

furszy commented Sep 3, 2023

View reviewed changes

knst mentioned this pull request Aug 26, 2024

WIP - DO NOT MERGE - benchmark descriptor wallets dashpay/dash#6226

Closed

5 tasks

bitcoin locked and limited conversation to collaborators Sep 3, 2024

wallet: bugfix, always use apostrophe for spkm descriptor ID #27920

wallet: bugfix, always use apostrophe for spkm descriptor ID #27920

Uh oh!

Conversation

furszy commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Jun 22, 2023

Uh oh!

furszy commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Jun 26, 2023

Uh oh!

Sjors commented Jun 28, 2023

Uh oh!

furszy commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ErikDeSmedt commented Jun 28, 2023

Uh oh!

achow101 commented Jun 28, 2023

Uh oh!

Sjors commented Jul 3, 2023

Uh oh!

luke-jr Jul 4, 2023

Choose a reason for hiding this comment

Uh oh!

achow101 Jul 4, 2023

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Jul 4, 2023

Uh oh!

fanquake commented Jul 4, 2023

Uh oh!

achow101 commented Jul 4, 2023

Uh oh!

Sjors commented Jul 4, 2023

Uh oh!

denavila commented Sep 3, 2023

Uh oh!

sipa commented Sep 3, 2023

Uh oh!

denavila commented Sep 3, 2023

Uh oh!

furszy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

denavila commented Sep 3, 2023

Uh oh!

Sjors commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy commented Sep 4, 2023

Uh oh!

Uh oh!

furszy commented Jun 20, 2023 •

edited

Loading

DrahtBot commented Jun 20, 2023 •

edited

Loading

achow101 commented Jun 20, 2023 •

edited

Loading

furszy commented Jun 22, 2023 •

edited

Loading

furszy commented Jun 23, 2023 •

edited

Loading

furszy commented Jun 28, 2023 •

edited

Loading

furszy left a comment •

edited

Loading

Sjors commented Sep 4, 2023 •

edited

Loading