Move maxTxFee out of mempool #7070

MarcoFalke commented at 3:58 PM on November 20, 2015: member

The mempool is not the right place for this single hard coded value. maxFeeRate should move out of the mempool.

Future PRs may use this and remove the misleading on-off-only logic from the rpc interface or set a different value for the wallet, etc.

This should be an uncontroversial refactor only commit not holding back a more sophisticated solution (like a dry-run AcceptToMemoryPool()).

in src/rpcrawtransaction.cpp:None in 73d275fa6c outdated

 807 | @@ -808,9 +808,9 @@ UniValue sendrawtransaction(const UniValue& params, bool fHelp)
 808 |          throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
 809 |      uint256 hashTx = tx.GetHash();
 810 |  
 811 | -    bool fOverrideFees = false;
 812 | -    if (params.size() > 1)
 813 | -        fOverrideFees = params[1].get_bool();
 814 | +    CFeeRate rawMaxFeeRate = CFeeRate(::minRelayTxFee.GetFeePerK() * 10000);

dcousens commented at 4:45 PM on November 20, 2015:

What does the 10000 represent here OOI?

petertodd commented at 9:35 PM on November 20, 2015:

This should be a separate constant I think - the min relay tx fee changing doesn't necessarily change what constitutes an "absurd" fee. For instance, when we recently upped the min relay fee it was for reasons that shouldn't have impacted the upper bound.

Better would be for this to be a config file setting, shared across the RPC interface and wallet, with some sane default value that gets adjusted as needed.

MarcoFalke commented at 11:50 PM on November 21, 2015:

Something like -maxtxfeerate ?

petertodd commented at 9:21 PM on November 22, 2015:

Exactly! Or probably actually a -maxtxfee rather than feerate - if for some reason you need a very large transaction to send some funds, that's equally likely to constitute a bug that should be investigated manually by a human and fixed.

jtimon commented at 4:39 PM on November 27, 2015:

What about DEFAULT_TRANSACTION_MAXFEE from #7084 ? In fact, why not unify both PRs and make both things at once?

petertodd commented at 9:45 PM on November 20, 2015: contributor

As this is a safety thing, I'm weakly inclined to NACK this based on the likelyhood that AcceptToMempool() will be called with an inappropriate max fee. Basically, you're taking the code that does that and splitting it in half, with one half duplicated.

A better design might be to make a single function in the wallet that does the task of accepting a transaction: verifying sanity checks, then sending it via AcceptToMempool()

jonasschnelli added the label Refactoring on Nov 21, 2015

in src/wallet/wallet.h:None in 73d275fa6c outdated

 198 | @@ -198,7 +199,7 @@ class CMerkleTx : public CTransaction
 199 |      int GetDepthInMainChain() const { const CBlockIndex *pindexRet; return GetDepthInMainChain(pindexRet); }
 200 |      bool IsInMainChain() const { const CBlockIndex *pindexRet; return GetDepthInMainChainINTERNAL(pindexRet) > 0; }
 201 |      int GetBlocksToMaturity() const;
 202 | -    bool AcceptToMemoryPool(bool fLimitFree=true, bool fRejectAbsurdFee=true);
 203 | +    bool AcceptToMemoryPool(bool fLimitFree=true, CFeeRate maxFeeRate=CFeeRate(::minRelayTxFee.GetFeePerK() * 10000));

jtimon commented at 4:37 PM on November 27, 2015:

Can't we just get rid of the default value and require the caller to always pass it?

jtimon commented at 4:47 PM on November 27, 2015: contributor

utACK modulo nits and unifying with #7084.

My preferred solution would be to take the absurd fees check to a new function that gets called just before AcceptToMemoryPool (unless they used to call with fRejectAbsurdFee=false). But that duplicates the calculation of the fees and hurts performance, so maybe not everybody likes it.

in src/main.h:None in 73d275fa6c outdated

 236 | @@ -237,7 +237,7 @@ void PruneAndFlush();
 237 |  
 238 |  /** (try to) add transaction to memory pool **/
 239 |  bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransaction &tx, bool fLimitFree,
 240 | -                        bool* pfMissingInputs, bool fOverrideMempoolLimit=false, bool fRejectAbsurdFee=false);
 241 | +                        bool* pfMissingInputs, bool fOverrideMempoolLimit=false, CFeeRate maxFeeRate=CFeeRate(0));

jtimon commented at 4:50 PM on November 27, 2015:

Actually, this is where I dislike the default value the most, I got confused with CWallet's method declaration. In that method I don't even care if fRejectAbsurdFee=true is preserved.

sipa commented at 4:58 PM on November 27, 2015: member

@jtimon The reason it's done inside AcceptToMemoryPool in the first place is because elsewhere we don't have access to the input amounts (which are necessary to compute the fee), at least not without significantly duplicating logic.

sipa commented at 4:59 PM on November 27, 2015: member

