Fixes for bumpfee and minimum fees (use incrementalRelayFee for BIP 125 (RBF) replacement)

morcos commented at 9:22 pm on January 19, 2017: member

This commit includes several minor fixups from #9380.

Most importantly the change to AcceptToMemoryPoolWorker to use incrementalRelayFeewas an oversight that was left out of #9380.

bumpfee is adapted for that change and the changes from #9380, Also includes fixes for #8456

Please tag as a bug fix for 0.14

Edited to reflect additional changes.

in src/wallet/rpcwallet.cpp: in 71e6298361 outdated

2651@@ -2652,8 +2652,8 @@ UniValue bumpfee(const JSONRPCRequest& request)
2652             "By default, the new fee will be calculated automatically using estimatefee.\n"
2653             "The user can specify a confirmation target for estimatefee.\n"
2654             "Alternatively, the user can specify totalFee, or use RPC setpaytxfee to set a higher fee rate.\n"
2655-            "At a minimum, the new fee rate must be high enough to pay a new relay fee (relay fee amount returned\n"
2656-            "by getnetworkinfo RPC) and to enter the node's mempool.\n"

ryanofsky commented at 9:39 pm on January 19, 2017:

Would it be a good idea to update getnetworkinfo to return the incremental relay fee?

morcos commented at 1:03 am on January 20, 2017:

I think from a user perspective, it might be easier to keep incremental relay fee as something that is kind of unknown to them and is effectively just the hard coded 1000 sat / KB, however if we ever think it makes sense to have people changing it (or we think the default needs changing it), then it would be time to expose it. I realize that’s not a perfect solution, but I also think confusing people with too many random minimums isn’t ideal either… At least the error message for bumpfee outputs what the needed increment is..

ryanofsky commented at 1:33 am on January 20, 2017:

I think it’d be good to have this if only so this documentation can be less vague. If I’m trying to write an RPC call that passes a totalFee and the documentation just says I need to “pay a new relay fee”, what am I supposed to do at the point to find out what value I need to pass for the call to complete reliably? Search for “bitcoin relay fee”? Look at the source code? I think I’d be better off with documentation that says I need to increase the fee by at least the amount returned in getnetworkinfo.

morcos commented at 2:20 am on January 20, 2017:

Ok done

in src/wallet/rpcwallet.cpp: in fa005cce2b outdated

