walletdb: don't reinitialize desc cache with multiple cache entries - #19441
Merged
Conversation
When loading descriptor caches, we would accidentally reinitialize the descriptor cache when seeing that one already exists. This should have only been initializing the cache when one does not exist. However this code itself is unnecessary as the act of looking up the cache to add to it will initialize it if it didn't already exist. This issue could be hit by trying to load a wallet that had imported a multisig descriptor. The wallet would fail to load. A test has been added to wallet_importdescriptors.py to catch this case. Another test case has also been added to check that loading a wallet with only single key descriptors works.
jonatack
reviewed
Jul 5, 2020
jonatack
left a comment
jonatack
left a comment
Member
There was a problem hiding this comment.
ACK a66a7a1
Without the change in wallet/walletdb.cpp::595-597 the updated test fails on reloading the wallet at line 383 with the error message from DescriptorScriptPubKeyMan::SetCache Error: Unable to expand wallet descriptor from cache (-4).
Contributor
|
tACK a66a7a1 |
meshcollider
left a comment
meshcollider
left a comment
Contributor
There was a problem hiding this comment.
Code review ACK a66a7a1
Verified the test fails on master and passes with the code change
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Aug 31, 2021
Summary: > When loading descriptor caches, we would accidentally reinitialize the > descriptor cache when seeing that one already exists. This should have > only been initializing the cache when one does not exist. However this > code itself is unnecessary as the act of looking up the cache to add to > it will initialize it if it didn't already exist. > > This issue could be hit by trying to load a wallet that had imported a > multisig descriptor. The wallet would fail to load. > > A test has been added to wallet_importdescriptors.py to catch this case. > Another test case has also been added to check that loading a wallet > with only single key descriptors works. This is a backport of [[bitcoin/bitcoin#19441 | core#19441]] Test Plan: `ninja all check-all` I confirmed that before the change in walletdb.cpp, the new test fails with an error `Error: Unable to expand wallet descriptor from cache (-4)`. And the change fixes it. Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9991
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When loading descriptor caches, we would accidentally reinitialize the descriptor cache when seeing that one already exists. This should have only been initializing the cache when one does not exist. However this code itself is unnecessary as the act of looking up the cache to add to it will initialize it if it didn't already exist.
This issue could be hit by trying to load a wallet that had imported a multisig descriptor. The wallet would fail to load.
A test has been added to wallet_importdescriptors.py to catch this case. Another test case has also been added to check that loading a wallet with only single key descriptors works.