Hmm.... what about making ATMP just return the fee, so the caller can check if it's insane?

jtimon commented at 5:22 PM on November 27, 2015: contributor

Yeah, well duplicating logic is not strictly necessary, but I pointed out the performance hit (from duplicated calculations). I like the solution of passing a ref CAmount& nTxFee to AcceptToMemoryPool like in #6445. By the way, that PR is closed because "was going to interfere with mempool work" as well...

MarcoFalke commented at 9:41 PM on November 29, 2015: member

I can't see how returning the fee helps here. When ATMP returns the fee it's already too late to abort ATMP adding the tx.

MarcoFalke commented at 9:41 PM on November 29, 2015: member

Closing for now...

MarcoFalke closed this on Nov 29, 2015

jtimon commented at 10:10 PM on November 29, 2015: contributor

@MarcoFalke You are right, returning the fee doesn't help at this point because AcceptToMemoryPool() does more than just checking. For what is worth, I like this approach to at least remove one more use of the global ::minRelayTxFee (modulo nits, specially unifying with the other PR).

MarcoFalke commented at 9:56 AM on November 30, 2015: member

@jtimon rebased on #7084.

MarcoFalke reopened this on Nov 30, 2015

MarcoFalke force-pushed on Nov 30, 2015

in src/wallet/wallet.h:None in fa0215e73a outdated

 195 | @@ -198,7 +196,8 @@ class CMerkleTx : public CTransaction
 196 |      int GetDepthInMainChain() const { const CBlockIndex *pindexRet; return GetDepthInMainChain(pindexRet); }
 197 |      bool IsInMainChain() const { const CBlockIndex *pindexRet; return GetDepthInMainChainINTERNAL(pindexRet) > 0; }
 198 |      int GetBlocksToMaturity() const;
 199 | -    bool AcceptToMemoryPool(bool fLimitFree=true, bool fRejectAbsurdFee=true);
 200 | +    /** Pass this transaction to the mempool. Fails if absolute fee exceeds maxTxFee. */
 201 | +    bool AcceptToMemoryPool(bool fLimitFree=true, CAmount nAbsurdFee=maxTxFee);

jtimon commented at 11:52 AM on November 30, 2015:

Why not default to the constant DEFAULT_TRANSACTION_MAXFEE instead of the global maxTxFee ?

MarcoFalke commented at 12:02 PM on November 30, 2015:

This would mitigate the effort of #7084 trying to fix the inconsistencies between wallet-high-fee and mempool-absurd-fee.

jtimon commented at 9:21 PM on November 30, 2015:

I would prefer not to default to the value of a global, either the constant as default or just making the parameter mandatory do it. Anyway, it's just a nit.

in src/rpcrawtransaction.cpp:None in fa0215e73a outdated

 807 | @@ -808,9 +808,9 @@ UniValue sendrawtransaction(const UniValue& params, bool fHelp)
 808 |          throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
 809 |      uint256 hashTx = tx.GetHash();
 810 |  
 811 | -    bool fOverrideFees = false;
 812 | -    if (params.size() > 1)
 813 | -        fOverrideFees = params[1].get_bool();
 814 | +    CAmount nMaxRawTxFee = maxTxFee;

jtimon commented at 11:52 AM on November 30, 2015:

Why not default to the constant DEFAULT_TRANSACTION_MAXFEE instead of the global maxTxFee ?

MarcoFalke commented at 12:00 PM on November 30, 2015:

A user could set it through -maxtxfee, which is preferred?

jtimon commented at 12:03 PM on November 30, 2015:

Well that's something related to the wallet, which this RPC used to be independent from. That's my reasoning, but let's see what others think...

MarcoFalke commented at 12:06 PM on November 30, 2015:

Maybe we could get a third arg to the rpc and make nMaxRawTxFee = params[2] and default to DEFAULT_TRANSACTION_MAXFEE?

jtimon commented at 12:12 PM on November 30, 2015:

Yes, maybe even replacing the boolean (although it breaks the old API). instead of override=true, people could use maxfee=0.

in src/main.h:None in fa0215e73a outdated

 240 | @@ -237,7 +241,7 @@ void PruneAndFlush();
 241 |  
 242 |  /** (try to) add transaction to memory pool **/
 243 |  bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransaction &tx, bool fLimitFree,
 244 | -                        bool* pfMissingInputs, bool fOverrideMempoolLimit=false, bool fRejectAbsurdFee=false);
 245 | +                        bool* pfMissingInputs, bool fOverrideMempoolLimit=false, CAmount nAbsurdFee=0);

jtimon commented at 11:56 AM on November 30, 2015:

I wouldn't oppose to just make nAbsurdFee mandatory (instead of defaulting to zero). In any case, we can const CAmount nAbsurdFee or const CAmount& nAbsurdFee.

MarcoFalke commented at 7:14 PM on February 2, 2016:

