Page MenuHomePhabricator

Enforce NULLDUMMY
Needs ReviewPublic

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

Details

Reviewers
deadalnix
jasonbcox
Fabien
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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 4996
Build 8055: Bitcoin ABC Teamcity Staging
Build 8054: arc lint + arc unit

Event Timeline

dagurval created this revision.Jan 19 2019, 00:33
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 19 2019, 00:33
Herald added a subscriber: schancel. · View Herald Transcript
dagurval edited the summary of this revision. (Show Details)Jan 19 2019, 00:36
deadalnix added inline comments.Jan 19 2019, 00:49
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.Mon, Jan 28, 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.Mon, Jan 28, 23:52
dagurval updated this revision to Diff 6980.Tue, Jan 29, 09:32

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

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

Fix name error in parameter to bitcoind

Fabien requested changes to this revision.Tue, Jan 29, 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.Tue, Jan 29, 17:08
dagurval updated this revision to Diff 7002.Tue, Jan 29, 22:41
dagurval marked 3 inline comments as done.

Fix abc-replay-protection + nits

jasonbcox requested changes to this revision.Tue, Jan 29, 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.Tue, Jan 29, 23:28
dagurval added inline comments.Tue, Jan 29, 23:31
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.Tue, Jan 29, 23:31
Fabien requested changes to this revision.Thu, Jan 31, 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.Fri, Feb 1, 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.

dagurval added inline comments.Thu, Feb 7, 13:40
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.

dagurval added inline comments.Thu, Feb 7, 13:41
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.

markblundeberg added inline comments.Fri, Feb 8, 17:06
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)

dagurval updated this revision to Diff 7356.Fri, Feb 15, 13:14

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.