Disable default fallbackfee on mainnet #11882

jonasschnelli commented at 10:40 PM on December 12, 2017: contributor

Removes the default fallback fee on mainnet (but keeps it on testnet/regtest).

Transactions using the fallbackfee in case the fallback fee has not been set are getting rejected.

jonasschnelli added the label Wallet on Dec 12, 2017

jonasschnelli force-pushed on Dec 12, 2017

MarcoFalke commented at 11:14 PM on December 12, 2017: member

Concept ACK on setting the rbf flag on transactions where it is unclear if they confirm with the default fallback fee fee. Not yet sure about adding a command line arg just to disable and disallow fallbackfee.

jonasschnelli commented at 6:24 AM on December 13, 2017: contributor

Not yet sure about adding a command line arg just to disable and disallow fallbackfee.

Using a static (!) fallback fee seems an incorrect concept. It will very likely result in significant overpay or significant underpay (==stuck transaction). I would strongly recommend to either reject during the time when no estimations are available or to collect fee estimations from somewhere else (probably controversial).

I would even vote for defaulting walletallowfallbackfee to true (which this PR does not, it just offers the option to disable it).

NicolasDorier commented at 6:44 AM on December 13, 2017: contributor

I think walletallowfallbackfee should be disabled by default.

jonasschnelli force-pushed on Dec 13, 2017

greenaddress commented at 10:57 AM on December 14, 2017: contributor

If fee estimates are not available perhaps is best to ask the user to wait or to manually set a fee rate (if they are a sufficiently advanced user, maybe even behind a config flag to reduce mistakes). BIP125 replacement enabled helps only the case when you underpay and does nothing for overpaying.

MarcoFalke commented at 6:12 PM on December 14, 2017: member

Well, instead of adding a command line arg to disable fallbackfee, why not remove the default value for fallbackfee and require that it is set whenever it is used?

jonasschnelli commented at 6:58 PM on December 14, 2017: contributor

Removing the fallback fee entirely would be an abrupt task and it would probably make regression tests more difficult. Reasonable steps are:

Allow to disable (but enable by default)
Disable by default
Eventually remove

MarcoFalke commented at 11:04 PM on December 14, 2017: member

I think I am bad at choosing words. Let me explain by example:

fallbackfee not set in conf or on command line -> Consider as fallbackfee disabled
fallbackfee set in conf or on command line -> Consider fallbackfee enabled

jonasschnelli commented at 6:04 AM on December 15, 2017: contributor

fallbackfee not set in conf or on command line -> Consider as fallbackfee disabled

That would disable the fallback fee by default. Right?

MarcoFalke commented at 7:16 PM on December 15, 2017: member

Jup, if we want to disable by default, I'd prefer that way.

morcos commented at 7:47 PM on December 15, 2017: member

Agree with @MarcoFalke

we already have too many configuration options if you want to use fallbackfee, then set one, otherwise you don't get one.

Sometimes we err too much on maintaining every aspect of backwards compatibility.
Just tell people in the release notes that RPC and GUI will now refuse to send when fee estimates are not initialized UNLESS you specifically instruct what feerate to fallback to.

We also already have the option to include the fee on a per transaction basis .

jonasschnelli commented at 7:48 PM on December 15, 2017: contributor

How would we handle regression tests (enable fallbackfee by default there)?

jonasschnelli commented at 7:50 PM on December 15, 2017: contributor

However, I'm fine with skipping step 1) (allow to disable) and directly disable it by default (with allow to enable).

morcos commented at 7:53 PM on December 15, 2017: member

Thats a good question. Maybe for testnet and regtest we have fallbackfee enabled by default? I'm not sure.. It does seem we're prone to errors that aren't caught by testing that way, but fundamentally testing fee estimation is hard. Another option is to just hard code in a fee estimate file for the functional tests that use the cached directory. But you'd still have the problem for all the others...

MarcoFalke commented at 8:07 PM on December 15, 2017: member

How would we handle regression tests (enable fallbackfee by default there)?