@jtimon made it const (pass by reference does not make sense for ints).

Not sure about making it mandatory.

jtimon commented at 11:56 AM on November 30, 2015: contributor

utACK (modulo nits).

MarcoFalke force-pushed on Dec 1, 2015

MarcoFalke force-pushed on Feb 2, 2016

MarcoFalke renamed this:
~~Move hardcoded maxFeeRate out of mempool~~
Move maxTxFee out of mempool
on Feb 2, 2016

Move maxTxFee out of mempool

Also, remove default values in CMerkleTx::AcceptToMemoryPool()

fa79db2641

MarcoFalke force-pushed on Feb 2, 2016

dcousens commented at 9:29 PM on February 2, 2016: contributor

utACK fa79db2

[wallet.h] Remove main.h include fa762d0f00

in src/main.cpp:None in fa79db2641 outdated

1219 | @@ -1220,10 +1220,10 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C
1220 |  }
1221 |  
1222 |  bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransaction &tx, bool fLimitFree,
1223 | -                        bool* pfMissingInputs, bool fOverrideMempoolLimit, bool fRejectAbsurdFee)
1224 | +                        bool* pfMissingInputs, bool fOverrideMempoolLimit, const CAmount nAbsurdFee)

laanwj commented at 11:17 AM on February 3, 2016:

Any specific reason for making this pass-by-value argument const? (also below in AcceptToMemoryPool) or did you mean const CAmount& nAbsurdFee?

MarcoFalke commented at 12:05 PM on February 3, 2016:

I don't have background in cpp but isn't this considered best practice? (Fiddling with nAbsurdFee here would give error: assignment of read-only parameter ‘nAbsurdFee’).

Pass by reference is not mentioned in /doc/developer-notes.md and I don't like doing this for basic types but I am happy to change if that's something we do.

laanwj commented at 12:22 PM on February 3, 2016:

Having pass-by-value args const is something we don't do anywhere, whereas there are many places where CAmount is passed by reference (as a git grep shows me). The idea is that CAmount is not a basic type in some derived code.

dcousens commented at 10:32 PM on February 3, 2016:

IMHO extra const is never a bad thing. In this case, if CAmount is not considered a basic enough type, then sure, const CAmount&.

jtimon commented at 12:44 AM on February 4, 2016:

FWIW, I didn't know that const-by-value could be ever considered worse than regular by-value either. @laanwj can you extend on this or were you just pointing at the fact that you can always turn a const-by-value into a const-by-ref unless you want to avoid the warning you'll get with basic types?

laanwj commented at 8:16 AM on February 4, 2016:

@jtimon I never said it was worse, it is just a convention we use nowhere else, whereas const CAmount& someAmount is a common convention (according to git grep).

There's no deep reason not to make pass-by-value arguments to a function const. On the other hand, it is only a programmer convention for marking a value as "Do not change this!". The compiler will still happily push a copy on the stack.

So usually that means someone made a typo. If this is intentional I'm fine with it, I didn't intend for this to become a whole argument.

in src/wallet/wallet.h:None in fa79db2641 outdated

   6 | @@ -7,6 +7,7 @@
   7 |  #define BITCOIN_WALLET_WALLET_H
   8 |  
   9 |  #include "amount.h"
  10 | +#include "main.h"

laanwj commented at 11:18 AM on February 3, 2016:

Does this header now need the header main? (I don't see why, from the changes)

MarcoFalke commented at 11:35 AM on February 3, 2016:

I don't think so. wallet.cpp already has it. (I think I added it when AcceptToMemoryPool() used maxTxFee as a default value)

jtimon commented at 11:39 AM on February 3, 2016:

Great! Could you try to remove it then? ~~I rmember not being able to do it in the past due to the many functions/methods defined in the .h instead of the .cpp but hopefully that has already changed.~~ I remember removing it and if it's in the .h other files depending on wallet will start to forget about depending on main if they do.

MarcoFalke force-pushed on Feb 3, 2016

ATMP: make nAbsurdFee const fad6244879

MarcoFalke force-pushed on Feb 3, 2016

laanwj merged this on Feb 4, 2016

laanwj closed this on Feb 4, 2016

laanwj referenced this in commit 4f4dc5ef72 on Feb 4, 2016

pedrobranco referenced this in commit 35f35d77c1 on Feb 4, 2016

pedrobranco referenced this in commit fdd47717d8 on Feb 4, 2016

MarcoFalke deleted the branch on Feb 4, 2016

codablock referenced this in commit 1fd57fde91 on Sep 16, 2017

codablock referenced this in commit 269d5c5905 on Sep 19, 2017

codablock referenced this in commit ccca5c9ea8 on Dec 9, 2017

codablock referenced this in commit d09be5ac12 on Dec 9, 2017

codablock referenced this in commit a3d1e5eda9 on Dec 11, 2017

DrahtBot locked this on Sep 8, 2021