Remove wallet access to some node arguments

jnewbery commented at 5:55 pm on October 14, 2019: member

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.

[wallet] Remove package limit config access from wallet

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

eea462de9c

[wallet] Remove pruning check for -rescan option

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

b96ed03962

fanquake added the label Wallet on Oct 14, 2019

DrahtBot commented at 6:51 pm on October 14, 2019: member

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 commented at 8:39 pm on October 14, 2019: contributor

Concept ACK

Thanks for working on this!

laanwj commented at 12:03 pm on October 15, 2019: member

Concept ACK

in src/wallet/init.cpp:126 in b96ed03962

121@@ -122,8 +122,6 @@ bool WalletInit::ParameterInteraction() const
122 
123     if (gArgs.GetBoolArg("-sysperms", false))
124         return InitError("-sysperms is not allowed in combination with enabled wallet functionality");
125-    if (gArgs.GetArg("-prune", 0) && gArgs.GetBoolArg("-rescan", false))
126-        return InitError(_("Rescans are not possible in pruned mode. You will need to use -reindex which will download the whole blockchain again.").translated);

ryanofsky commented at 3:24 pm on October 15, 2019:

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

promag commented at 5:59 pm on October 15, 2019:

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

Side note, there was no test for this 😞

ariard commented at 6:11 pm on October 15, 2019:

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?

ryanofsky commented at 6:13 pm on October 15, 2019:

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.

ryanofsky approved

ryanofsky commented at 3:26 pm on October 15, 2019: member

Code review ACK b96ed0396294fc4fa89d83ceab6bc169dd09f002

MarcoFalke renamed this:
~~Remove wallet access to some node arguments~~
refactor: Remove wallet access to some node arguments
on Oct 15, 2019

MarcoFalke added the label Refactoring on Oct 15, 2019

MarcoFalke removed the label Refactoring on Oct 15, 2019

MarcoFalke renamed this:
~~refactor: Remove wallet access to some node arguments~~
Remove wallet access to some node arguments
on Oct 15, 2019

promag commented at 6:01 pm on October 15, 2019: member

Code review ACK b96ed0396294fc4fa89d83ceab6bc169dd09f002.

ariard approved

ariard commented at 6:12 pm on October 15, 2019: member

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

MarcoFalke commented at 6:55 pm on October 15, 2019: member

Before (master) always showing an error:

Screenshot from 2019-10-15 14-46-47

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

Screenshot from 2019-10-15 14-49-56

Tested ACK b96ed0396294fc4fa89d83ceab6bc169dd09f002

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3Tested ACK b96ed0396294fc4fa89d83ceab6bc169dd09f002
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhSaAv/aA1uM2rqWt+jTHUWQOVRX+wJC9Mj1fnJr4YE7FS9zcSVepfET22JsZVg
 8i8aN3DZnKJZioh4MpWqTQokOIChWl0FMsLEWrNfjb4w+8Mc4ngemGeo8jnNVP9+H
 9CZv+ZE4xLfVztUx/mrRQQIaXcgKLhgL4lgEPhbuEwa7/3WpHVGFO4nugyLTAprvQ
10mNMd4ZFjXCXGomyDurtX9wSantZyhHYzPPWjIAc1DrvLRbVIR0fUssWSeGz+2Sks
11bZ6l8IML1Lx5IcaOZz3tjWx1ZGcw132LQrELfmU7NccGADo6b3W/hpoYb9Fbs9H4
124tchOhdi+RMBTMRcKVBt4RnG8Z5xQbgpUEYWjHrbuq0N3F5tewa8DbffLN9dNO+s
13iUZB/J/++6iKKB0JQLh8BMQ7W1WXCCL5gBK60DCCMOa48vISwxyjTcehoeUq9r/n
145bzbB5ECWNBEBeZkA4t9SwGpVBMlfU2tExAudncLIL4XRvPCR6NGyAg2YYHb4vb1
158bDj7sd6
16=mR0T
17-----END PGP SIGNATURE-----

Timestamp of file with hash 7fc406744b9a7b419ca0b002011cb8b2f67acde9d3060b2717e11fe44257c7dd -

MarcoFalke commented at 7:01 pm on October 15, 2019: member

To put it into context, this pull is doing for command line args, what has already been done for the rpc, see #15870

MarcoFalke referenced this in commit a3af5b5c13 on Oct 15, 2019

MarcoFalke merged this on Oct 15, 2019

MarcoFalke closed this on Oct 15, 2019

jnewbery deleted the branch on Oct 15, 2019

sipa commented at 8:37 pm on October 15, 2019: member

What a weird language you’re testing things with, @MarcoFalke.

MarkLTZ referenced this in commit c3549d43c7 on Nov 17, 2019

jasonbcox referenced this in commit ae229def7d on Jul 31, 2020

jasonbcox referenced this in commit 756dd87415 on Jul 31, 2020

DrahtBot locked this on Dec 16, 2021

Remove wallet access to some node arguments #17138

Conflicts