Jup, that makes sense to me. Don't worry too much about the tests for now, you can ignore the travis result and then later push a test-fix commit that follows your implementation and is acceptable to everyone.

jonasschnelli force-pushed on Dec 16, 2017

jonasschnelli commented at 6:48 AM on December 16, 2017: contributor

Changes this PR so it disables the fallback fee on mainnet, but keeps it enabled on testnet/regtest. Kept the option to change the state (renamed to enablefallbackfee=<state>).
Renamed the -walletfallbackfeerbf125 to fallbackfeerbf125.

morcos commented at 1:01 PM on December 16, 2017: member

@jonasschnelli Can we please get rid of both of the new options?

I haven't understood why they are necessary.

We already have -fallbackfee, it seems to me if you set that then you pretty clearly want to enable it and it doesn't make sense to have an option to enable fallbackfee without explicitly specifying what you want it to be.

-fallbackfeerbf125 also seems unnecessary. In the GUI, we'll already have the warning and can set rbf on a per transaction basis. I think we can just warn people using RPC that if they set a fallbackfee and don't have rbf set, then they are taking some risk. Unlike the prior option, this at least serves some purpose, but its just so esoteric that I think it's essentially clutter.

jonasschnelli force-pushed on Dec 19, 2017

jonasschnelli renamed this:
~~Improve fallback fee situations~~
Disable default fallbackfee on mainnet
on Dec 19, 2017

jonasschnelli commented at 11:25 PM on December 19, 2017: contributor

Followed @morcos advice. This does now simply disable the default fallback fee on mainnet (keeps it on testnet/regtest). Transactions using the fallbackfee in case the fallback fee has not been set are getting rejected (on mainnet).

jonasschnelli force-pushed on Dec 20, 2017

MarcoFalke commented at 3:26 PM on December 20, 2017: member

@jonasschnelli Needs rebase if still relevant. Also see #11918

jonasschnelli force-pushed on Dec 20, 2017

