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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 4965
Build 7993: Bitcoin ABC Teamcity Staging
Build 7992: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
markblundeberg added inline comments.Feb 10 2019, 06:05
src/validation.cpp
1230 ↗(On Diff #7265)

A thought -- I suppose I should use REJECT_INVALID with a different error name here instead of "non-mandatory-script-verify-flag"

What about "upgrade-conditional-script-failure" or something like that?

address comments; fix up variable names

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

markblundeberg marked 8 inline comments as done.Feb 10 2019, 17:17
markblundeberg edited the summary of this revision. (Show Details)Feb 10 2019, 17:39
markblundeberg edited the test plan for this revision. (Show Details)
jasonbcox added inline comments.Feb 11 2019, 22:15
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 ↗(On Diff #7265)

Mengerian is correct here

13 ↗(On Diff #7265)

I see. thanks.

37 ↗(On Diff #7265)

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

340 ↗(On Diff #7265)

👍

rebased; shorten comment

markblundeberg marked an inline comment as done.Feb 12 2019, 00:34
markblundeberg added inline comments.
test/functional/abc-schnorr-activation.py
37 ↗(On Diff #7265)

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

Fabien added inline comments.Feb 12 2019, 17:32
test/functional/abc-schnorr-activation.py
58

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 accepted this revision.Feb 12 2019, 23:36
jasonbcox added inline comments.
test/functional/abc-schnorr-activation.py
37 ↗(On Diff #7265)

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

Fabien added inline comments.Feb 13 2019, 09:09
test/functional/abc-schnorr-activation.py
85

Please use format()instead

86

dito

rebase after D2527 landing

markblundeberg added inline comments.Feb 13 2019, 17:36
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?

markblundeberg added inline comments.Feb 13 2019, 17:42
test/functional/abc-schnorr-activation.py
58

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

👍 will do

86

👍 will do

37 ↗(On Diff #7265)

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

markblundeberg planned changes to this revision.Feb 14 2019, 00:17
markblundeberg marked 3 inline comments as done.Feb 14 2019, 01:59
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 summary of this revision. (Show Details)Feb 14 2019, 02:15
markblundeberg edited the test plan for this revision. (Show Details)
Fabien accepted this revision.Feb 14 2019, 20:26
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
deadalnix accepted this revision.Feb 15 2019, 03:06
This revision is now accepted and ready to land.Feb 15 2019, 03:06
This revision was automatically updated to reflect the committed changes.