[rpc] [tests] mempoolminfee should not drop below minRelayTxFee #11410

#6941

I tested this in wallet.py. Please advise if you think there is a better place where to test this (for example a more comprehensive new test which checks getmempoolinfo)

utACK d4e253e.

I think we should just replace mempoolminfee with max(mempoolminfee, minrelayfee) as we no longer support priority/free-relay txn that will likely not be mined (so the original arguments for separating them no longer hold).

Agree with @TheBlueMatt.

Thanks for the feedback.

We already do max between rollingMinimumFeeRate and incrementalRelayFee in https://github.com/bitcoin/bitcoin/blob/master/src/txmempool.cpp#L1002, but there are cases when we return 0:

• https://github.com/bitcoin/bitcoin/blob/master/src/txmempool.cpp#L999
• https://github.com/bitcoin/bitcoin/blob/master/src/txmempool.cpp#L984

To solve this, (imo) we have a few options:

1. Do max between the mempool.GetMinFee(maxmempool) and ::minRelayTxFee in https://github.com/bitcoin/bitcoin/blob/master/src/rpc/blockchain.cpp#L1328
2. Do the max in https://github.com/bitcoin/bitcoin/blob/master/src/txmempool.cpp#L981
   • for this I would suggest to create a new method in https://github.com/bitcoin/bitcoin/blob/master/src/txmempool.cpp which returns the max between GetMinFee and ::minRelayTxFee so we don't do 3 max statements (one for each return)
   • or do max in the 3 returns of the method

Option 1 seems like the least behavior changing because it is there for the RPC interface. I am not sure weather option 2 is desired because it might alter some behavior, but it does seem like the correct place where to put it.

At the moment, I am leaning towards option 1. I want to ask for some advice, which option do you think is better 1 or 2?

If you create the new method (option 2), you should also use it in the other places where GetMinFee is called and remove the (now obsolete) std::max from them. If you only plan to use it in getmempoolinfo, you could as well just do the std::max there (option 1).

I think I agree with @sipa and @TheBlueMatt. There are still a couple of differences between minRelayTxFee and mempoolMinFee. One I can thing of now is the wallet will allow you to create a transaction below mempoolMinFee but not below minRelayTxFee. But I think if we carefully went through the code to see where either of them were used we might find that there is no point in ever returning a mempoolMinFee below the minRelayTxFee. In which case we could do as suggested.

• Line 984 is a bug as this should still have been maxed with minIncrementalRelayFee, so the test before that could just be reversed and used to guard executing the bulk of the function so there is no early return.
• Line 999 could just go away and not early return
• Line 1002 could be changed to max with minRelayTxFee (but also max with minIncrementalRelayFee).

Then we may be able to simplify other places in the code that check both like AcceptToMemoryPool.

Rebased

utACK 52711b9aa8c35b6f9d275fde7206bdb2276ce201

Catching up here, but I guess the original version of this PR added minrelaytxfee as a new field returned by getmempoolinfo, while the new version instead incorporates it into the existing mempoolminfee field as suggested by Matt.

This is implemented by changing CTxMemPool::GetMinFee to now return the max of the rolling minimum with minrelaytxfee. There are also two bugfixes to CTxMemPool::GetMinFee to make it consistently also max with incrementalRelayFee instead of skipping this when the rolling minimum is 0.

A side effect of changing CTxMemPool::GetMinFee is that validation code now returns a "mempool min fee not met" error instead of "min relay fee not met" when fee is below minrelaytxfee, so there is also a test update and simplification to the validation code in the PR.

You can remove GetRequiredFee in src/wallet/fees.cpp now.

utACK 9423369fabb9ad13769a594f8b7b2ed5b655170c

I don't think this directly relates to GetRequiredFee. There are still a number of calls to it, and it seems worth keeping.

@ryanofsky fixed, thx @MarcoFalke prefer to do that in a different PR. Is that fine? Will create a different issue if that is ok.

To elaborate a bit: Previously, when calling GetMinimumFee and the resulting fee was smaller than the minRelayTxFee, the reason for increasing the fee would always be FeeReason::REQUIRED. Now, the reason for increasing the fee would depend on whether paytxfee (would result in FeeReason::REQUIRED) was used or smart fee estimation (would result in FeeReason::MEMPOOL_MIN). With your changes, I'd prefer to set FeeReason::MEMPOOL_MIN in all cases where the mempool min fee is not met (including minRelayTxFee). Thus, removing the call to GetRequiredFee in GetMinimumFee makes sense to me.

I know that those are just internal debugging details, so I am fine if this is done in a follow up pull.

Removing the call to GetRequiredFee in GetMinimumFee does seem like the correct thing to do here (and the change would only be harder to understand in another context). I just didn't think GetRequiredFee should be removed entirely.

Oh, ok, now I understand. I removed the GetRequiredFee code from GetMinimumFee

After repeated runs of ./test/functional/test_runner.py fundrawtransaction, I have my doubts that the code in https://github.com/bitcoin/bitcoin/pull/11410/files#diff-ca74c4b28865382863b8fe7633a85cd6 is correct.

Sometimes, I get this error:

..........................................................................................................................................................................
fundrawtransaction.py failed, Duration: 85 s

stdout:
2017-10-03 14:51:58.853000 TestFramework (INFO): Initializing test directory /tmp/bitcoin_test_runner_20171003_175158/fundrawtransaction_49
{'936dc7485b7e275eed5d7a4fa4f2f5b26b7ffb180a05fc7d28a395ba07b006b8'}
{'936dc7485b7e275eed5d7a4fa4f2f5b26b7ffb180a05fc7d28a395ba07b006b8'}
{'936dc7485b7e275eed5d7a4fa4f2f5b26b7ffb180a05fc7d28a395ba07b006b8'}
set()
2017-10-03 14:53:21.925000 TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/whoami/pr0n/bitcoin/test/functional/test_framework/test_framework.py", line 117, in main
    self.run_test()
  File "/home/whoami/pr0n/bitcoin/test/functional/fundrawtransaction.py", line 505, in run_test
    self.sync_all()
  File "/home/whoami/pr0n/bitcoin/test/functional/test_framework/test_framework.py", line 326, in sync_all
    sync_mempools(group)
  File "/home/whoami/pr0n/bitcoin/test/functional/test_framework/util.py", line 401, in sync_mempools
    raise AssertionError("Mempool sync failed")
AssertionError: Mempool sync failed
2017-10-03 14:53:21.926000 TestFramework (INFO): Stopping nodes
2017-10-03 14:53:24.184000 TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_test_runner_20171003_175158/fundrawtransaction_49
2017-10-03 14:53:24.184000 TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_test_runner_20171003_175158/fundrawtransaction_49/test_framework.log

NOTE: on master, this doesn't happen. Need to investigate