ensure RPC calls AcceptToMemoryPool using same fee policy as net
AbandonedPublic
Actions

Authored by markblundeberg on Dec 15 2019, 08:16.

Details

Reviewers

jasonbcox
deadalnix

Group Reviewers

Restricted Project

Summary

This ensures that AcceptToMemoryPool initiated from RPC does not deviate
from the network fee policy (e.g. net_processing). Note RPC still has the
nAbsurdFee thing, but that is purely restrictive and optional so it's OK.

Since the default is -limitfreerelay=0, the discrepancy meant that any low
fee, high priority transaction broadcasted from sendrawtransaction would
end up stuck in the mempool of that node, and no peers will accept it.
This has been a frustrating issue for various light wallet users where
their portal ends up running through sendrawtransaction and they end
up having low fee transactions get apparently "successfully broadcast"
yet stuck forever since they don't get accepted upon relay. The issue has
been seen in two popular wallets: Electron Cash, bitcoin.com wallet.

There remains one callsite with fLimitFree = false, namely
updateMempoolForReorg. I suppose this is better than having =true, and it's
not really something that matters much for everyday usage so let's not
deal with that here.

A few functional tests had to be fixed as they relied on this quirk of
RPC to accept lowfee transactions (they would have failed if they used
submission via net). A bug was also exposed in the fee calculator (caused
dbcrash to fail). The 'high priority transactions' test has also been
updated to check default behaviour, and probe both RPC and net.

Depends on D4729

Test Plan

make check
test_runner.py --extended

Diff Detail

Repository

rABC Bitcoin ABC

Branch

limitfree

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 8565
Build 15120: Default Diff Build & Tests
Build 15119: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Dec 15 2019, 08:16

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 15 2019, 08:16

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8565: Diff 14866.Dec 15 2019, 08:31

markblundeberg mentioned this in D4729: fix fee estimation bug in functional tests.Dec 15 2019, 09:11

While re-testing locally I got a free-transaction error on line 82 of abc-high_priority_transaction... Probably because I increased the -limitfreerelay just barely enough for it to pass, so it's unreliable.

OK looking at debug.log, indeed upon restarting the node, dFreeCount got incremented up to 24705 on startup (while loading the saved mempool). Then 150 more lowfee txns got inserted and the 149th bumped dFreeCount up to 49856.8... and the 150th put it just over the top.

It seems the dFreeCount increments are a bit irregular, some are 157 (spending from P2PK) and some are 191 (spending from P2PKH), presumably due to how create_confirmed_utxos uses listunspent() and thus gets a mixture of coinbase / non-coinbase.

So setting the first phase to 3 and second phase to 6 (not 5) will be entirely sufficient even assuming the worst case of all-p2pkh. For comparison the old default value (before https://github.com/bitcoin/bitcoin/pull/9179 set it to 0) was 15.

Bump limitfree in the test from 5 to 6, and add explanation for why
these numbers are chosen. Also test that the mempool reloads fully
on restart.

Build Bitcoin-ABC / Diffs / Diff Testing started.

By the way, this free transaction system does not seem very robust, and it's not suitable for zero conf since it's easy to disturb in malicious ways. For example you can spam someone with a bunch of lowfee high priority transactions that are nonstandard, causing their free limit to fill up (because they add to the free limit regardless of acceptance!). So our default limit of -limitfreerelay=0, which disables this mechanism altogether, is actually a good choice in terms of keeping objective uniform relay policy.

I think it's a good idea to remove the code from ATMP, and if we ever actually need free transactions then we can design a more reliable and less subjective system (we can keep it based on coindays of course, that is nice!).

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8566: Diff 14867.Dec 15 2019, 10:26

IMO, we should just cleanup this whole thing. If someone wants to keep it alive, they can step up and maintain it. Let's not burden ourselves any more than required.

This revision now requires changes to proceed.Dec 16 2019, 13:25

In D4730#113949, @deadalnix wrote:

IMO, we should just cleanup this whole thing. If someone wants to keep it alive, they can step up and maintain it. Let's not burden ourselves any more than required.

What kind of cleanup do you have in mind? If it were up to me, I'd just make it count as if -limitfreerelay=0 and fLimitFree=true always. In other words:

remove priority transactions exception -- all txns need to meet the fee limit.
remove the fLimitFree clause (it will never matter, once priority exception is gone).
change the error message to something nicer like 'low fee' (everyone is confused by 'insufficient priority').
relaypriority and limitfreerelay options gone too.
also do the low fee check *before* the mempool fee floor check. This is so that when you get the 'mempool min fee not met' error you know for sure it was due to congestion, which is a more alarming circumstance than accidentally preparing a low fee txn.