Disable default fallbackfee on mainnet #11882

jonasschnelli · 2017-12-12T22:40:45Z

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.

maflcko · 2017-12-12T23:14:47Z

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 · 2017-12-13T06:24:10Z

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 · 2017-12-13T06:44:12Z

I think walletallowfallbackfee should be disabled by default.

greenaddress · 2017-12-14T10:57:03Z

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.

maflcko · 2017-12-14T18:12:45Z

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 · 2017-12-14T18:58:00Z

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

maflcko · 2017-12-14T23:04:16Z

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 · 2017-12-15T06:04:09Z

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

That would disable the fallback fee by default. Right?

maflcko · 2017-12-15T19:16:41Z

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

morcos · 2017-12-15T19:47:28Z

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 · 2017-12-15T19:48:55Z

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

jonasschnelli · 2017-12-15T19:50:43Z

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

morcos · 2017-12-15T19:53:04Z

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

maflcko · 2017-12-15T20:07:35Z

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 · 2017-12-16T06:48:00Z

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 · 2017-12-16T13:01:13Z

@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 · 2017-12-19T23:25:09Z

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

maflcko · 2017-12-20T15:26:39Z

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

maflcko · 2017-12-22T09:15:44Z

test/functional/wallet-fallbackfee.py

+        self.start_node(0, extra_args=["-fallbackfee=0"])
+        addr = self.nodes[0].getnewaddress()
+        assert_raises_rpc_error(-4, "Fee estimation failed", self.nodes[0].sendtoaddress, addr, 1)
+


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

Thanks.
Added those tests.

maflcko · 2017-12-25T15:43:08Z

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

promag

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?

promag · 2018-02-05T14:09:58Z

test/functional/wallet-fallbackfee.py

+
+        # test sending a tx with disabled fallback fee (must fail)
+        self.stop_nodes()
+        self.start_node(0, extra_args=["-fallbackfee=0"])


Remove stop above and change here to restart.

jonasschnelli · 2018-02-09T07:09:55Z

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.

maflcko · 2018-02-10T19:56:03Z

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

jonasschnelli · 2018-02-12T10:19:11Z

Fixed the test name (uses now underscore).

promag · 2018-02-12T11:33:23Z

src/wallet/init.cpp

