[wallet] Move maxtxfee from node to wallet #15778

jnewbery commented at 8:18 PM on April 9, 2019: member

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 commented at 8:18 PM on April 9, 2019: member

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

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

jnewbery renamed this:
~~Mmove maxtxfee from node~~
Move maxtxfee from node to wallet
on Apr 9, 2019

jnewbery renamed this:
~~Move maxtxfee from node to wallet~~
[wallet] Move maxtxfee from node to wallet
on Apr 9, 2019

MarcoFalke commented at 8:26 PM on April 9, 2019: member

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 commented at 8:49 PM on April 9, 2019: member

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

meshcollider added the label Wallet on Apr 9, 2019

DrahtBot commented at 10:25 PM on April 9, 2019: member

Needs rebase

DrahtBot added the label Needs rebase on Apr 9, 2019

jnewbery commented at 1:23 PM on April 10, 2019: member

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 force-pushed on Apr 10, 2019

jnewbery commented at 1:59 PM on April 10, 2019: member

Rebased on master. This is ready for review.

fanquake removed the label Needs rebase on Apr 10, 2019

MarcoFalke commented at 2:48 PM on April 10, 2019: member

Many tests fail. Do they pass locally in valgrind?

DrahtBot commented at 3:06 PM on April 10, 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:

#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 commented at 6:30 PM on April 10, 2019: member

Many tests fail.

You're right. Investigating now.

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

jnewbery closed this on Apr 10, 2019

jnewbery reopened this on Apr 10, 2019

jnewbery force-pushed on Apr 10, 2019

in src/wallet/wallet.h:77 in bef4ce8ca9 outdated

  72 | @@ -73,6 +73,11 @@ static const unsigned int DEFAULT_TX_CONFIRM_TARGET = 6;
  73 |  static const bool DEFAULT_WALLET_RBF = false;
  74 |  static const bool DEFAULT_WALLETBROADCAST = true;
  75 |  static const bool DEFAULT_DISABLE_WALLET = false;
  76 | +//! -maxtxfee default
  77 | +constexpr CAmount DEFAULT_TRANSACTION_MAXFEE = COIN / 10;

MarcoFalke commented at 1:54 PM on April 11, 2019:

constexpr CAmount DEFAULT_TRANSACTION_MAXFEE{COIN / 10};

Could use c++11 to assert the result fits into CAmount without truncation?

jnewbery commented at 3:54 PM on April 11, 2019:

fixed

in src/wallet/wallet.h:80 in bef4ce8ca9 outdated

  72 | @@ -73,6 +73,11 @@ static const unsigned int DEFAULT_TX_CONFIRM_TARGET = 6;
  73 |  static const bool DEFAULT_WALLET_RBF = false;
  74 |  static const bool DEFAULT_WALLETBROADCAST = true;
  75 |  static const bool DEFAULT_DISABLE_WALLET = false;
  76 | +//! -maxtxfee default
  77 | +constexpr CAmount DEFAULT_TRANSACTION_MAXFEE = COIN / 10;
  78 | +
  79 | +/** Absolute maximum transaction fee (in satoshis) used by wallet */
  80 | +extern CAmount g_max_tx_fee;

MarcoFalke commented at 1:57 PM on April 11, 2019:

Please no more globals :hand: :weary:

This will just force future refactoring changes to make it a wallet member, when we allow per-wallet settings.