in src/wallet/fees.cpp:58 in edcc468550 outdated

  52 | @@ -53,6 +53,9 @@ CAmount GetMinimumFee(unsigned int nTxBytes, const CCoinControl& coin_control, c
  53 |              // if we don't have enough data for estimateSmartFee, then use fallbackFee
  54 |              fee_needed = CWallet::fallbackFee.GetFee(nTxBytes);
  55 |              if (feeCalc) feeCalc->reason = FeeReason::FALLBACK;
  56 | +
  57 | +            // directly return if fallback fee is disabled (feerate 0 == disabled)
  58 | +            if (CWallet::fallbackFee.GetFee(1000) == 0) return fee_needed;

MarcoFalke commented at 8:26 PM on December 20, 2017:

shorter, faster and clearer:

if (fee_needed == 0)

jonasschnelli commented at 6:41 AM on December 22, 2017:

fee_needed == 0 seems the wrong check, it could be 0 in case you would run through with nTxBytes=0, very unlikely, but testing the fallbackfee instead the current fee_needed seems correct.

MarcoFalke commented at 9:11 AM on December 22, 2017:

Right, should only matter for unit tests, but makes sense to keep it that way.

in src/wallet/fees.cpp:72 in edcc468550 outdated

  68 | @@ -66,7 +69,7 @@ CAmount GetMinimumFee(unsigned int nTxBytes, const CCoinControl& coin_control, c
  69 |      CAmount required_fee = GetRequiredFee(nTxBytes);
  70 |      if (required_fee > fee_needed) {
  71 |          fee_needed = required_fee;
  72 | -        if (feeCalc) feeCalc->reason = FeeReason::REQUIRED;
  73 | +        if (feeCalc && feeCalc->reason != FeeReason::FALLBACK) feeCalc->reason = FeeReason::REQUIRED;

MarcoFalke commented at 8:26 PM on December 20, 2017:

How is this not dead code?

jonasschnelli commented at 7:03 AM on December 22, 2017:

Why would that be dead code?

MarcoFalke commented at 9:11 AM on December 22, 2017:

Excuse my brevity, but I fail to see why fallbackfee is special cased here.

Let's assume the user sets paytxfee to one satoshi per byte, but the required fee is higher. It will correctly report that the required fee rate was used. Let's assume the user sets fallbackfee to something very low and the required fee is higher. It will incorrectly report in the debug.log that the fallbackfee was used, no? Since in this case g_wallet_allow_fallback_fee will be true, it won't incorrectly hit the check in wallet.cpp.

So if there is a reason to add this piece of code, please explain and/or add a comment.

jonasschnelli commented at 7:08 PM on December 22, 2017:

You right. It's no longer in use. The question here is, that we return a single flag where a bitset would be required. Reason can be FeeReason::REQUIRED but also (AND) FeeReason::FALLBACK.

This code part was required to enable RBF125 in case a fallback fee has been used regardless of the mintxrelayfee. But no longer relevant, will remove.

jonasschnelli commented at 7:36 PM on December 22, 2017:

Fixed.

in test/functional/wallet-fallbackfee.py:2 in edcc468550 outdated

   0 | @@ -0,0 +1,27 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2014-2016 The Bitcoin Core developers

MarcoFalke commented at 8:27 PM on December 20, 2017:

nit: off by two years, almost

jonasschnelli commented at 7:04 AM on December 22, 2017:

Oh. Yes. Fixed.

in test/functional/test_runner.py:129 in edcc468550 outdated

 129 | @@ -130,6 +130,7 @@
 130 |      'feature_logging.py',
 131 |      'node_network_limited.py',
 132 |      'conf_args.py',
 133 | +    'wallet-fallbackfee.py',

MarcoFalke commented at 8:28 PM on December 20, 2017:

Could move up by a few lines to avoid merge conflicts

jonasschnelli commented at 7:04 AM on December 22, 2017:

Fixed.

MarcoFalke commented at 8:28 PM on December 20, 2017: member

Just some style nits inline

laanwj commented at 11:23 AM on December 21, 2017: member

Concept ACK

jonasschnelli force-pushed on Dec 22, 2017

in test/functional/wallet-fallbackfee.py:25 in 8ba3ffba3c outdated

  20 | +        # test sending a tx with disabled fallback fee (must fail)
  21 | +        self.stop_nodes()
  22 | +        self.start_node(0, extra_args=["-fallbackfee=0"])
  23 | +        addr = self.nodes[0].getnewaddress()
  24 | +        assert_raises_rpc_error(-4, "Fee estimation failed", self.nodes[0].sendtoaddress, addr, 1)
  25 | +

MarcoFalke commented at 9:15 AM on December 22, 2017:

Could also add tests for the other rpc that potentially hit this error:

E.g.

        assert_raises_rpc_error(-4, "Fee estimation failed. Fallbackfee is disabled", lambda: self.nodes[0].fundrawtransaction(self.nodes[0].createrawtransaction([], {self.nodes[0].getnewaddress(): 1})))
        assert_raises_rpc_error(-4, "Fee estimation failed. Fallbackfee is disabled", lambda: self.nodes[0].sendfrom("", self.nodes[0].getnewaddress(), 1))
        assert_raises_rpc_error(-6, "Fee estimation failed. Fallbackfee is disabled", lambda: self.nodes[0].sendmany("", {self.nodes[0].getnewaddress(): 1}))

jonasschnelli commented at 7:36 PM on December 22, 2017:

Thanks. Added those tests.

jonasschnelli force-pushed on Dec 22, 2017

MarcoFalke commented at 3:43 PM on December 25, 2017: member

Thanks for adding the tests. I also slightly tested a previous version in the gui.

MarcoFalke added the label Needs release notes on Dec 25, 2017

in test/functional/wallet-fallbackfee.py:22 in 93ba98a6dc outdated

  17 | +        # sending a transaction without fee estimations must be possible by default on regtest
  18 | +        self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1)
  19 | +
  20 | +        # test sending a tx with disabled fallback fee (must fail)
  21 | +        self.stop_nodes()
  22 | +        self.start_node(0, extra_args=["-fallbackfee=0"])

promag commented at 2:09 PM on February 5, 2018:

Remove stop above and change here to restart.

promag commented at 2:23 PM on February 5, 2018: member

Needs rebase.

Concept ACK on setting the rbf flag on transactions where it is unclear if they confirm with the default fallback fee

I see 2 distinct options here: 1) automatically set RBF; 2) give error if coin control RBF is not set. From the UI point of view, being "preventive" is not that bad, I would go for option 2).

