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)
./test/functional/wallet_multiwallet.py without compiling. It should fail.
170@@ -183,22 +171,6 @@ bool WalletInit::ParameterInteraction()
171 _("This is the transaction fee you may discard if change is smaller than dust at this level"));
172 CWallet::m_discard_rate = CFeeRate(nFeePerK);
173 }
174- if (gArgs.IsArgSet("-paytxfee"))
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.
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.
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).
static from three more members as requested by @ryanofsky
p2p_compactblocks.py failed in Travis. Rebase on master to pick up fix?
@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
2757@@ -2758,10 +2758,10 @@ UniValue settxfee(const JSONRPCRequest& request)
2758 return NullUniValue;
2759 }
2760
2761- if (request.fHelp || request.params.size() < 1 || request.params.size() > 1)
2762+ if (request.fHelp || request.params.size() < 1 || request.params.size() > 1) {
2763 throw std::runtime_error(
2764 "settxfee amount\n"
2765- "\nSet the transaction fee per kB. Overwrites the paytxfee parameter.\n"
2766+ "\nSet the transaction fee per kB for this wallet. Overwrites the global -paytxfee command line parameter.\n"
46-
47 //! Default for -keypool
48 static const unsigned int DEFAULT_KEYPOOL_SIZE = 1000;
49 //! -paytxfee default
50-static const CAmount DEFAULT_TRANSACTION_FEE = 0;
51+constexpr CAmount DEFAULT_PAY_TX_FEE = 0;
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?
25@@ -26,11 +26,9 @@ class Coin;
26 class RPCTimerInterface;
27 class UniValue;
28 class proxyType;
29-enum class FeeReason;
CCoinControl forward declaration?
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.
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.