Skip to content

mempool: Replace maxFeeRate of 10000*minRelayTxFee with maxTxFee #7084

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
laanwj merged 2 commits into from
Feb 2, 2016
Merged

mempool: Replace maxFeeRate of 10000*minRelayTxFee with maxTxFee #7084

laanwj merged 2 commits into from
Feb 2, 2016

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Nov 23, 2015

maxTxFee is the absolute upper bound on fees of transactions created by the wallet.

  • 10000*minRelayTxFee_default = 0.1 BTC/kB
  • maxTxFee_default = 0.1 BTC

Further info:
This check was initially only added to the mempool to reject high fee raw transactions by default (sendrawtransaction) but was later extended to also check wallet transactions. minRelayTxFee may not be the best config param to adjust fee behavior so switching to the already used maxTxFee could make sense.

This fixes the issue:

-> settxfee 0.1
<- true

-> sendtoaddress mwEHWzBhaPYiErWWhmWW7QCjGoKNPaQQax 16
<- Error: The transaction was rejected! This might happen if some of the coins in your wallet were already spent, such as if you used a copy of wallet.dat and coins were spent in the copy but not marked as spent here. (code -4)

@maflcko maflcko force-pushed the MarcoFalke-2015-mempoolMaxTxFee branch 2 times, most recently from fa4ee78 to faeb997 Compare November 23, 2015 21:50
@dcousens
Copy link
Contributor

dcousens commented Nov 24, 2015

utACK

@jtimon
Copy link
Contributor

jtimon commented Nov 27, 2015

Maybe you prefer to pass it to AcceptToMemoryPool explicitly directly. That would be a little bit more disruptive though.

Anyway, better is better, utACK.

Bike-shedding: Can the global be named globalMaxTxFee ? You could also add the following to the beginning of AcceptToMemoryPool() ``` const CAmount& maxTxFee = globalMaxTxFee; ``` as a preparation for making it a explicit parameter to AcceptToMemoryPool.

EDIT: never mind the bike-shedding nit, the name maxTxFee has to be preserved because it's currently used like that in the wallet.

@jtimon jtimon mentioned this pull request Nov 27, 2015
Merged
@maflcko maflcko closed this Dec 1, 2015
@maflcko maflcko deleted the MarcoFalke-2015-mempoolMaxTxFee branch December 1, 2015 19:30
@maflcko maflcko restored the MarcoFalke-2015-mempoolMaxTxFee branch December 1, 2015 19:30
@maflcko maflcko reopened this Dec 1, 2015
@maflcko maflcko force-pushed the MarcoFalke-2015-mempoolMaxTxFee branch from faeb997 to ffff08a Compare December 4, 2015 16:18
@maflcko
Copy link
Member Author

maflcko commented Dec 10, 2015

Rebased (trivial)

morcos
morcos reviewed Dec 10, 2015
View reviewed changes
src/wallet/wallet.cpp Outdated
Copy link
Contributor

@morcos morcos Dec 10, 2015

Choose a reason for hiding this comment

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

nit: this comment didn't really help me. maybe // Wallet uses same maxTxFee as ATMP so tx should not be rejected for fee but maybe its just unnecessary.

Copy link
Contributor

@jtimon jtimon Dec 10, 2015

Choose a reason for hiding this comment

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

But, please, don't use acronyms for the name functions in the comments: the full name of the function is something that can be seen in grep.

Copy link
Member Author

@maflcko maflcko Dec 10, 2015

Choose a reason for hiding this comment

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

Guess I will drop this because it is already mentioned in wallet.h: https://github.com/bitcoin/bitcoin/pull/7084/files#diff-12635a58447c65585f51d32b7e04075bR205

@morcos
Copy link
Contributor

morcos commented Dec 10, 2015

utACK

@maflcko maflcko force-pushed the MarcoFalke-2015-mempoolMaxTxFee branch from ffff08a to faa1b1f Compare December 11, 2015 07:59
@maflcko
Copy link
Member Author

