[wallet] Move maxtxfee from node to wallet #15778

jnewbery · 2019-04-09T20:18:11Z

Closes #15355

Moves the -maxtxfee from the node to the wallet. See discussion in issue for details.

This is a cleanup. There is no change in behaviour.

Completes #15620

jnewbery · 2019-04-09T20:18:39Z

~~Builds on #15638. Only the last commit is for this PR.~~

EDIT: base PR is merged. I've rebased on master.

maflcko · 2019-04-09T20:26:41Z

This only requires d38cfdf of the other pull request. The other pull request has a lot of conflicts and might need a long time for review to get in, so I'd prefer if you just cherry-picked that commit and your maxtxfee changes here. Review can then happen here instead of the larger pull request. No strong opinion, though.

jnewbery · 2019-04-09T20:49:21Z

Thanks for looking at this @MarcoFalke . I'm in no rush for this to get merged, and would much prefer to see progress on #15638, so I'm going to try to avoid causing rebases for Russ.

(incidentally, 15638 is a very easy review. It's almost entirely move-only and all the commits are well commented, so in theory at least it should be possible to review/merge quite quickly).

DrahtBot · 2019-04-09T22:25:33Z

Needs rebase

jnewbery · 2019-04-10T13:23:31Z

This now requires rebase. I plan not to do that until #15638 is merged. Review is still welcomed for the last commit (although ACKs will need to be refreshed after rebase).

jnewbery · 2019-04-10T13:59:54Z

Rebased on master. This is ready for review.

maflcko · 2019-04-10T14:48:05Z

Many tests fail. Do they pass locally in valgrind?

DrahtBot · 2019-04-10T15:06:17Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15870 (wallet: Only fail rescan when blocks have actually been pruned by MarcoFalke)
#15845 (wallet: Fast rescan with BIP157 block filters by MarcoFalke)
#15764 (Native descriptor wallets by achow101)
#15713 (refactor: Replace chain relayTransactions/submitMemoryPool by higher method by ariard)
#12419 (Force distinct destinations in CWallet::CreateTransaction by promag)
#12096 ([rpc] [wallet] Allow specifying the output index when using bumpfee by kallewoof)

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.

jnewbery · 2019-04-10T18:30:24Z

Many tests fail.

You're right. Investigating now.

EDIT: I wasn't initializing g_max_tx_fee properly. Should be fixed now.

maflcko

utACK, some style comments

src/wallet/wallet.h

ryanofsky

utACK bef4ce8. Could say explicitly in the PR description that this is just cleanup, and there's no change in behavior.

src/interfaces/wallet.cpp

src/wallet/wallet.h

jnewbery · 2019-04-11T15:53:58Z

Moved the max tx fee option to be a per-wallet setting. This required adding a max_tx_fee parameter to CWalletTx::AcceptToMemoryPool() and CWalletTx::RelayTransaction() which is ugly. I hope we can remove clean those up in a future PR.

ryanofsky · 2019-04-11T16:40:12Z

This required adding a max_tx_fee parameter to CWalletTx::AcceptToMemoryPool() and CWalletTx::RelayTransaction() which is ugly

I think you could use the CWalletTx::pwallet pointer to avoid this.

jnewbery · 2019-04-11T16:41:36Z

I think you could use the CWalletTx::pwallet pointer to avoid this.

You're right. Thanks. ~~Will fix.~~ EDIT: Fixed.

ryanofsky

utACK a9706d1

ryanofsky · 2019-04-17T15:05:28Z

src/interfaces/wallet.cpp

@@ -464,6 +464,7 @@ class WalletImpl : public Wallet
    bool IsWalletFlagSet(uint64_t flag) override { return m_wallet->IsWalletFlagSet(flag); }
    OutputType getDefaultAddressType() override { return m_wallet->m_default_address_type; }
    OutputType getDefaultChangeType() override { return m_wallet->m_default_change_type; }
+    CAmount getMaxTxFee() override { return m_wallet->m_default_max_tx_fee; }


Maybe call this getDefaultMaxTxFee for consistency. Also because I could imagine a GUI interface that allows overriding this and doesn't treat it as a hard limit.

ryanofsky · 2019-04-17T15:10:47Z

src/wallet/feebumper.cpp

     if (new_fee > max_tx_fee) {
-         errors.push_back(strprintf("Specified or calculated fee %s is too high (cannot be higher than maxTxFee %s)",
+         errors.push_back(strprintf("Specified or calculated fee %s is too high (cannot be higher than maxtxfee %s)",


I think tweaking this error message is the only change in behavior in the pr. Might be good to note in the commit message that the commit doesn't change behavior except for this.

Also I might suggest writing -maxtxfee instead of maxtxfee to suggest that this is referring to an argument/config setting. It could help someone find the relevant manpage / help documentation.

updated commit log

ryanofsky · 2019-04-17T15:19:45Z

src/init.cpp

@@ -502,8 +502,6 @@ void SetupServerArgs()
    gArgs.AddArg("-mocktime=<n>", "Replace actual time with <n> seconds since epoch (default: 0)", true, OptionsCategory::DEBUG_TEST);
    gArgs.AddArg("-maxsigcachesize=<n>", strprintf("Limit sum of signature cache and script execution cache sizes to <n> MiB (default: %u)", DEFAULT_MAX_SIG_CACHE_SIZE), true, OptionsCategory::DEBUG_TEST);
    gArgs.AddArg("-maxtipage=<n>", strprintf("Maximum tip age in seconds to consider node in initial block download (default: %u)", DEFAULT_MAX_TIP_AGE), true, OptionsCategory::DEBUG_TEST);
-    gArgs.AddArg("-maxtxfee=<amt>", strprintf("Maximum total fees (in %s) to use in a single wallet transaction; setting this too low may abort large transactions (default: %s)", // TODO move setting to wallet


This is another change in behavior that might be worth noting in the commit message or release notes: that specifiying -maxtxfee will now result in an error in non-wallet builds.

I don't think this is true. Doesn't defining -maxtxfee in dummywallet.cpp ensure that there's no error?

I can change this so there is an error, which would effectively alert users that the -maxtxfee setting is not used in non-wallet builds.

re: #15778 (comment)

You're right, I just didn't pay attention to the dummywallet change. I think it probably shouldn't be an error to specify -maxtxfee, based on the way other wallet options are handled, though I could see reasons for doing it either way.

ryanofsky · 2019-04-17T15:29:58Z

src/wallet/init.cpp

@@ -124,10 +126,6 @@ bool WalletInit::ParameterInteraction() const
    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."));

-    if (::minRelayTxFee.GetFeePerK() > HIGH_TX_FEE_PER_KB)
-        InitWarning(AmountHighWarn("-minrelaytxfee") + " " +


I can see how it makes sense to disable this particular warning about the -minrelaytxfee setting in non-wallet builds even though it is not a wallet setting, because the warning message is about wallet behavior.

But would it make sense to to keep some warning about really high -minrelaytxfee settings even in non-wallet builds?

The warning was added as a wallet warning in #8486.

This PR is meant to be a pure refactor (except for the updated log message) so I'm not going to add the suggested new warning for a high -minrelaytxfee to the node, but agree that it could make sense to add it in a separate PR. I'd also suggest that in a follow-up PR we change this warning to only trigger if -minrelayfee is greater than walletInstance->m_default_max_tx_fee.

EDIT: these warnings will now show for each wallet at startup rather than just once for the node, so that's also a minor behaviour change when starting with multiple wallets.

maflcko · 2019-04-18T15:34:42Z

For unknown reasons GitHub delivers a corrupt branch when fetching this pull request. This leads to erroneous conflict calculations in DrahtBot, and the ci systems unable to run.

8c3e6f9 is delivered, whereas this pull request was force pushed in the meantime.

This commit moves the maxtxfee setting to the wallet. There is only one minor behavior change: - an error message in feebumper now refers to -maxtxfee instead of maxTxFee.

ryanofsky

utACK 5c759c7. Changes since last review: updated commit message and an error message and method name.

meshcollider

utACK 5c759c7

meshcollider · 2019-04-27T03:45:00Z

src/wallet/wallet.cpp

@@ -4204,6 +4204,29 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
            return nullptr;
        }
    }
+
+    if (gArgs.IsArgSet("-maxtxfee"))
+    {


μ-nit: bracket

Didn't catch this when moving the code from init.cpp. Sorry!

maflcko · 2019-04-27T13:28:37Z

utACK 5c759c7

Show signature

5c759c7 [wallet] Move maxTxFee to wallet (John Newbery) Pull request description: Closes bitcoin#15355 Moves the `-maxtxfee` from the node to the wallet. See discussion in issue for details. This is a cleanup. There is no change in behaviour. Completes bitcoin#15620 ACKs for commit 5c759c: MarcoFalke: utACK 5c759c7 ryanofsky: utACK 5c759c7. Changes since last review: updated commit message and an error message and method name. meshcollider: utACK bitcoin@5c759c7 Tree-SHA512: 2f9b2729da3940a5cda994d3f3bc11ee1a52fcc1c5e9842ea0ea63e4eb0300e8416853046776311298bc449ba07554aa46f0f245ce28598a5b0bd7347c12e752

Summary: This commit moves the maxtxfee setting to the wallet. There is only one minor behavior change: - an error message in feebumper now refers to -maxtxfee instead of maxTxFee. Backport of Core [[bitcoin/bitcoin#15778 | PR15778]] Depends on D6266 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6267

…llet 5c759c7 [wallet] Move maxTxFee to wallet (John Newbery) Pull request description: Closes bitcoin#15355 Moves the `-maxtxfee` from the node to the wallet. See discussion in issue for details. This is a cleanup. There is no change in behaviour. Completes bitcoin#15620 ACKs for commit 5c759c: MarcoFalke: utACK 5c759c7 ryanofsky: utACK 5c759c7. Changes since last review: updated commit message and an error message and method name. meshcollider: utACK bitcoin@5c759c7 Tree-SHA512: 2f9b2729da3940a5cda994d3f3bc11ee1a52fcc1c5e9842ea0ea63e4eb0300e8416853046776311298bc449ba07554aa46f0f245ce28598a5b0bd7347c12e752

jnewbery changed the title ~~Mmove maxtxfee from node~~ Move maxtxfee from node to wallet Apr 9, 2019

jnewbery changed the title ~~Move maxtxfee from node to wallet~~ [wallet] Move maxtxfee from node to wallet Apr 9, 2019

meshcollider added the Wallet label Apr 9, 2019

DrahtBot added the Needs rebase label Apr 9, 2019

jnewbery force-pushed the remove_maxtxfeerate_from_node branch from 71862f4 to e3abaa3 Compare April 10, 2019 13:59

fanquake removed the Needs rebase label Apr 10, 2019

jnewbery closed this Apr 10, 2019

jnewbery reopened this Apr 10, 2019

jnewbery force-pushed the remove_maxtxfeerate_from_node branch from e3abaa3 to bef4ce8 Compare April 10, 2019 21:27

maflcko reviewed Apr 11, 2019

View reviewed changes

src/wallet/wallet.h Outdated Show resolved Hide resolved

src/wallet/wallet.h Outdated Show resolved Hide resolved

ryanofsky reviewed Apr 11, 2019

View reviewed changes

src/interfaces/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.h Outdated Show resolved Hide resolved

jnewbery force-pushed the remove_maxtxfeerate_from_node branch from bef4ce8 to 8c3e6f9 Compare April 11, 2019 15:54

jnewbery force-pushed the remove_maxtxfeerate_from_node branch from 8c3e6f9 to 65b6d9c Compare April 11, 2019 16:41

jnewbery force-pushed the remove_maxtxfeerate_from_node branch from 65b6d9c to a9706d1 Compare April 11, 2019 16:55

jnewbery mentioned this pull request Apr 12, 2019

[WIP] mempool: Remove nAbsurdFee fee from AcceptToMemoryPool #15810

Closed

ryanofsky approved these changes Apr 17, 2019

View reviewed changes

[wallet] Move maxTxFee to wallet

5c759c7

This commit moves the maxtxfee setting to the wallet. There is only one minor behavior change: - an error message in feebumper now refers to -maxtxfee instead of maxTxFee.

maflcko closed this Apr 18, 2019

maflcko reopened this Apr 18, 2019

jnewbery force-pushed the remove_maxtxfeerate_from_node branch from a9706d1 to 5c759c7 Compare April 18, 2019 15:46

ryanofsky approved these changes Apr 18, 2019

View reviewed changes

maflcko mentioned this pull request Apr 19, 2019

Feedback for GitHub CEO #15847

Closed

meshcollider reviewed Apr 27, 2019

View reviewed changes

maflcko merged commit 5c759c7 into bitcoin:master Apr 27, 2019

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 1, 2019

Merge bitcoin#15778: [wallet] Move maxtxfee from node to wallet

7c3aaab

kwvg added a commit to kwvg/dash that referenced this pull request Dec 3, 2021

merge bitcoin#15778: Move maxtxfee from node to wallet

e119297

kwvg added a commit to kwvg/dash that referenced this pull request Dec 4, 2021

merge bitcoin#15778: Move maxtxfee from node to wallet

eef7827

kwvg added a commit to kwvg/dash that referenced this pull request Dec 6, 2021

merge bitcoin#15778: Move maxtxfee from node to wallet

54b98c3

kwvg added a commit to kwvg/dash that referenced this pull request Dec 8, 2021

merge bitcoin#15778: Move maxtxfee from node to wallet

1c83e7e

kwvg added a commit to kwvg/dash that referenced this pull request Dec 8, 2021

merge bitcoin#15778: Move maxtxfee from node to wallet

82e9656

kwvg added a commit to kwvg/dash that referenced this pull request Dec 8, 2021

merge bitcoin#15778: Move maxtxfee from node to wallet

5c3b913

kwvg added a commit to kwvg/dash that referenced this pull request Dec 11, 2021

merge bitcoin#15778: Move maxtxfee from node to wallet

0abe776

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

merge bitcoin#15778: Move maxtxfee from node to wallet

4d6e671

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

merge bitcoin#15778: Move maxtxfee from node to wallet

d92370b

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

merge bitcoin#15778: Move maxtxfee from node to wallet

6a08bdd

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

merge bitcoin#15778: Move maxtxfee from node to wallet

17ca31d

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

merge bitcoin#15778: Move maxtxfee from node to wallet

e3f9145

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

merge bitcoin#15778: Move maxtxfee from node to wallet

77d4c51

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

[wallet] Move maxtxfee from node to wallet #15778

[wallet] Move maxtxfee from node to wallet #15778

jnewbery commented Apr 9, 2019 •

edited

jnewbery commented Apr 9, 2019 •

edited

maflcko commented Apr 9, 2019

jnewbery commented Apr 9, 2019

DrahtBot commented Apr 9, 2019

jnewbery commented Apr 10, 2019

jnewbery commented Apr 10, 2019

maflcko commented Apr 10, 2019

DrahtBot commented Apr 10, 2019 •

edited

jnewbery commented Apr 10, 2019 •

edited

maflcko left a comment

ryanofsky left a comment

jnewbery commented Apr 11, 2019

ryanofsky commented Apr 11, 2019

jnewbery commented Apr 11, 2019 •

edited

ryanofsky left a comment

ryanofsky Apr 17, 2019

jnewbery Apr 18, 2019

ryanofsky Apr 17, 2019

jnewbery Apr 18, 2019

ryanofsky Apr 17, 2019

jnewbery Apr 18, 2019

ryanofsky Apr 18, 2019

ryanofsky Apr 17, 2019

jnewbery Apr 18, 2019

jnewbery Apr 18, 2019

maflcko commented Apr 18, 2019

ryanofsky left a comment

meshcollider left a comment

meshcollider Apr 27, 2019 •

edited

jnewbery Apr 28, 2019

maflcko commented Apr 27, 2019

[wallet] Move maxtxfee from node to wallet #15778

[wallet] Move maxtxfee from node to wallet #15778

Conversation

jnewbery commented Apr 9, 2019 • edited

jnewbery commented Apr 9, 2019 • edited

maflcko commented Apr 9, 2019

jnewbery commented Apr 9, 2019

DrahtBot commented Apr 9, 2019

jnewbery commented Apr 10, 2019

jnewbery commented Apr 10, 2019

maflcko commented Apr 10, 2019

DrahtBot commented Apr 10, 2019 • edited

Conflicts

jnewbery commented Apr 10, 2019 • edited

maflcko left a comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

jnewbery commented Apr 11, 2019

ryanofsky commented Apr 11, 2019

jnewbery commented Apr 11, 2019 • edited

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko commented Apr 18, 2019

ryanofsky left a comment

Choose a reason for hiding this comment

meshcollider left a comment

Choose a reason for hiding this comment

meshcollider Apr 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko commented Apr 27, 2019

jnewbery commented Apr 9, 2019 •

edited

jnewbery commented Apr 9, 2019 •

edited

DrahtBot commented Apr 10, 2019 •

edited

jnewbery commented Apr 10, 2019 •

edited

jnewbery commented Apr 11, 2019 •

edited

meshcollider Apr 27, 2019 •

edited