wallet: Make fee settings to be non-static members #12909

maflcko · 2018-04-07T18:36:04Z

The wallet header defined some globals (they were called "settings"), that should be class members instead.

This commit is hopefully only refactoring, apart from a multiwallet bugfix: Calling the rpc settxfee for one wallet, would set (and change) the fee rate for all loaded wallets. (See added test case)

maflcko · 2018-04-07T18:36:42Z

For testing you can checkout this commit and run ./test/functional/wallet_multiwallet.py without compiling. It should fail.

ryanofsky · 2018-04-09T22:02:30Z

src/wallet/init.cpp

Is there a reason not to make options like -discardfee and -mintxfee wallet members, the same way this is doing for -fallbackfee and -paytxfee options? It doesn't seem like a good thing to create a distinction here when moving the options together is possible.

Making more options members also seems nice in case we want to add a wallet RPC for changing preferences dynamically without affecting other wallets.

mintxfee et al are already members (static, though). I agree that those should be changed to be non-static members, but I'd rather do this in a follow up pr. Otherwise this one will explode in scope, scare away revierers and will gracelessly end up in rebase hell.

Making more options members also seems nice in case we want to add a wallet RPC for changing preferences dynamically without affecting other wallets.

Please see the included test case: We are already doing it wrong.

#12909 (comment)

mintxfee et al are already members (static, though). I agree that those should be changed to be non-static members, but I'd rather do this in a follow up pr. Otherwise this one will explode in scope, scare away revierers and will gracelessly end up in rebase hell.

I think "wallet-specific fee options" would be an appropriate scope for this PR and not too broad. The -mintxfee, -fallbackfee, -discardfee, and -paytxfee options are literally initialized one after another here, so it's strange to me that you want to move the second and fourth initializations but not the first and third based them on being global variables instead of static class member variables (since all the variables function identically).

laanwj · 2018-04-11T06:05:53Z

Concept ACK

ryanofsky

utACK fad3bc5cef786049f70ff485001e9e11791ef350

ryanofsky · 2018-04-13T20:33:14Z

src/interfaces/node.h

