Remove wallet access to some node arguments #17138

pull jnewbery wants to merge 2 commits into bitcoin:master from jnewbery:2019-10-remove-wallet-access-to-node-args changing 4 files +15 −5

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

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 7fc406744b9a7b419ca0b002011cb8b2f67acde9d3060b2717e11fe44257c7dd -

</details>

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

Contributors

Labels

Linked (view graph)

#17137 Remove wallet access to node arguments

Remove wallet access to some node arguments #17138

Conflicts