AcceptToMemoryPool: Use -maxtxfee when rejecting high fee txs #6723

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:MarcoFalke-2015-mempoolHighFeeFix changing 1 files +4 −3

MarcoFalke commented at 4:34 PM on September 25, 2015: member

It is unreasonable to ignore -maxtxfee when rejecting high fee transactions. This PR removes the hard coded limit and uses -maxtxfee instead.

EDIT: This fixes #6725 by coupling maxtxfee from the wallet with the mempool's REJECT_HIGHFEE.
AcceptToMemoryPool: Use -maxtxfee when rejecting high fee txs b824e63c13
in src/main.cpp:None in b824e63c13
```
  32 | @@ -33,6 +33,7 @@
  33 |  #include "utilmoneystr.h"
  34 |  #include "utilstrencodings.h"
  35 |  #include "validationinterface.h"
  36 | +#include "wallet/wallet.h"
```
MarcoFalke commented at 4:42 PM on September 25, 2015:

Including wallet.h here makes the smallest diff but I am not sure if this complies with bitcoin coding guidelines? Also travis fails because I did not yet consider no-wallet builds.
jonasschnelli commented at 4:53 PM on September 25, 2015: contributor

Seems to be NACK territory.

Coupling the wallets -maxtxfee to mempool seems as a step in the wrong direction. -maxtxfee is a protection of the wallet part. If we are unhappy with minRelayTxFee.GetFee(nSize) * 10000 as max mempool fee, we should think about adding another -arg or searching after a better non-magic-number solution.
MarcoFalke commented at 5:00 PM on September 25, 2015: member

Yes, maybe you are right. I created the PR to point out a bug in the wallet:

Locally created transaction (Qt, sendtoaddress, rawtransaction, ...) are usually added to the mempool. The problem appears when the mempool uses different rules than Qt to determine if a transaction is "valid": A "valid" transaction created (by e.g. Qt) may be rejected AFTER being added to the wallet. This leads to the double-spend-warning and conflicted-tx icon in Qt. Imo, this is not only misleading but a wallet bug which should be fixed. This feature was introduced in #2949.
sipa commented at 5:01 PM on September 25, 2015: member

Agree with Jonas.
jonasschnelli commented at 5:05 PM on September 25, 2015: contributor

@MarcoFalke: if this is the cases (didn't check myself), than the wallet should reject a transaction that cannot be added to the local mempool. In future, a mempool could be optional for the (QT)wallet, ... maybe the wallet should check if the transaction was broadcasted over the network (by checking if some invs of some nodes come in). If no invs of the just sent tx are coming in (after reasonable timespan), warn the user, allow to increase fees, etc.
gmaxwell commented at 5:37 PM on September 25, 2015: contributor

I think the whole maximum behavior is not worth having if it introduces another economically varying parameter that must be maintained.

I've tested rawtx and it correctly does not add the transaction to the wallet.

I don't think this behavior really should be applied to non-raw-tx as there is no mechanism to override and it should have better ways to avoid crazy fees. This really only exists as a last ditch protection to catch the crazy cases we've observed where users have lost tens or hundreds of BTC to fees because they (or software they're running) misunderstood and misused the raw transaction interface.
MarcoFalke commented at 6:48 PM on September 25, 2015: member
@jonasschnelli didn't check myself

Steps to reproduce on 48efbdb:
```
-> settxfee 0.04
<- 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)
```
@gmaxwell I've tested rawtx and it correctly does not add the transaction to the wallet.

Sorry, my mistake. (Only transactions where the wallet calculates the fee are affected)

You can "work around" this bug by setting -minrelaytxfee higher:
```
[command-line arg -minrelaytxfee=0.00002000]

-> settxfee 0.04
<- true


-> sendtoaddress mwEHWzBhaPYiErWWhmWW7QCjGoKNPaQQax 8
<- f2b6c51afbf09ee05422a19916410c038198b3c4f9921b6c6262c98b69e9817b
```
However, "normal" transaction will then falsely include a higher fee, even though minrelaytxfee should not be coupled with transaction creation. (At least not without educating the non-code reading user):
```
[still command-line arg -minrelaytxfee=0.00002000]

-> settxfee 0
<- true


-> sendtoaddress mwEHWzBhaPYiErWWhmWW7QCjGoKNPaQQax 8
<- 6269040f060061858ff9e11bbce9bed2ded6cc299f296b8898ecc30e77a7f1bf
```
Where 6296040... pays a fee of .00002 BTC per kB (=minrelaytxfee)

@gmaxwell I don't think this behavior really should be applied to non-raw-tx

Sounds reasonable and should be easy to code. I will have a look...
sipa commented at 6:59 PM on September 25, 2015: member

An alternative (not for this PR) would be to have a way of calling AcceptToMemoryPool in a dummy mode, where everything is done except actually adding to the mempool. Such a function could optionally return a data structure with information about the transaction's effects that can only be computed with the UTXO set present, such as fees, age of inputs, ...

That could move the high fee detection logic to RPC, rather than being part of the network relay code...
MarcoFalke commented at 5:45 PM on September 26, 2015: member

Closing per "I don't think this behavior really should be applied to non-raw-tx".
MarcoFalke closed this on Sep 26, 2015
MarcoFalke deleted the branch on Sep 26, 2015
MarcoFalke commented at 12:35 AM on March 14, 2016: member

This was merged as #7084 instead.
MarcoFalke locked this on Sep 8, 2021

Contributors

Linked (view graph)

#6725 Mempool should not reject wallet created transactions with sufficient fee