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

morcos · 2017-01-19T21:22:33Z

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.

ryanofsky

utACK fa005cce2b7a8e02cedc7356d2d1933b94c26c39

ryanofsky · 2017-01-19T21:39:57Z

src/wallet/rpcwallet.cpp

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

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

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.

ryanofsky · 2017-01-19T21:48:32Z

src/wallet/rpcwallet.cpp

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.

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.

ryanofsky · 2017-01-19T21:51:54Z

src/wallet/rpcwallet.cpp

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.

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

Great, that sounds fine to me.

maflcko · 2017-01-20T00:20:34Z

utACK fa005cc, agree with the nits.

morcos · 2017-01-20T01:32:59Z

Only change is adding :: to global variables

morcos · 2017-01-20T02:21:04Z

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.

ryanofsky

utACK 72af459a53f83d507aa83c93cd3ef8aad2e1c7e0

ryanofsky · 2017-01-20T05:18:01Z

src/wallet/rpcwallet.cpp

typo getnetowrkinfo

morcos · 2017-01-20T12:41:17Z

typo fixed

ryanofsky · 2017-01-23T22:10:01Z

re-utACK fe8e8ef

TheBlueMatt · 2017-01-23T22:44:02Z

utACK fe8e8ef

jtimon · 2017-01-23T23:50:52Z

per-commit utACK: de6400d fe8e8ef 6b331e6

maflcko · 2017-01-24T18:33:08Z

utACK fe8e8ef

sdaftuar · 2017-01-25T17:58:46Z

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 · 2017-01-25T18:03:13Z

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

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

sdaftuar · 2017-01-25T18:28:00Z

src/wallet/rpcwallet.cpp

-        if (nNewFeeRate.GetFeePerK() < nOldFeeRate.GetFeePerK() + ::minRelayTxFee.GetFeePerK()) {
-            nNewFeeRate = CFeeRate(nOldFeeRate.GetFeePerK() + ::minRelayTxFee.GetFeePerK());
+        // new fee rate must be at least old rate + minimum incremental relay rate
+        if (nNewFeeRate.GetFeePerK() < nOldFeeRate.GetFeePerK() + ::incrementalRelayFee.GetFeePerK()) {


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

morcos · 2017-01-26T03:35:51Z

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.

maflcko · 2017-01-26T10:23:34Z

re-ACK 9412ca4e7baba5130baaf173b5f189c240f1408a

sdaftuar · 2017-01-26T14:16:32Z

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

morcos · 2017-01-26T15:06:18Z

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 · 2017-01-26T16:13:20Z

ACK e8021ec

maflcko · 2017-01-26T16:23:50Z

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

TheBlueMatt · 2017-01-26T16:36:04Z

utACK e8021ec much nicer to use wallet's GetMinimumFee functions, thanks.

btcdrak

utACK e8021ec

ryanofsky approved these changes Jan 19, 2017

View reviewed changes

maflcko added this to the 0.14.0 milestone Jan 20, 2017

maflcko added the TX fees and policy label Jan 20, 2017

morcos added 2 commits January 19, 2017 20:31

Use incrementalRelayFee for BIP 125 replacement

5b15870

Fix missing use of dustRelayFee

de6400d

morcos force-pushed the incrementalFee branch from fa005cc to de6400d Compare January 20, 2017 01:32

morcos changed the title ~~Use incrementalRelayFee for BIP 125 (RBF) replacement logic~~ Fixes for minimum fees (use incrementalRelayFee for BIP 125 (RBF) replacement) Jan 20, 2017

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

6b331e6

morcos force-pushed the incrementalFee branch from a144322 to 72af459 Compare January 20, 2017 02:35

ryanofsky approved these changes Jan 20, 2017

View reviewed changes

src/wallet/rpcwallet.cpp Outdated

Copy link

Contributor

ryanofsky Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo getnetowrkinfo

[rpc] Add incremental relay fee to getnetworkinfo

fe8e8ef

morcos force-pushed the incrementalFee branch from 72af459 to fe8e8ef Compare January 20, 2017 12:40

morcos mentioned this pull request Jan 23, 2017

Wallet incremental fee #9615

Merged

sdaftuar reviewed Jan 25, 2017

View reviewed changes

Refactor GetMinimumFee to give option of providing targetFee

ae9719a

morcos force-pushed the incrementalFee branch from 9412ca4 to e8021ec Compare January 26, 2017 15:05

morcos changed the title ~~Fixes for minimum fees (use incrementalRelayFee for BIP 125 (RBF) replacement)~~ Fixes for bumpfee and minimum fees (use incrementalRelayFee for BIP 125 (RBF) replacement) Jan 26, 2017

sdaftuar mentioned this pull request Jan 26, 2017

Bumpfee: bugfixes for error handling and feerate calculation #9640

Merged

maflcko added Wallet RPC/REST/ZMQ labels Jan 26, 2017

btcdrak approved these changes Jan 27, 2017

View reviewed changes

laanwj merged commit e8021ec into bitcoin:master Jan 30, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

morcos commented Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

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!

maflcko commented Jan 20, 2017

Uh oh!

morcos commented Jan 20, 2017

Uh oh!

morcos commented Jan 20, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jan 20, 2017

Uh oh!

ryanofsky commented Jan 23, 2017

Uh oh!

TheBlueMatt commented Jan 23, 2017

Uh oh!

jtimon commented Jan 23, 2017

Uh oh!

maflcko commented Jan 24, 2017 via email

Uh oh!

sdaftuar commented Jan 25, 2017

Uh oh!

sdaftuar commented Jan 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jan 26, 2017

Uh oh!

maflcko commented Jan 26, 2017

Uh oh!

sdaftuar commented Jan 26, 2017

Uh oh!

morcos commented Jan 26, 2017

Uh oh!

sdaftuar commented Jan 26, 2017

Uh oh!

maflcko commented Jan 26, 2017

Uh oh!

TheBlueMatt commented Jan 26, 2017

Uh oh!

btcdrak left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

morcos commented Jan 19, 2017 •

edited

Loading