Page MenuHomePhabricator

Schnorr signature activation
ClosedPublic

Authored by markblundeberg on Feb 2 2019, 20:25.

Details

Summary

Includes python activation tests based on replay protection activation.

Features:

  • Tests that when upgrade / downgrade happens, newly-invalid transactions are dumped from mempool (but not valid ones).
  • Don't ban peers who send a tx that would be valid under the mandatory flags on other side of upgrade/downgrade.
  • Tests of 64-byte DER signatures
Test Plan

run functional tests
uncomment fakeDER64 and patch VerifySignature (see comment in file about fakeDER64)
run abc-schnorr-activation.py again

(I have also run these tests with the various critical bits of mainline code cancelled out, and verified the tests fail as expected.)

Diff Detail

Repository
rABC Bitcoin ABC
Branch
schnorr_activation
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4937
Build 7937: Bitcoin ABC Buildbot (legacy)
Build 7936: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

address comments; fix up variable names

Also changed upgrade-mismatch mempool acceptance error from standardness to a more appropriate error message.

markblundeberg edited the test plan for this revision. (Show Details)
test/functional/abc-schnorr-activation.py
286 ↗(On Diff #7290)

The commend lines with GREAT_WALL_START_TIME + x fill up reading space. I think removing them is best. Readers can easily refer to next_block in this same file to see what the timestamps are.

3

Mengerian is correct here

13

I see. thanks.

37

got it. and yes, the names could improve :P

340

👍

markblundeberg added inline comments.
test/functional/abc-schnorr-activation.py
37

let me know what you think of the new error names

Fabien requested changes to this revision.Feb 12 2019, 11:32
Fabien added a subscriber: Fabien.

Bunch of nits in the test

test/functional/abc-schnorr-activation.py
30 ↗(On Diff #7306)

Add spaces: GREAT_WALL_START_TIME * 2

73 ↗(On Diff #7290)

Move the comment above

146 ↗(On Diff #7290)

This is not necessary, as it is done by make_conform_to_ctor

164 ↗(On Diff #7290)

Nit: self.nodes[0] is not a function

185 ↗(On Diff #7290)

Use spaces b"Schnorr!" * 4

188 ↗(On Diff #7290)

Move comment above

235 ↗(On Diff #7290)

This is convention only, but _ is usually the name of an unused variable, which is not the case here

This revision now requires changes to proceed.Feb 12 2019, 11:32

fix nits that Fabien pointed out

test/functional/abc-schnorr-activation.py
58 ↗(On Diff #7310)

Thinking out loud : maybe a debug flag could be added to bypass the script evaluation, allowing for testing the fakeDER64 signature in the test in a permanent manner ? Maybe only allowed if the node is running on regtest.
Thought ?

jasonbcox added inline comments.
test/functional/abc-schnorr-activation.py
37

My critique of the naming is it sounds like "before an error" and "after an error" because it doesn't say what it's before and after. just a nit though

test/functional/abc-schnorr-activation.py
85 ↗(On Diff #7310)

Please use format()instead

86 ↗(On Diff #7310)

dito

test/functional/abc-schnorr-activation.py
26 ↗(On Diff #7322)

Now that the replay protection has been postponed to the graviton upgrade date, does it make sense to use the real Great Wall time for this test?

test/functional/abc-schnorr-activation.py
58 ↗(On Diff #7310)

Seems a bit too complicated for what we want, though indeed such a test would be more robust.

Note that this will anyway be removed in May. It's only because of that that I'm comfortable having it done in this awkward way.

85 ↗(On Diff #7310)

👍 will do

86 ↗(On Diff #7310)

👍 will do

37

yeah the same thing was bugging at me, so I think it should be changed. :-)

markblundeberg marked an inline comment as done.

various:

  • rebase on now-landed D2479 (+ address conflicts in overlap)
  • now fully mock-timed
  • added actual mininode testing of non-banning (borrowed from D2479)
  • added a reorg test at end, for good measure
  • nits
markblundeberg edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.Feb 14 2019, 22:57

Ok the general flow of the code and test looks good. However I'm not convinced that the check for banning actually checks anything. Can you remove the whitelist and then make sure to test manually that the node gets banned when sending a transaction that is actually invalid, for instance a schnorr multisig transaction?

test/functional/abc-schnorr-activation.py
96 ↗(On Diff #7334)

If 127.0.0.1 is whitelisted, does the banning test actually test anything ? I'm not confident it does.

200 ↗(On Diff #7334)

:)

296 ↗(On Diff #7334)

Please add a list of transaction as a parameter to block(). That would be better than reproducing the same pattern all the time.

462 ↗(On Diff #7334)

Well, as long as things do not get too deep :)

This revision now requires changes to proceed.Feb 14 2019, 22:57
  • add extra node for doing actual ban-checking (whitelist was stopping this)
  • include positive ban tests (for schnorr multisig)
  • get rid of update_block
This revision is now accepted and ready to land.Feb 15 2019, 03:06