Remove wallet access to some node arguments #17138

jnewbery · 2019-10-14T17:55:44Z

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.

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().

DrahtBot · 2019-10-14T18:51:59Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17127 (util: Correct permissions for datadir and wallets subdir by hebasto)
#16659 (refactoring: Remove unused includes by practicalswift)
#16224 (gui: Bilingual GUI error messages by hebasto)

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.

practicalswift · 2019-10-14T20:39:20Z

Concept ACK

Thanks for working on this!

laanwj · 2019-10-15T12:03:22Z

Concept ACK

ryanofsky

Code review ACK b96ed03

ryanofsky · 2019-10-15T15:24:25Z

src/wallet/init.cpp

@@ -122,8 +122,6 @@ bool WalletInit::ParameterInteraction() const

    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);


This seems like a pretty user friendly error message that might be better to keep.

I agree with commit description, which is in line with #16037.

Side note, there was no test for this 😞

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?

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.

promag

Code review ACK b96ed03.

promag · 2019-10-15T17:59:18Z

src/wallet/init.cpp

@@ -122,8 +122,6 @@ bool WalletInit::ParameterInteraction() const

    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);


I agree with commit description, which is in line with #16037.

Side note, there was no test for this 😞

ariard

ACK b96ed03, check there isn't left anymore wallet access to node arguments.

ariard · 2019-10-15T18:11:36Z

src/wallet/init.cpp

@@ -122,8 +122,6 @@ bool WalletInit::ParameterInteraction() const

    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);


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?

maflcko · 2019-10-15T18:55:24Z

Before (master) always showing an error:

After (this pull) either doing a rescan or showing an error:

Tested ACK b96ed03

Show signature and timestamp

Signature:

Timestamp of file with hash 7fc406744b9a7b419ca0b002011cb8b2f67acde9d3060b2717e11fe44257c7dd -

maflcko · 2019-10-15T19:01:01Z

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

sipa · 2019-10-15T20:37:31Z

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

jnewbery added 2 commits October 14, 2019 13:32

[wallet] Remove package limit config access from wallet

eea462d

The wallet should not be able to directly access global configuration from the node. Remove access of "-limitancestorcount" and "-limitdescendantcount".

fanquake added the Wallet label Oct 14, 2019

ryanofsky approved these changes Oct 15, 2019

View reviewed changes

maflcko changed the title ~~Remove wallet access to some node arguments~~ refactor: Remove wallet access to some node arguments Oct 15, 2019

maflcko added Refactoring and removed Refactoring labels Oct 15, 2019

maflcko changed the title ~~refactor: Remove wallet access to some node arguments~~ Remove wallet access to some node arguments Oct 15, 2019

promag reviewed Oct 15, 2019

View reviewed changes

ariard approved these changes Oct 15, 2019

View reviewed changes

maflcko merged commit b96ed03 into bitcoin:master Oct 15, 2019

jnewbery deleted the 2019-10-remove-wallet-access-to-node-args branch October 15, 2019 19:09

This was referenced Oct 16, 2019

util: Set safe permissions for data directory and wallets/ subdir #17127

Merged

gui: Bilingual GUI error messages #16224

Merged

MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Nov 17, 2019

Remove wallet access to some node arguments (bitcoin#17138)

c3549d4

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

jnewbery mentioned this pull request Apr 24, 2020

Remove wallet access to node arguments #17137

Closed

7 tasks

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove wallet access to some node arguments #17138

Remove wallet access to some node arguments #17138

Uh oh!

jnewbery commented Oct 14, 2019

Uh oh!

DrahtBot commented Oct 14, 2019

Uh oh!

practicalswift commented Oct 14, 2019

Uh oh!

laanwj commented Oct 15, 2019

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky Oct 15, 2019

Uh oh!

promag Oct 15, 2019

Uh oh!

ariard Oct 15, 2019

Uh oh!

ryanofsky Oct 15, 2019

Uh oh!

promag left a comment

Uh oh!

promag Oct 15, 2019

Uh oh!

ariard left a comment

Uh oh!

ariard Oct 15, 2019

Uh oh!

maflcko commented Oct 15, 2019

Uh oh!

maflcko commented Oct 15, 2019

Uh oh!

sipa commented Oct 15, 2019

Uh oh!

Uh oh!

Remove wallet access to some node arguments #17138

Remove wallet access to some node arguments #17138

Uh oh!

Conversation

jnewbery commented Oct 14, 2019

Uh oh!

DrahtBot commented Oct 14, 2019

Conflicts

Uh oh!

practicalswift commented Oct 14, 2019

Uh oh!

laanwj commented Oct 15, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

promag Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

ariard Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

ariard Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 15, 2019

Uh oh!

maflcko commented Oct 15, 2019

Uh oh!

sipa commented Oct 15, 2019

Uh oh!

Uh oh!