Skip to content
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.

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

[qt] Use maxTxFee instead of 10000000 #6951

Merged
merged 1 commit into from Nov 18, 2015

Conversation

@MarcoFalke
Copy link
Member

MarcoFalke commented Nov 5, 2015

@morcos @laanwj @cozz bitcoin-qt should not ignore the param and use DEFAULT_TRANSACTION_MAXFEE = 0.1 * COIN all the time.

This code behaves exactly the same as the code which got rejected in #6887. (C.f. in-source comment for explanation)

@laanwj laanwj added GUI Wallet labels Nov 9, 2015
@laanwj
Copy link
Member

laanwj commented Nov 9, 2015

utACK

@fanquake
Copy link
Member

fanquake commented Nov 11, 2015

utACK

@jonasschnelli
Copy link
Member

jonasschnelli commented Nov 16, 2015

Slightly NACK.
CreateTransaction() at L277 (https://github.com/bitcoin/bitcoin/blob/master/src/qt/walletmodel.cpp#L277) will already reduce the fee to maxTxFee (through GetMinimumFee(nBytes, nTxConfirmTarget, mempool)).
Check: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L2144

IMO it's impossible that the changed if triggers (if (nFeeRequired > maxTxFee))).

I think we should remove the if.

The only concern I see – is – if a user sets an absurde fee over the QT send coins UI, lets assume 10.0 BTC per kb (which will be accepted in the input filed), the fee will automatically – without warning or informing – reduced to maxTxFee. A warning or information would be appropriate in this case.

@MarcoFalke
Copy link
Member Author

MarcoFalke commented Nov 16, 2015

I think we should remove the if.

@laanwj did not like that.

@jonasschnelli
Copy link
Member

jonasschnelli commented Nov 16, 2015

After reading the ifs comment, I see the purpose of this check. It's a "belt-and-suspenders check".
So, it shouldn't hurt and might protect from insane fees if we once change the wallet layer.

Changed my mind:
Tested ACK.

@jonasschnelli jonasschnelli merged commit 513686d into bitcoin:master Nov 18, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jonasschnelli added a commit that referenced this pull request Nov 18, 2015
513686d [qt] Use maxTxFee instead of 10000000 (MarcoFalke)
@MarcoFalke MarcoFalke deleted the MarcoFalke:MarcoFalke-2015-qtMaxFee branch Nov 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.