I think we can just warn people using RPC that if they set a fallbackfee and don't have rbf set, then they are taking some risk

Should update descriptions of send/fund RPC?

laanwj added this to the "Blockers" column in a project

jonasschnelli force-pushed on Feb 9, 2018

jonasschnelli commented at 7:09 AM on February 9, 2018: contributor

Rebased and fixed the test nit reported by @promag.

Concept ACK on setting the rbf flag on transactions where it is unclear if they confirm with the default fallback fee

I see 2 distinct options here: 1) automatically set RBF; 2) give error if coin control RBF is not set. From the UI point of view, being "preventive" is not that bad, I would go for option 2).

The idea 1) has been removed (was there in a previous version of Core): #11882 (comment) For 2) / GUI UX optimization, see #11892.

MarcoFalke commented at 7:56 PM on February 10, 2018: member

Should probably use underscore for the test name (see other test script names)

jonasschnelli force-pushed on Feb 12, 2018

jonasschnelli commented at 10:19 AM on February 12, 2018: contributor

Fixed the test name (uses now underscore).

in src/wallet/init.cpp:19 in 0d51422439 outdated

  15 | @@ -15,6 +16,9 @@
  16 |  
  17 |  std::string GetWalletHelpString(bool showDebug)
  18 |  {
  19 | +    const auto defaultChainParams = CreateChainParams(CBaseChainParams::MAIN);

promag commented at 11:33 AM on February 12, 2018:

Unused, remove? same below.

in src/wallet/init.cpp:142 in 0d51422439 outdated

 138 | @@ -132,6 +139,7 @@ bool WalletParameterInteraction()
 139 |              InitWarning(AmountHighWarn("-fallbackfee") + " " +
 140 |                          _("This is the transaction fee you may pay when fee estimates are not available."));
 141 |          CWallet::fallbackFee = CFeeRate(nFeePerK);
 142 | +        g_wallet_allow_fallback_fee = (nFeePerK == 0 ? false : true); //disable fallback fee in case value was set to 0, enable if non-null value

promag commented at 11:34 AM on February 12, 2018:

Nit

g_wallet_allow_fallback_fee = nFeePerK != 0;

in test/functional/wallet_fallbackfee.py:27 in 0d51422439 outdated

  21 | +        self.restart_node(0, extra_args=["-fallbackfee=0"])
  22 | +        assert_raises_rpc_error(-4, "Fee estimation failed", lambda: self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1))
  23 | +        assert_raises_rpc_error(-4, "Fee estimation failed", lambda: self.nodes[0].fundrawtransaction(self.nodes[0].createrawtransaction([], {self.nodes[0].getnewaddress(): 1})))
  24 | +        assert_raises_rpc_error(-4, "Fee estimation failed", lambda: self.nodes[0].sendfrom("", self.nodes[0].getnewaddress(), 1))
  25 | +        assert_raises_rpc_error(-6, "Fee estimation failed", lambda: self.nodes[0].sendmany("", {self.nodes[0].getnewaddress(): 1}))
  26 | +if __name__ == '__main__':

promag commented at 11:35 AM on February 12, 2018:

Missing newline before.

in src/chainparams.h:96 in 0d51422439 outdated

  92 | @@ -91,6 +93,7 @@ class CChainParams
  93 |      bool fMineBlocksOnDemand;
  94 |      CCheckpointData checkpointData;
  95 |      ChainTxData chainTxData;
  96 | +    bool m_fallback_fee_enabled;