@@ -15,6 +16,9 @@

 std::string GetWalletHelpString(bool showDebug)
 {
+    const auto defaultChainParams = CreateChainParams(CBaseChainParams::MAIN);


Unused, remove? same below.

promag · 2018-02-12T11:34:55Z

src/wallet/init.cpp

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


Nit

g_wallet_allow_fallback_fee = nFeePerK != 0;

promag · 2018-02-12T11:35:35Z

test/functional/wallet_fallbackfee.py

+        assert_raises_rpc_error(-4, "Fee estimation failed", lambda: self.nodes[0].fundrawtransaction(self.nodes[0].createrawtransaction([], {self.nodes[0].getnewaddress(): 1})))
+        assert_raises_rpc_error(-4, "Fee estimation failed", lambda: self.nodes[0].sendfrom("", self.nodes[0].getnewaddress(), 1))
+        assert_raises_rpc_error(-6, "Fee estimation failed", lambda: self.nodes[0].sendmany("", {self.nodes[0].getnewaddress(): 1}))
+if __name__ == '__main__':


Missing newline before.

promag · 2018-02-12T11:38:10Z

src/chainparams.h

@@ -91,6 +93,7 @@ class CChainParams
    bool fMineBlocksOnDemand;
    CCheckpointData checkpointData;
    ChainTxData chainTxData;
+    bool m_fallback_fee_enabled;


Nit, could be after fMineBlocksOnDemand above.

Can you explain why?

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.

promag · 2018-02-12T11:39:32Z

src/wallet/init.cpp

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


Nit, shorter:

g_wallet_allow_fallback_fee = Params().IsFallbackFeeEnabled();

promag · 2018-02-12T11:41:13Z

src/wallet/wallet.cpp

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


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

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

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

jonasschnelli · 2018-02-25T01:39:04Z

Fixed points reported by @promag

maflcko · 2018-02-25T16:14:27Z

Only changes were feedback addressed by promag.

promag · 2018-02-25T21:24:13Z

utACK 3f592b8.

3f592b8 [QA] add wallet-rbf test (Jonas Schnelli) 8222e05 Disable wallet fallbackfee by default on mainnet (Jonas Schnelli) Pull request description: 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. Tree-SHA512: e54d2594b7f954e640cc513a18b0bfbe189f15e15bdeed4fe02b7677f939bca1731fef781b073127ffd4ce08a595fb118259b8826cdaa077ff7d5ae9495810db

New global variables were introduced in bitcoin#11882 and not setting them causes: wallet/test/wallet_tests.cpp(638): error in "ListCoins": check wallet->CreateTransaction({recipient}, wtx, reservekey, fee, changePos, error, dummy) failed wallet/test/wallet_tests.cpp(679): error in "ListCoins": check list.begin()->second.size() == 2 failed [1 != 2] wallet/test/wallet_tests.cpp(686): error in "ListCoins": check available.size() == 2 failed [1 != 2] wallet/test/wallet_tests.cpp(705): error in "ListCoins": check list.begin()->second.size() == 2 failed [1 != 2] It's possible to reproduce the failure reliably by running: src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/ListCoins Failures happen nondeterministically because boost test framework doesn't run tests in a specified order, and tests that run previously can set the global variables and mask the bug.

…fallback_fee 7ba2d57 Fix ListCoins test failure due to unset g_wallet_allow_fallback_fee (Russell Yanofsky) Pull request description: New global variable was introduced in #11882 and not setting it causes: ``` wallet/test/wallet_tests.cpp(638): error in "ListCoins": check wallet->CreateTransaction({recipient}, wtx, reservekey, fee, changePos, error, dummy) failed wallet/test/wallet_tests.cpp(679): error in "ListCoins": check list.begin()->second.size() == 2 failed [1 != 2] wallet/test/wallet_tests.cpp(686): error in "ListCoins": check available.size() == 2 failed [1 != 2] wallet/test/wallet_tests.cpp(705): error in "ListCoins": check list.begin()->second.size() == 2 failed [1 != 2] ``` It's possible to reproduce the failure reliably by running: ``` src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/ListCoins ``` Failures happen nondeterministically because boost test framework doesn't run tests in a specified order, and tests that run previously can set the global variables and mask the bug. This is similar to bugs #12150 and #12424. Example travis failures are: https://travis-ci.org/bitcoin/bitcoin/jobs/348296805#L2676 https://travis-ci.org/bitcoin/bitcoin/jobs/348362560#L2769 https://travis-ci.org/bitcoin/bitcoin/jobs/348362563#L2824 Tree-SHA512: ca37b554a75c12ac2d534de62bf74eb9e0b29e4399ebf1fa10053a40887e55e9e7135f754a01e5a67499cc8677ae226542146b370b1e83d08bb63d79ff379073

New global variables were introduced in bitcoin#11882 and not setting them causes: wallet/test/wallet_tests.cpp(638): error in "ListCoins": check wallet->CreateTransaction({recipient}, wtx, reservekey, fee, changePos, error, dummy) failed wallet/test/wallet_tests.cpp(679): error in "ListCoins": check list.begin()->second.size() == 2 failed [1 != 2] wallet/test/wallet_tests.cpp(686): error in "ListCoins": check available.size() == 2 failed [1 != 2] wallet/test/wallet_tests.cpp(705): error in "ListCoins": check list.begin()->second.size() == 2 failed [1 != 2] It's possible to reproduce the failure reliably by running: src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/ListCoins Failures happen nondeterministically because boost test framework doesn't run tests in a specified order, and tests that run previously can set the global variables and mask the bug.

jonasschnelli added the Wallet label Dec 12, 2017

jonasschnelli force-pushed the 2017/12/feeest_readyness branch 2 times, most recently from 69f76ba to 0cc0c25 Compare December 12, 2017 22:44

jonasschnelli force-pushed the 2017/12/feeest_readyness branch from 0cc0c25 to 9aa8a67 Compare December 13, 2017 07:07

jonasschnelli mentioned this pull request Dec 14, 2017

[Qt] Warn if fallback fee has been used #11892

Closed

jonasschnelli force-pushed the 2017/12/feeest_readyness branch from 9aa8a67 to 7f12981 Compare December 16, 2017 06:43

maflcko mentioned this pull request Dec 16, 2017

fees: Remove fallbackfee default #11918

Closed

1 task

jonasschnelli force-pushed the 2017/12/feeest_readyness branch from 7f12981 to 9bb0390 Compare December 19, 2017 23:22

jonasschnelli changed the title ~~Improve fallback fee situations~~ Disable default fallbackfee on mainnet Dec 19, 2017

jonasschnelli force-pushed the 2017/12/feeest_readyness branch from 9bb0390 to 7941d0e Compare December 20, 2017 06:36

maflcko mentioned this pull request Dec 20, 2017

cost too many fees? #10247

Closed

jonasschnelli force-pushed the 2017/12/feeest_readyness branch from 7941d0e to edcc468 Compare December 20, 2017 19:34

maflcko reviewed Dec 22, 2017

View reviewed changes

jonasschnelli force-pushed the 2017/12/feeest_readyness branch from 8ba3ffb to 93ba98a Compare December 22, 2017 19:36

maflcko added the Needs release note label Dec 25, 2017

promag reviewed Feb 5, 2018

View reviewed changes

jonasschnelli force-pushed the 2017/12/feeest_readyness branch from 93ba98a to a357763 Compare February 9, 2018 07:06

jonasschnelli force-pushed the 2017/12/feeest_readyness branch from a357763 to 0d51422 Compare February 12, 2018 10:18

promag reviewed Feb 12, 2018

View reviewed changes

jonasschnelli force-pushed the 2017/12/feeest_readyness branch from 0d51422 to 3e02133 Compare February 25, 2018 01:27

Disable wallet fallbackfee by default on mainnet

8222e05

jonasschnelli force-pushed the 2017/12/feeest_readyness branch from 3e02133 to 2876598 Compare February 25, 2018 01:34

[QA] add wallet-rbf test

3f592b8

jonasschnelli force-pushed the 2017/12/feeest_readyness branch from 2876598 to 3f592b8 Compare February 25, 2018 01:39

laanwj merged commit 3f592b8 into bitcoin:master Mar 1, 2018

ryanofsky mentioned this pull request Mar 2, 2018

Fix ListCoins test failure due to unset g_wallet_allow_fallback_fee #12582

Merged

fanquake removed the Needs release note label Mar 20, 2019

Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Nov 30, 2021

Merge bitcoin#11882: Disable wallet fallbackfee by default on mainnet

d05dbb5

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Disable default fallbackfee on mainnet #11882

Disable default fallbackfee on mainnet #11882

Uh oh!

Conversation

jonasschnelli commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Dec 12, 2017

Uh oh!

jonasschnelli commented Dec 13, 2017

Uh oh!

NicolasDorier commented Dec 13, 2017

Uh oh!

greenaddress commented Dec 14, 2017

Uh oh!

maflcko commented Dec 14, 2017

Uh oh!

jonasschnelli commented Dec 14, 2017

Uh oh!

maflcko commented Dec 14, 2017

Uh oh!

jonasschnelli commented Dec 15, 2017

Uh oh!

maflcko commented Dec 15, 2017

Uh oh!

morcos commented Dec 15, 2017

Uh oh!

jonasschnelli commented Dec 15, 2017

Uh oh!

jonasschnelli commented Dec 15, 2017

Uh oh!

morcos commented Dec 15, 2017

Uh oh!

maflcko commented Dec 15, 2017

Uh oh!

jonasschnelli commented Dec 16, 2017

Uh oh!

morcos commented Dec 16, 2017

Uh oh!

jonasschnelli commented Dec 19, 2017

Uh oh!

maflcko commented Dec 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 25, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Feb 9, 2018

Uh oh!

maflcko commented Feb 10, 2018

Uh oh!

jonasschnelli commented Feb 12, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Dec 12, 2017 •

edited

Loading