maflcko commented Jan 7, 2016

@laanwj Anything holding this back?

@maflcko maflcko mentioned this pull request Jan 7, 2016
Merged
@laanwj laanwj added the Mempool label Jan 29, 2016
@laanwj
Copy link
Member

laanwj commented Jan 29, 2016

You should also move -maxtxfee in init; it's still under the wallet options, whereas it now applies to transactions submitted over RPC too.

@dcousens
Copy link
Contributor

dcousens commented Jan 30, 2016

utACK faa1b1f, but waiting on fix for init option as specified by @laanwj above.

@maflcko maflcko force-pushed the MarcoFalke-2015-mempoolMaxTxFee branch from faa1b1f to fa331db Compare January 30, 2016 10:29
@maflcko
Copy link
Member Author

maflcko commented Jan 30, 2016

I have also fixed the doxygen comments in main.h. Let me know if this is not wanted.

laanwj
laanwj reviewed Feb 2, 2016
View reviewed changes
src/main.h Outdated
Copy link
Member

@laanwj laanwj Feb 2, 2016

Choose a reason for hiding this comment

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

Nit: /**, otherwise it won't show up in doxygen at all

Copy link
Member Author

@maflcko maflcko Feb 2, 2016

Choose a reason for hiding this comment

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

Good catch! Need to bookmark https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#doxygen-comments I guess.

@laanwj
Copy link
Member

laanwj commented Feb 2, 2016

I have also fixed the doxygen comments in main.h. Let me know if this is not wanted.

I'm ok with that, but let's limit the amount of new changes from here on, as they keep people busy with reviewing and commenting :) I'm trying to get this ready for merge.

@jtimon
Copy link
Contributor

jtimon commented Feb 2, 2016

re-utACK fa1193e

@laanwj laanwj merged commit fa1193e into bitcoin:master Feb 2, 2016
laanwj added a commit that referenced this pull request Feb 2, 2016
@laanwj
Merge #7084: mempool: Replace maxFeeRate of 10000*minRelayTxFee with …
11d74f6 
…maxTxFee

fa1193e [doxygen] Actually display comment (MarcoFalke)
fa331db mempool: Replace maxFeeRate of 10000*minRelayTxFee with maxTxFee (MarcoFalke)
@maflcko maflcko deleted the MarcoFalke-2015-mempoolMaxTxFee branch February 2, 2016 18:27
@maflcko maflcko mentioned this pull request Feb 2, 2016
Closed
This was referenced Mar 14, 2016
Closed
Closed
codablock referenced this pull request in codablock/dash Dec 11, 2017
@laanwj @codablock
Merge dashpay#7084: mempool: Replace maxFeeRate of 10000*minRelayTxFe…
0afbd1c 
…e with maxTxFee

fa1193e [doxygen] Actually display comment (MarcoFalke)
fa331db mempool: Replace maxFeeRate of 10000*minRelayTxFee with maxTxFee (MarcoFalke)
@str4d str4d mentioned this pull request Dec 17, 2019
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Dec 18, 2019
@zkbot
Auto merge of #4268 - str4d:2074-cleanup-2, r=str4d
577f7ef 
Bitcoin 0.12 cleanup PRs 2

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6631
- bitcoin/bitcoin#6664
  - Only the first commit (we already had the second through bitcoin/bitcoin#6825).
- bitcoin/bitcoin#6669
- bitcoin/bitcoin#6887
  - Only the non-QT parts.
- bitcoin/bitcoin#6962
- bitcoin/bitcoin#6822
  - Only first and third commits (we already had the second through an earlier PR).
- bitcoin/bitcoin#7136
  - Excludes Travis CI changes, and fixes to documents we don't have anymore.
- bitcoin/bitcoin#7084
- bitcoin/bitcoin#7509
- bitcoin/bitcoin#7617
- bitcoin/bitcoin#7726

Part of #2074.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Mempool

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maflcko @dcousens @jtimon @morcos @laanwj