New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
walletdb: don't reinitialize desc cache with multiple cache entries #19441
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
tACK a66a7a1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK a66a7a1
Verified the test fails on master and passes with the code change
…tiple cache entries a66a7a1 walletdb: don't reinitialize desc cache with multiple cache entries (Andrew Chow) Pull request description: 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. ACKs for top commit: hugohn: tACK [a66a7a1](bitcoin@a66a7a1) jonatack: ACK a66a7a1 meshcollider: Code review ACK a66a7a1 Tree-SHA512: 3df746421a008708eaa3bbbdd12b9ddd3e2ec111d54625a212dca7414b971cc1f6e2b1757b3232c31a2f637d1b1ef43bf3ffa4ac4216646cf1e92db5f79954f1
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
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.