Page MenuHomePhabricator

do not accept anything into mempool before UAHF has activated
AbandonedPublic

Authored by markblundeberg on Dec 12 2019, 21:56.

Details

Reviewers
Fabien
Mengerian
deadalnix
Group Reviewers
Restricted Project
Summary

Although a node during IBD will not request transactions from peers, it
is possible for it to be sent unsolicited transactions from peers, or,
some RPC method might try to inject a transaction into the mempool.

When we are before UAHF, this will end up rejecting transactions due to
mismatching sighash. But this is only due to a last-minute check and this
will generate some harmless but scary sounding "BUG! PLEASE REPORT THIS!"
log messages.

So, just as a sanity check, we can fast-reject all transactions when our
chain is before the UAHF. Even if some transactions were to be accepted,
they would likely just end up slowing down IBD anyway (since mempool needs
to keep getting updated) and anyway, the tx would likely end up getting
discarded by some double spent conflict.

As a result, the scary log message should only appear in actually
scary situations.

Note: it is not really possible to unit test this error code since regtest
UAHF activates at height=0; however we can try putting in graviton
activation instead, which causes a fair number of functional tests to fail,
not just the graviton activation tests, but also some other tests that use
the 'setup chain' and don't drive themselves out of IBD. There are some
future backports that will make tests start out of IBD, by default. Until
then this rejection has to be restricted to activations that occur at
height=0 on regtest.

Test Plan
make check
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
uahf-reject
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 8533
Build 15061: Default Diff Build & Tests
Build 15060: arc lint + arc unit

Event Timeline

This is the future backport that will make all tests start out of IBD: https://github.com/bitcoin/bitcoin/pull/15419

Unfortunately, it has some slight dependencies on earlier backports.

jasonbcox added inline comments.
src/consensus/validation.h
19

I was going to suggest using 0x80 or something so as not to collide with any new message codes added by Core. As it turns out, they removed BIP61 reject messages... I guess we're in the clear there. :P

Something like this should really have IBD run on it as part of the test plan. I've run it for you.

Yeah to be clear, I'm not 100% sure that this is something we want or that this is the right way to do it, and I'm curious what you guys think.

I have tried running this with graviton activation in place of UAHF, with a massaged backport of #15419 put in place. Indeed as I expected, this only causes the graviton feature activation tests to fail (since they try to submit txns before activation).

However, once we move graviton activation to height=0 on regtest, we can bump this from UAHF->graviton without any issue, regardless of which backports have been done.

deadalnix requested changes to this revision.Dec 13 2019, 02:40

Add a test.

This revision now requires changes to proceed.Dec 13 2019, 02:40

Add a test.

In principle this could be tested, by having the 'too-early' height be configurable by a command-line option; defaulting to UAHF height. On regtest this would default to height=0 (UAHF activation) but we could have one functional test where it sets that param to a nonzero value. Do you think it's worth the trouble?

Note that one thing that is being tested for us already is that this doesn't affect anything that happens after UAHF -- all the tests continue to work perfectly.

@Fabien suggested we could alternatively use IBD as the marker (though unfortunately this would again impact those tests which fail to leave IBD).

Another alternative is we can check that the tip is past nMinimumChainWork -- this is 0 on regtest by default, but configurable with -minimumchainwork which means it can be easily tested.

Fabien added some comments to this effect in D4642.