2833@@ -2834,7 +2834,7 @@ UniValue bumpfee(const JSONRPCRequest& request)
2834 
2835     // If the output would become dust, discard it (converting the dust to fee)
2836     poutput->nValue -= nDelta;
2837-    if (poutput->nValue <= poutput->GetDustThreshold(::minRelayTxFee)) {
2838+    if (poutput->nValue <= poutput->GetDustThreshold(dustRelayFee)) {

ryanofsky commented at 9:48 pm on January 19, 2017:

Not directly related, but CTxOut IsDust and GetDustThreshold methods still take arguments named minRelayTxFee. Would seem like a good idea to rename those to dustRelayFee.

morcos commented at 1:00 am on January 20, 2017:

yes, that has annoyed me a few times as well and I was going to change it, but @jtimon has #9279 which I was hoping would eventually get merged and make it go away all together.

in src/wallet/rpcwallet.cpp: in fa005cce2b outdated

2803@@ -2804,9 +2804,9 @@ UniValue bumpfee(const JSONRPCRequest& request)
2804             nNewFeeRate = CWallet::fallbackFee;
2805         }
2806 
2807-        // new fee rate must be at least old rate + minimum relay rate
2808-        if (nNewFeeRate.GetFeePerK() < nOldFeeRate.GetFeePerK() + ::minRelayTxFee.GetFeePerK()) {

ryanofsky commented at 9:51 pm on January 19, 2017:

Consider keeping the :: prefix for global variables. As someone new to the code who doesn’t have all the global variables memorized, I think the prefix is very helpful for readability, especially in a large function like bumpfee with similar sounding local variables.

morcos commented at 0:58 am on January 20, 2017:

ok i don’t mind doing that, but will just do for the changes introduced in this PR here?

ryanofsky commented at 1:39 am on January 20, 2017:

Great, that sounds fine to me.

ryanofsky approved

ryanofsky commented at 9:52 pm on January 19, 2017: member

utACK fa005cce2b7a8e02cedc7356d2d1933b94c26c39

MarcoFalke commented at 0:20 am on January 20, 2017: member

utACK fa005cc, agree with the nits.

MarcoFalke added this to the milestone 0.14.0 on Jan 20, 2017

MarcoFalke added the label TX fees and policy on Jan 20, 2017

Use incrementalRelayFee for BIP 125 replacement 5b158707f2

Fix missing use of dustRelayFee de6400de5d

morcos force-pushed on Jan 20, 2017

morcos commented at 1:32 am on January 20, 2017: member

Only change is adding :: to global variables

morcos commented at 2:21 am on January 20, 2017: member

Took @ryanofsky’s suggestion to add the incremental fee to getnetworkinfo and also added a commit that fixed an issue with block min tx fee in a test.

morcos renamed this:
~~Use incrementalRelayFee for BIP 125 (RBF) replacement logic~~
Fixes for minimum fees (use incrementalRelayFee for BIP 125 (RBF) replacement)
on Jan 20, 2017

Fix to have miner test aware of new separate block min tx fee 6b331e6cf9

morcos force-pushed on Jan 20, 2017

in src/wallet/rpcwallet.cpp: in 72af459a53 outdated

2651@@ -2652,8 +2652,8 @@ UniValue bumpfee(const JSONRPCRequest& request)
2652             "By default, the new fee will be calculated automatically using estimatefee.\n"
2653             "The user can specify a confirmation target for estimatefee.\n"
2654             "Alternatively, the user can specify totalFee, or use RPC setpaytxfee to set a higher fee rate.\n"
2655-            "At a minimum, the new fee rate must be high enough to pay a new relay fee over the original fee\n"
2656-            "to enter the node's mempool.\n"
2657+            "At a minimum, the new fee rate must be high enough to pay an additional new relay fee (incrementalfee\n"
2658+            "returned by getnetowrkinfo) to enter the node's mempool.\n"

ryanofsky commented at 5:18 am on January 20, 2017:

typo getnetowrkinfo

ryanofsky approved

ryanofsky commented at 5:18 am on January 20, 2017: member

utACK 72af459a53f83d507aa83c93cd3ef8aad2e1c7e0

[rpc] Add incremental relay fee to getnetworkinfo fe8e8efcf9

morcos force-pushed on Jan 20, 2017

morcos commented at 12:41 pm on January 20, 2017: member

typo fixed

ryanofsky commented at 10:10 pm on January 23, 2017: member

re-utACK fe8e8efcf91fa92db68aabeb0a1709b032e60dd6

TheBlueMatt commented at 10:44 pm on January 23, 2017: member

utACK fe8e8efcf91fa92db68aabeb0a1709b032e60dd6

jtimon commented at 11:50 pm on January 23, 2017: contributor

per-commit utACK: https://github.com/bitcoin/bitcoin/pull/9589/commits/de6400de5d4b654007f21901d9555ddea0face82 https://github.com/bitcoin/bitcoin/pull/9589/commits/fe8e8efcf91fa92db68aabeb0a1709b032e60dd6 https://github.com/bitcoin/bitcoin/pull/9589/commits/6b331e6cf977caa20c83a28a8c6c2af0b6c451c5

MarcoFalke commented at 6:33 pm on January 24, 2017: member

utACK fe8e8efcf91fa92db68aabeb0a1709b032e60dd6

sdaftuar commented at 5:58 pm on January 25, 2017: member

Perhaps also update the comments in CTransaction::GetDustThreshold()? The comment there is pretty old I guess, referring to CTransaction::minRelayTxFee. Anyway some explanation of the dustRelayFee would be helpful there.

sdaftuar commented at 6:03 pm on January 25, 2017: member

This is minor, but this comment could be fixed in policyestimator_tests.cpp, line 190 (minRelayTxFee should be incrementalRelayFee):

0// evict that transaction which should set a mempool min fee of minRelayTxFee + feeV[5]

in src/wallet/rpcwallet.cpp: in 5b158707f2 outdated

2803@@ -2804,9 +2804,9 @@ UniValue bumpfee(const JSONRPCRequest& request)
2804             nNewFeeRate = CWallet::fallbackFee;
2805         }
2806 
2807-        // new fee rate must be at least old rate + minimum relay rate
2808-        if (nNewFeeRate.GetFeePerK() < nOldFeeRate.GetFeePerK() + ::minRelayTxFee.GetFeePerK()) {
2809-            nNewFeeRate = CFeeRate(nOldFeeRate.GetFeePerK() + ::minRelayTxFee.GetFeePerK());
2810+        // new fee rate must be at least old rate + minimum incremental relay rate
2811+        if (nNewFeeRate.GetFeePerK() < nOldFeeRate.GetFeePerK() + ::incrementalRelayFee.GetFeePerK()) {

sdaftuar commented at 6:28 pm on January 25, 2017:

This change (while correct) exposes a bug in the logic, where we weren’t otherwise comparing nNewFeeRate to either minTxFee or minRelayTxFee – we should probably either add a call to compare against GetRequiredFee(), or rewrite the above to just use CWallet::GetMinimumFee.

Separately – I now realize that we’re also not comparing nNewFeeRate to maxTxFee either? That seems like a separate bug in bumpfee…

Refactor GetMinimumFee to give option of providing targetFee ae9719ab87

morcos commented at 3:35 am on January 26, 2017: member

I added two new commits at the end to address the bugs @sdaftuar identified, hopefully this will help the prior review of the other commits remain useful.

I didn’t bother addressing the nits, b/c I’d like to get this merged ASAP and want to concentrate on the important parts.

MarcoFalke commented at 10:23 am on January 26, 2017: member

re-ACK 9412ca4e7baba5130baaf173b5f189c240f1408a

sdaftuar commented at 2:16 pm on January 26, 2017: member

I don’t think we should ever allow maxTxFee to be violated. Apart from the minor issue of slightly confusing semantics if maxTxFee is only enforced on totalFee and not an automatically generated one, the maxTxFee limit is the only thing preventing us from generating infinitely high (bounded only by the change output) fees if the user keeps calling bumpfee in a loop.

(Moreover, I believe a fee in excess of maxTxFee will be rejected by the mempool, and currently bumpfee does not handle this gracefully – I’ll open another PR to clean up the handling in this case.)

Use CWallet::GetMinimumFee in bumpfee

Use the wallet's fee calculation logic to properly clamp fee against minimums and maximums when calculating the fee for a bumpfee transaction.  Unless totalFee is explictly given, in which case, manually check against min, but do nothing to adjust given fee.

In all cases do a final check against maxTxFee (after adding any incremental amount).

e8021ec919

morcos force-pushed on Jan 26, 2017

morcos commented at 3:06 pm on January 26, 2017: member

OK sorry for all the churn.. Modified last commit to check against maxTxFee in all cases

Diff from 9412ca4 is here: http://pastebin.com/C1db37i3

sdaftuar commented at 4:13 pm on January 26, 2017: member

ACK e8021ec

MarcoFalke commented at 4:23 pm on January 26, 2017: member

Concept ACK on the last diff. Thanks @sdaftuar for raising the issue with maxTxFee!

morcos renamed this:
~~Fixes for minimum fees (use incrementalRelayFee for BIP 125 (RBF) replacement)~~
Fixes for bumpfee and minimum fees (use incrementalRelayFee for BIP 125 (RBF) replacement)
on Jan 26, 2017

TheBlueMatt commented at 4:36 pm on January 26, 2017: member

utACK e8021ec9193c7e8137f9716bcafd197a357a624e much nicer to use wallet’s GetMinimumFee functions, thanks.

MarcoFalke added the label Wallet on Jan 26, 2017

MarcoFalke added the label RPC/REST/ZMQ on Jan 26, 2017

btcdrak approved

btcdrak commented at 6:57 pm on January 27, 2017: contributor

utACK e8021ec

laanwj merged this on Jan 30, 2017

laanwj closed this on Jan 30, 2017

DrahtBot locked this on Sep 8, 2021

Fixes for bumpfee and minimum fees (use incrementalRelayFee for BIP 125 (RBF) replacement) #9589