wallet: remove unused DummySignTx and CKeyPool from GetReservedDestination #25881
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
| fInternal = keypool.fInternal; | ||
| fInternal = internal; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
ReserveKeyFromKeyPoolwill erase the last key index fromset_pre_split_keypool. Leaving the set empty.- As
set_pre_split_keypoolis empty,ReturnDestinationwill not return the index toset_pre_split_keypooland wrongly push it insidesetExternalKeyPool(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 (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 (#25881 (comment)). 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
ce81b9a to
56d4e97
Compare
|
Closing it for now. Have other PRs with more priority than this one. |
Grouped few simple cleanups, found them while was doing other stuff (similar to #25526).
CKeyPoolonGetReservedDestination.CKeyPoolconstructors fromwallet.cppas them should only be part of the legacy spkm.CWallet::DummySignTx.