Grouped few simple cleanups, found them while was doing other stuff (similar to #25526).
CKeyPool on GetReservedDestination.CKeyPool constructors from wallet.cpp as them should only be part of the legacy spkm.CWallet::DummySignTx.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Stale ACK | aureleoules, w0xlt, fanquake |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
ACK ce81b9a4fa821e92813e62d15d0b1ce5131e0a2e.
c88092403d42d15714f6cd72a39528859c8c2725: verified that the keypool argument can be removed without altering the behavior of the code
310f25e42a98c05c467c03b8df761c00f16ab214: looks better and is the prefered way for initializing class members :+1:
ce81b9a4fa821e92813e62d15d0b1ce5131e0a2e: verified that this overloaded function is unused
2472 | - auto op_address = m_spk_man->GetReservedDestination(type, internal, nIndex, keypool); 2473 | + auto op_address = m_spk_man->GetReservedDestination(type, internal, nIndex); 2474 | if (!op_address) return op_address; 2475 | address = *op_address; 2476 | - fInternal = keypool.fInternal; 2477 | + fInternal = internal;
I'm pretty sure that this is actually incorrect.
internal and keypool.fInternal will not always be the same. For wallets before HD chain split, internal=true will still result in keypool.fInternal=false. Later, ReserveDestination.fInternal is used by ReturnDestination in order to put the kepool data back into the correct keypool set, and the correct set is determined by the value of ReserveDestionation.fInternal.
With a pre-split wallet or wallets with a pre-split keypool, with this change we will end up placing returned destinations to the internal keypool where they should go into the external keypool or the pre-split keypool. Since we decide which set to fetch new keypool keys from by using the feature flags, for such wallets we will not choose the internal keypool as the flags indicate that is not supported. Thus with such wallets, we are just throwing away change addresses rather than correctly returning them to the keypool.
hmm true, good catch. The code is quite entangled there.
You know, the legacy version of ReturnDestination should be checking whether the wallet is pre-split or not to return the key to the proper place as ReserveKeyFromKeyPool is doing.. and not just place it into the internal pool directly if the internal flag is true.
Another thing that looks like an existent bug is what happens when the wallet takes the last key from the pre-split keypool:
ReserveKeyFromKeyPool will erase the last key index from set_pre_split_keypool. Leaving the set empty.set_pre_split_keypool is empty, ReturnDestination will not return the index to set_pre_split_keypool and wrongly push it inside setExternalKeyPool (because before return the index into the pre-split set we are checking that the set is not empty).
@furszy what are the next steps here? This has two ACKs, but they can be discarded, given the issue pointed out by achow. Are you planning up updating this to drop that commit (c88092403d42d15714f6cd72a39528859c8c2725), or follow up with different changes? If you're not actively working on this at the moment, do you want to turn this into a draft until it's ready for review again?
@furszy what are the next steps here? This has two ACKs, but they can be discarded, given the issue pointed out by achow. Are you planning up updating this to drop that commit (c880924), or follow up with different changes? If you're not actively working on this at the moment, do you want to turn this into a draft until it's ready for review again?
yeah, IIRC I found an existent bug in the legacy wallet while was checking achow's comment (https://github.com/bitcoin/bitcoin/pull/25881#discussion_r953824206).
Will move it to draft until I'm able to get back to it. I don't think that the other two cleanup commits alone worth enough to ping reviewers.
removing the constructors from wallet.cpp
Closing it for now. Have other PRs with more priority than this one.