Maybe revert this formatting change (this is what caused me to open #12982).

ryanofsky · 2018-04-13T20:44:24Z

src/wallet/init.cpp

#12909 (comment)

mintxfee et al are already members (static, though). I agree that those should be changed to be non-static members, but I'd rather do this in a follow up pr. Otherwise this one will explode in scope, scare away revierers and will gracelessly end up in rebase hell.

I think "wallet-specific fee options" would be an appropriate scope for this PR and not too broad. The -mintxfee, -fallbackfee, -discardfee, and -paytxfee options are literally initialized one after another here, so it's strange to me that you want to move the second and fourth initializations but not the first and third based them on being global variables instead of static class member variables (since all the variables function identically).

maflcko · 2018-04-17T03:24:30Z

Removed static from three more members as requested by @ryanofsky

jnewbery · 2018-04-17T18:45:25Z

p2p_compactblocks.py failed in Travis. Rebase on master to pick up fix?

maflcko · 2018-04-17T18:56:00Z

@jnewbery Travis seems to fetch from git fetch origin +refs/pull/${PULL_NUMBER}/merge, so a re-run should suffice?

C.f. https://travis-ci.org/bitcoin/bitcoin/jobs/366515927#L356

ryanofsky

utACK fa66188a5c8b862695dd8d730f7c900bfd303dcf. This is great cleanup! It's really nice to see these settings start to get consolidated and handled in a consistent way.

ryanofsky · 2018-04-17T21:44:18Z

src/wallet/rpcwallet.cpp

Comment is kind of misleading. I think it would be more accurate to say "overrides" instead of "overwrites" now.

TIL those words mean different things. Thx and done. Should be trivial to re-ACK if you still have the previous commit.

ryanofsky

utACK fafdeca1ec5d32291fe7b0313e123ad22d8e284b. Only change since the previous review is the updated RPC documentation string.

maflcko · 2018-04-19T13:15:32Z

Clean rebase to reduce the diff (getting rid of deleted node-interface methods)

ryanofsky

utACK eeeea3c066f8b1351b911593aac7b062f48f1976, only change since last review is rebase

jnewbery

utACK eeeea3c066f8b1351b911593aac7b062f48f1976 with request for follow-up commit.

jnewbery · 2018-04-19T16:59:57Z

src/wallet/wallet.h

Seems a little weird to change this to constexpr and leave all the other default constants as static const. It's fine, but can we have a follow-up PR to change the remaining constants to constexprs?

Sure. There is a bit of refactoring going on in the wallet, so I will wait until the dust settled a bit.

laanwj · 2018-04-23T10:11:51Z

Sorry, needs rebase again

jonasschnelli · 2018-04-23T12:00:46Z

Concept ACK (utACK on eeeea3c066f8b1351b911593aac7b062f48f1976 which though requires rebase).

maflcko · 2018-04-23T14:53:18Z

Rebased

promag

This commit is hopefully only refactoring, apart from a multiwallet bugfix

Probably not that problematic, it's very unlikely someone depends on the current behavior, but I think this could have release note.

utACK fac0db0.

promag · 2018-04-23T15:27:55Z

src/interfaces/node.h

 class RPCTimerInterface;
 class UniValue;
 class proxyType;
-enum class FeeReason;


Also remove CCoinControl forward declaration?

Will leave it in for now, because I don't want to go through another rebase-reACK iteration just for this. Also, there are redundant forward declarations in other parts of the code, so this is not making the overall problem worse.

Yeah, don't mind that, just noted in case you have to push again.

jnewbery · 2018-04-23T15:46:12Z

I think this could have release note.

I don't think it's necessary. We don't add notes for every bugfix, and this seems very trivial.

jnewbery · 2018-04-23T16:07:44Z

tested ACK fac0db0

fac0db0 wallet: Make fee settings non-static members (MarcoFalke) Pull request description: The wallet header defined some globals (they were called "settings"), that should be class members instead. This commit is hopefully only refactoring, apart from a multiwallet bugfix: Calling the rpc `settxfee` for one wallet, would set (and change) the fee rate for all loaded wallets. (See added test case) Tree-SHA512: 4ab6ec2f5c714742396ded5e451ec3b1ceb771e3696492de29889d866de4365b3fbe4a2784d085c8b8bd11b1ebb8a1fec99ab2c62eee716791cfc67c0cf29e1b

maflcko added Wallet RPC/REST/ZMQ labels Apr 7, 2018

maflcko added the Needs release note label Apr 7, 2018

maflcko force-pushed the Mf1804-walletMembers branch from fa62e3b to 5a08c94 Compare April 7, 2018 18:50

maflcko removed the Needs release note label Apr 7, 2018

maflcko force-pushed the Mf1804-walletMembers branch 7 times, most recently from 1643a7c to fad3bc5 Compare April 9, 2018 18:36

ryanofsky reviewed Apr 9, 2018

View reviewed changes

ryanofsky reviewed Apr 13, 2018

View reviewed changes

maflcko force-pushed the Mf1804-walletMembers branch 2 times, most recently from faac0e6 to fa04752 Compare April 14, 2018 14:44

maflcko changed the title ~~wallet: Make "settings" to be class members~~ wallet: Make fee settings to be non-static members Apr 14, 2018

maflcko force-pushed the Mf1804-walletMembers branch 5 times, most recently from fae9a4b to fa66188 Compare April 14, 2018 15:10

ryanofsky reviewed Apr 17, 2018

View reviewed changes

maflcko force-pushed the Mf1804-walletMembers branch from fa66188 to fafdeca Compare April 17, 2018 22:00

ryanofsky reviewed Apr 17, 2018

View reviewed changes

maflcko force-pushed the Mf1804-walletMembers branch 2 times, most recently from b79c4d0 to eeeea3c Compare April 19, 2018 13:14

ryanofsky reviewed Apr 19, 2018

View reviewed changes

jnewbery reviewed Apr 19, 2018

View reviewed changes

jnewbery mentioned this pull request Apr 20, 2018

[RFC] Long term plan for wallet command-line args #13044

Closed

laanwj self-assigned this Apr 23, 2018

maflcko force-pushed the Mf1804-walletMembers branch from eeeea3c to bcf4e8c Compare April 23, 2018 14:49

wallet: Make fee settings non-static members

fac0db0

maflcko force-pushed the Mf1804-walletMembers branch from bcf4e8c to fac0db0 Compare April 23, 2018 14:49

promag reviewed Apr 23, 2018

View reviewed changes

laanwj merged commit fac0db0 into bitcoin:master Apr 24, 2018

maflcko deleted the Mf1804-walletMembers branch April 24, 2018 14:47

Bushstar mentioned this pull request Apr 25, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#308

Merged

ryanofsky mentioned this pull request Apr 25, 2018

Refactor: separate wallet from node #10973

Merged

maflcko mentioned this pull request Dec 3, 2018

settxfee should require wallet unlock #14824

Closed

maflcko mentioned this pull request Apr 11, 2019

[wallet] Move maxtxfee from node to wallet #15778

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

wallet: Make fee settings to be non-static members #12909

wallet: Make fee settings to be non-static members #12909

Uh oh!

Conversation

maflcko commented Apr 7, 2018

Uh oh!

maflcko commented Apr 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Apr 11, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 17, 2018

Uh oh!

jnewbery commented Apr 17, 2018

Uh oh!

maflcko commented Apr 17, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 19, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Apr 23, 2018

Uh oh!

jonasschnelli commented Apr 23, 2018

Uh oh!

maflcko commented Apr 23, 2018

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Apr 23, 2018

Uh oh!

jnewbery commented Apr 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

promag left a comment •

edited

Loading