promag commented at 11:38 AM on February 12, 2018:

Nit, could be after fMineBlocksOnDemand above.

jonasschnelli commented at 1:33 AM on February 25, 2018:

Can you explain why?

promag commented at 9:21 PM on February 25, 2018:

Take the following example:

#include <iostream>
using namespace std;

struct s1 {
  char a;
  int b;
};

struct s2 {
  char a;
  int b;
  char c;
};

struct s3 {
  char a;
  char c;
  int b;
};

int main() {
  cout << sizeof(s1) << endl;
  cout << sizeof(s2) << endl;
  cout << sizeof(s3) << endl;
  return 0;
}

on my system the output is

8
12
8

See http://www.catb.org/esr/structure-packing/#_structure_alignment_and_padding

But it's no big deal in this case, but should be taken into account when it's instanced a lot.

in src/wallet/init.cpp:132 in 0d51422439 outdated

 126 | @@ -123,6 +127,9 @@ bool WalletParameterInteraction()
 127 |                          _("This is the minimum transaction fee you pay on every transaction."));
 128 |          CWallet::minTxFee = CFeeRate(n);
 129 |      }
 130 | +
 131 | +    const CChainParams& chainParams = Params();
 132 | +    g_wallet_allow_fallback_fee = chainParams.IsFallbackFeeEnabled();

promag commented at 11:39 AM on February 12, 2018:

Nit, shorter:

g_wallet_allow_fallback_fee = Params().IsFallbackFeeEnabled();

in src/wallet/wallet.cpp:47 in 0d51422439 outdated

  43 | @@ -44,6 +44,7 @@ bool bSpendZeroConfChange = DEFAULT_SPEND_ZEROCONF_CHANGE;
  44 |  bool fWalletRbf = DEFAULT_WALLET_RBF;
  45 |  OutputType g_address_type = OUTPUT_TYPE_NONE;
  46 |  OutputType g_change_type = OUTPUT_TYPE_NONE;
  47 | +bool g_wallet_allow_fallback_fee = false; //<! will be defined via chainparams

promag commented at 11:41 AM on February 12, 2018:

Following the same concept of #12408, move to wallet member?

jonasschnelli commented at 1:36 AM on February 25, 2018:

Fallback fee yes or now seems to me after a global setting (not per wallet).

promag commented at 9:23 PM on February 25, 2018:

Ok then.

MarcoFalke commented at 3:33 PM on March 3, 2018:

In which case it could be a static member, just saying

jonasschnelli force-pushed on Feb 25, 2018

Disable wallet fallbackfee by default on mainnet 8222e057fe

jonasschnelli force-pushed on Feb 25, 2018

[QA] add wallet-rbf test 3f592b81dc

jonasschnelli commented at 1:39 AM on February 25, 2018: contributor

Fixed points reported by @promag

jonasschnelli force-pushed on Feb 25, 2018

MarcoFalke commented at 4:14 PM on February 25, 2018: member

Only changes were feedback addressed by promag.

promag commented at 9:24 PM on February 25, 2018: member

utACK 3f592b8.

laanwj merged this on Mar 1, 2018

laanwj closed this on Mar 1, 2018

laanwj referenced this in commit 987a80995a on Mar 1, 2018

laanwj removed this from the "Blockers" column in a project

ryanofsky referenced this in commit 7ba2d57852 on Mar 2, 2018

MarcoFalke referenced this in commit 6012f1caf7 on Mar 3, 2018

CyrusZhou-CN referenced this in commit b920af6e80 on Mar 3, 2018

HashUnlimited referenced this in commit ccc7d5defa on Jul 31, 2018

lionello referenced this in commit de4a0cb52d on Nov 7, 2018

fanquake removed the label Needs release note on Mar 20, 2019

Munkybooty referenced this in commit d05dbb5e2f on Nov 30, 2021

DrahtBot locked this on Dec 16, 2021