Page MenuHomePhabricator

Enforce NULLDUMMY
AbandonedPublic

Authored by Mengerian on Jan 19 2019, 00:33.

Details

Reviewers
deadalnix
jasonbcox
Fabien
dagurval
Group Reviewers
Restricted Project
Summary

Parts of test code is based on work by laanwj in Core PR8636

Test Plan

Tests added

Diff Detail

Repository
rABC Bitcoin ABC
Branch
nulldummy
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4733
Build 7529: Bitcoin ABC Buildbot (legacy)
Build 7528: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
src/consensus/activation.h
28 ↗(On Diff #6738)

The next upgrade is referred in the codebase a "great wall". You can find for instance the greatWallActivationTime field in the chainparams. You should use this to activate NULLDUMMY.

jasonbcox requested changes to this revision.Jan 28 2019, 23:52
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
test/functional/nulldummy.py
53 ↗(On Diff #6738)

Please use self.tip = int(self.nodes[0].getbestblockhash(), 16) instead. I know there's some old code that still uses this...

64 ↗(On Diff #6738)

This refactor can be applied as a separate diff to help speed up review of this diff.

This revision now requires changes to proceed.Jan 28 2019, 23:52

Update after prerequisite todo items have landed. Cleanup and rebase.

dagurval retitled this revision from WIP: Enforce NULLDUMMY to Enforce NULLDUMMY.Jan 29 2019, 09:33
dagurval edited the summary of this revision. (Show Details)

Fix name error in parameter to bitcoind

Fabien requested changes to this revision.Jan 29 2019, 17:08
Fabien added a subscriber: Fabien.

The abc-replay-protection.py scripts fails when NULLDUMMY is enforced.
I suggest you to add "-greatwallactivationtime={}".format(2 * REPLAY_PROTECTION_START_TIME) to the node arguments in this script to avoid the failure, then it can be fixed later.

test/functional/abc-nulldummy-activation.py
36 ↗(On Diff #6981)

Update codename to great wall

test/functional/feature_nulldummy.py
51 ↗(On Diff #6981)

Prefer using int(self.hash, 16) instead

74 ↗(On Diff #6981)

coinbaes => coinbase

This revision now requires changes to proceed.Jan 29 2019, 17:08
dagurval marked 3 inline comments as done.

Fix abc-replay-protection + nits

jasonbcox requested changes to this revision.Jan 29 2019, 23:28
jasonbcox added inline comments.
test/functional/abc-nulldummy-activation.py
30 ↗(On Diff #7002)

GREAT_WALL_ACTIVATION_TIME * 2 instead, then you can remove IN_THE_FUTURE, which is unnecessary to have a separate definition.

test/functional/test_framework/cdefs.py
95 ↗(On Diff #7002)

The tests shouldn't care about the actual activation time. It's an unnecessary dependency. Look at other tests for *_ACTIVATION_TIME and you'll see them set to arbitrarily large values like 9999999. This should be sufficient to set a value like this in all tests where it's needed.

This revision now requires changes to proceed.Jan 29 2019, 23:28
test/functional/abc-nulldummy-activation.py
30 ↗(On Diff #7002)

GREAT_WALL_ACTIVATION_TIME * 2 is a magic number that makes the intention unclear

test/functional/test_framework/cdefs.py
95 ↗(On Diff #7002)

And they don't care, but may as well mimic reality as closely as possible

dagurval marked 2 inline comments as not done.Jan 29 2019, 23:31
Fabien requested changes to this revision.Jan 31 2019, 08:45

One nit, looks good to me otherwise.
Please request review when you're adding comments so the diff pops up in the review queue again.

test/functional/abc-nulldummy-activation.py
29 ↗(On Diff #7002)

Please use the format method (e.g. '-greatwallactivationtime={}'.format(GREAT_WALL_ACTIVATION_TIME)) instead of % for string formatting

deadalnix requested changes to this revision.Feb 1 2019, 00:56
deadalnix added inline comments.
test/functional/abc-nulldummy-activation.py
13 ↗(On Diff #7002)

Please chose a fixed value so the test is repeatable.

29 ↗(On Diff #7002)

Having a linter yelling at people for this would be beneficial. % syntax is so much nicer, idk why pything is deprecating it :(

30 ↗(On Diff #7002)

@dagurval is correct. However, it indeed needs to be further in the future than GREAT_WALL_ACTIVATION_TIME is.

92 ↗(On Diff #7002)

The structure of this test is very confusing.

Here is a good test plan:

  1. Check that the tx is accepted, both when sent to the mempool and in blocks (you may want to disable the standardness check).
  2. Move the MTP to GREAT_WALL_ACTIVATION_TIME - 1
  3. Check that you get the same as 1.
  4. Move the MTP to GREAT_WALL_ACTIVATION_TIME
  5. Check that the tx is rejected from the mempool.
  6. Check that the tx cannot be mined and it is accepted in a block.
  7. invalid another block.
  8. tx can be sent back in the mempool.

You can use tests for the activation of the last fork as examples. Very tight testing is required to detect problem such as off by one errors. You probably want to have two nodes, one set to accept non standard txns, and one set to reject. You also want to check that node do nto ban each others when one activated teh fork and one did not.

test/functional/abc-replay-protection.py
61 ↗(On Diff #7002)

Just launch the node with GREAT_WALL_ACTIVATION_TIME set absurdly far away in the future.

test/functional/feature_nulldummy.py
16 ↗(On Diff #7002)

No.

39 ↗(On Diff #7002)

While I have nothing aainst these per se, mixing refactoring with logic change makes the hole changeset very hard to work with.

Please split refactoring out of this diff so we can see clearly what logic changed.

63 ↗(On Diff #7002)

Isn't this completely redundant ?

test/functional/test_framework/cdefs.py
95 ↗(On Diff #7002)

Choosing a value that has nothing to do with anything in the cpp codebase ensure that this doesn't just happen to be working for reasons others than what we expect. For instance, using this, if the command line parameter do not work, we wouldn't know.

activation_tests.cpp actually checks that the trigger is set at the right time.

@dagurval I'm working on a very similar thing in D2483, perhaps our efforts would benefit from cross-pollination here.

test/functional/abc-nulldummy-activation.py
92 ↗(On Diff #7002)

This script is to simply test that the mempool is cleared on activation. Its here for completeness only as it affects most likely no one at all, as there are no miners accepting non-standard transactions.

What you're looking for is in the other test. It's directly based on Core's activation of nulldummy and does no more, no less. It's not as extensive as what you're asking for though.

test/functional/abc-replay-protection.py
61 ↗(On Diff #7002)

That's worse, not better.

markblundeberg added inline comments.
src/validation.cpp
1928 ↗(On Diff #7002)

@florian is working on on an improvement for this btw, that doesn't involve throwing away everyone's transactions unnecessarily. -- all three activations (segwit recovery, nulldummy, schnorr) will benefit.

test/functional/abc-nulldummy-activation.py
92 ↗(On Diff #7002)

To be honest I'd suggest taking a look at abc-replay-activation.py and building off that instead. It uses more of the modern test equipment and it's closer to what Amaury wants to see.

test/functional/abc-nulldummy-activation.py
92 ↗(On Diff #7002)

(you can take a look at D2483 to see how I adapted it to Schnorr, for instance)

New approach. Instead of modifing the existing nulldummy test, create a new test following deadalnix's test plan above.

Hey @dagurval It seems this is now too late to include for "Great Wall" activation.

I would suggest, however, that you keep working on it and just change the activation to "Graviton". I see no reason why this Diff shouldn't be added in fairly soon with Nov 2019 activation.

Mengerian added a reviewer: dagurval.

Going to try to revive this for Nov 2019 Upgrade

Mengerian planned changes to this revision.

Work in progress

Work in progress
NULLDUMMY activation ported to Graviton
tests updated to work with recent refactors
To Do: check node banning behavior

After more investigation and discussion, I have decided to add banning exception, and re-implement the test based on the recent Schnorr and Segwit Recovery.

So it makes more sense to abandon this, and start again from scratch.