Remove wallet access to some node arguments #17138
Conversation
The wallet should not be able to directly access global configuration from the node. Remove access of "-limitancestorcount" and "-limitdescendantcount".
Prior to this PR, the wallet would not allow the `-rescan` option at startup if pruning was enabled. This is unnecessarily restrictive. It should be possible to rescan if pruning is enabled, as long as no blocks have actually been pruned yet. Remove the pruning check from WalletInit::ParameterInteraction(). If any blocks have been pruned, that will be caught in CreateWalletFromFile().
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
|
Concept ACK Thanks for working on this! |
|
Concept ACK |
| if (gArgs.GetBoolArg("-sysperms", false)) | ||
| return InitError("-sysperms is not allowed in combination with enabled wallet functionality"); | ||
| if (gArgs.GetArg("-prune", 0) && gArgs.GetBoolArg("-rescan", false)) | ||
| return InitError(_("Rescans are not possible in pruned mode. You will need to use -reindex which will download the whole blockchain again.").translated); |
There was a problem hiding this comment.
This seems like a pretty user friendly error message that might be better to keep.
There was a problem hiding this comment.
I agree with commit description, which is in line with #16037.
Side note, there was no test for this 😞
There was a problem hiding this comment.
I think this error message is wrong. You can rescan in pruned mode if rescan height is in range of non-pruned blocks. It's really likely it's going to fail if wallet has been disabled for a long time but not sure. Maybe a warning?
There was a problem hiding this comment.
I agree with commit description, which is in line with #16037.
Oops, I hadn't seen the commit description. I see the message in CreateWalletFile keeps the suggestion to use -reindex, so this looks good to me.
| if (gArgs.GetBoolArg("-sysperms", false)) | ||
| return InitError("-sysperms is not allowed in combination with enabled wallet functionality"); | ||
| if (gArgs.GetArg("-prune", 0) && gArgs.GetBoolArg("-rescan", false)) | ||
| return InitError(_("Rescans are not possible in pruned mode. You will need to use -reindex which will download the whole blockchain again.").translated); |
There was a problem hiding this comment.
I agree with commit description, which is in line with #16037.
Side note, there was no test for this 😞
| if (gArgs.GetBoolArg("-sysperms", false)) | ||
| return InitError("-sysperms is not allowed in combination with enabled wallet functionality"); | ||
| if (gArgs.GetArg("-prune", 0) && gArgs.GetBoolArg("-rescan", false)) | ||
| return InitError(_("Rescans are not possible in pruned mode. You will need to use -reindex which will download the whole blockchain again.").translated); |
There was a problem hiding this comment.
I think this error message is wrong. You can rescan in pruned mode if rescan height is in range of non-pruned blocks. It's really likely it's going to fail if wallet has been disabled for a long time but not sure. Maybe a warning?
|
Before (master) always showing an error:
After (this pull) either doing a rescan or showing an error:
Tested ACK b96ed03 Show signature and timestampSignature: Timestamp of file with hash |
|
To put it into context, this pull is doing for command line args, what has already been done for the rpc, see #15870 |
b96ed03 [wallet] Remove pruning check for -rescan option (John Newbery) eea462d [wallet] Remove package limit config access from wallet (John Newbery) Pull request description: Removes wallet access to `-limitancestorcount`, `-limitdescendantcount` and `-prune`: - `-limitancestorcount` and `-limitdescendantcount` are now accessed with a method `getPackageLimits` in the `Chain` interface. - `-prune` is not required. It was only used in wallet component initiation to prevent running `-rescan` when pruning was enabled. This check is not required. Partially addresses #17137. ACKs for top commit: MarcoFalke: Tested ACK b96ed03 ryanofsky: Code review ACK b96ed03 promag: Code review ACK b96ed03. ariard: ACK b96ed03, check there isn't left anymore wallet access to node arguments. Tree-SHA512: 90c8e3e083acbd37724f1bccf63dab642cf9ae95cc5e684872a67443ae048b4fdbf57b52ea47c5a1da6489fd277278fe2d9bbe95e17f3d4965a1a0fbdeb815bf
|
What a weird language you're testing things with, @MarcoFalke. |
…wallet Summary: The wallet should not be able to directly access global configuration from the node. Remove access of "-limitancestorcount" and "-limitdescendantcount". bitcoin/bitcoin@eea462d --- Partial backport of Core [[bitcoin/bitcoin#17138 | PR17138]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7090
Summary: Prior to this PR, the wallet would not allow the `-rescan` option at startup if pruning was enabled. This is unnecessarily restrictive. It should be possible to rescan if pruning is enabled, as long as no blocks have actually been pruned yet. Remove the pruning check from WalletInit::ParameterInteraction(). If any blocks have been pruned, that will be caught in CreateWalletFromFile(). bitcoin/bitcoin@b96ed03 --- Depends on D7090 Concludes backport of Core [[bitcoin/bitcoin#17138 | PR17138]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7091
Removes wallet access to
-limitancestorcount,-limitdescendantcountand-prune:-limitancestorcountand-limitdescendantcountare now accessed with a methodgetPackageLimitsin theChaininterface.-pruneis not required. It was only used in wallet component initiation to prevent running-rescanwhen pruning was enabled. This check is not required.Partially addresses #17137.