See CWallet::m_min_fee and (wallet: Make fee settings to be non-static members #12909) for inspiration

ryanofsky commented at 2:45 PM on April 11, 2019:

re: #15778 (review)

See CWallet::m_min_fee and (wallet: Make fee settings to be non-static members #12909) for inspiration

A suggestion would be to call the setting m_default_max_tx_fee, since similar to m_default_address_type/m_default_change_type it's a default preference and something that could be overridden for individual wallet operations.

jnewbery commented at 2:52 PM on April 11, 2019:

Yes, I guess that makes sense and fits with what I originally wrote here: #13044. It just seems strange to me to have options which (currently) apply to all wallets stored in each individual CWallet (see also fBroadcastTransactions which is a CWallet member, but I don't think makes sense as a per-wallet config).

I'll move max_tx_fee to live alongside the other wallet member fee settings.

jnewbery commented at 3:54 PM on April 11, 2019:

fixed

MarcoFalke commented at 1:57 PM on April 11, 2019: member

utACK, some style comments

in src/interfaces/wallet.cpp:237 in bef4ce8ca9 outdated

 233 | @@ -234,6 +234,7 @@ class WalletImpl : public Wallet
 234 |          LOCK(m_wallet->cs_wallet);
 235 |          return m_wallet->ListLockedCoins(outputs);
 236 |      }
 237 | +    CAmount getMaxTxFee() override { return g_max_tx_fee; }

ryanofsky commented at 2:34 PM on April 11, 2019:

Suggest moving this down below getDefaultAddressType() and getDefaultChangeType(), since these are other wallet settings the gui asks about.

jnewbery commented at 4:56 PM on April 11, 2019:

done

ryanofsky commented at 2:49 PM on April 11, 2019: member

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

jnewbery commented at 3:53 PM on April 11, 2019: member

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.

jnewbery force-pushed on Apr 11, 2019

ryanofsky commented at 4:40 PM on April 11, 2019: member

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 force-pushed on Apr 11, 2019

jnewbery commented at 4:41 PM on April 11, 2019: member

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

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

jnewbery force-pushed on Apr 11, 2019

in src/interfaces/wallet.cpp:467 in a9706d1134 outdated

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

ryanofsky commented at 3:05 PM on April 17, 2019:

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.

jnewbery commented at 3:46 PM on April 18, 2019:

done

in src/wallet/feebumper.cpp:167 in a9706d1134 outdated

 161 | @@ -162,9 +162,9 @@ Result CreateTransaction(const CWallet* wallet, const uint256& txid, const CCoin
 162 |      }
 163 |  
 164 |      // Check that in all cases the new fee doesn't violate maxTxFee
 165 | -     const CAmount max_tx_fee = wallet->chain().maxTxFee();
 166 | +     const CAmount max_tx_fee = wallet->m_default_max_tx_fee;
 167 |       if (new_fee > max_tx_fee) {
 168 | -         errors.push_back(strprintf("Specified or calculated fee %s is too high (cannot be higher than maxTxFee %s)",
 169 | +         errors.push_back(strprintf("Specified or calculated fee %s is too high (cannot be higher than maxtxfee %s)",

ryanofsky commented at 3:10 PM on April 17, 2019:

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.

jnewbery commented at 3:46 PM on April 18, 2019:

updated commit log

in src/init.cpp:505 in a9706d1134 outdated

 501 | @@ -502,8 +502,6 @@ void SetupServerArgs()
 502 |      gArgs.AddArg("-mocktime=<n>", "Replace actual time with <n> seconds since epoch (default: 0)", true, OptionsCategory::DEBUG_TEST);
 503 |      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);
 504 |      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);
 505 | -    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

ryanofsky commented at 3:19 PM on April 17, 2019:

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.

jnewbery commented at 3:30 PM on April 18, 2019:

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.

ryanofsky commented at 3:56 PM on April 18, 2019:

re: #15778 (review)

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.

in src/wallet/init.cpp:128 in a9706d1134 outdated

 125 | @@ -124,10 +126,6 @@ bool WalletInit::ParameterInteraction() const
 126 |      if (gArgs.GetArg("-prune", 0) && gArgs.GetBoolArg("-rescan", false))
 127 |          return InitError(_("Rescans are not possible in pruned mode. You will need to use -reindex which will download the whole blockchain again."));
 128 |  
 129 | -    if (::minRelayTxFee.GetFeePerK() > HIGH_TX_FEE_PER_KB)
 130 | -        InitWarning(AmountHighWarn("-minrelaytxfee") + " " +

ryanofsky commented at 3:29 PM on April 17, 2019:

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?

jnewbery commented at 3:45 PM on April 18, 2019:

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.

jnewbery commented at 3:46 PM on April 18, 2019:

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.

ryanofsky approved

ryanofsky commented at 3:37 PM on April 17, 2019: member

utACK a9706d1134b12584df8519383c621f990113a976

MarcoFalke commented at 3:34 PM on April 18, 2019: member

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.

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

[wallet] Move maxTxFee to wallet

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.

5c759c73b2

MarcoFalke closed this on Apr 18, 2019

MarcoFalke reopened this on Apr 18, 2019

jnewbery force-pushed on Apr 18, 2019

ryanofsky approved

ryanofsky commented at 4:00 PM on April 18, 2019: member

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

in src/wallet/wallet.cpp:4209 in 5c759c73b2

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

meshcollider commented at 3:45 AM on April 27, 2019:

μ-nit: bracket

jnewbery commented at 3:40 PM on April 28, 2019:

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

meshcollider commented at 3:49 AM on April 27, 2019: contributor

utACK https://github.com/bitcoin/bitcoin/pull/15778/commits/5c759c73b2602c7fde1c50dbafe5525904c1b64c

MarcoFalke commented at 1:28 PM on April 27, 2019: member

utACK 5c759c73b2602c7fde1c50dbafe5525904c1b64c

<details><summary>Show signature</summary>

</details>

MarcoFalke referenced this in commit 3356799ee3 on Apr 27, 2019

MarcoFalke merged this on Apr 27, 2019

MarcoFalke closed this on Apr 27, 2019

sidhujag referenced this in commit 7c3aaab3d2 on May 1, 2019

deadalnix referenced this in commit 8a33d815de on May 27, 2020

vijaydasmp referenced this in commit e9cbf74c5e on Oct 30, 2021

vijaydasmp referenced this in commit 3a9c0dc4f0 on Oct 30, 2021

kittywhiskers referenced this in commit e119297937 on Dec 3, 2021

kittywhiskers referenced this in commit eef7827ed9 on Dec 4, 2021

kittywhiskers referenced this in commit 54b98c3f29 on Dec 6, 2021

kittywhiskers referenced this in commit 1c83e7e316 on Dec 8, 2021

kittywhiskers referenced this in commit 82e9656eea on Dec 8, 2021

kittywhiskers referenced this in commit 5c3b913241 on Dec 8, 2021

kittywhiskers referenced this in commit 0abe776925 on Dec 11, 2021

kittywhiskers referenced this in commit 4d6e671d78 on Dec 12, 2021

kittywhiskers referenced this in commit d92370b38d on Dec 12, 2021

kittywhiskers referenced this in commit 6a08bdd74c on Dec 12, 2021

kittywhiskers referenced this in commit 17ca31d1f6 on Dec 12, 2021

kittywhiskers referenced this in commit e3f9145775 on Dec 12, 2021

kittywhiskers referenced this in commit 77d4c51507 on Dec 12, 2021

MarcoFalke locked this on Dec 16, 2021