Page MenuHomePhabricator

Schnorr signature activation

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



Includes python activation tests based on replay protection activation.


  • 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 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

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
markblundeberg added inline comments.Sun, Feb 10, 05:53
264 β†—(On Diff #7265)


340 β†—(On Diff #7265)

see the if-block just above
(hard to make valid-passing 65-byte ecdsa sigs without that fakeDER hack)

Mengerian added inline comments.Sun, Feb 10, 06:00
3 β†—(On Diff #7265)

Copyright law doesn't care about new files, only if it's a "derivative work".

So if it's "derived" from another file, it should retain the copyright notices from that file.

13 β†—(On Diff #7265)

Following from the Copyright, it's good to say where it's derived from.

If you look in, it also has a similar comment saying where it was derived from.

markblundeberg added inline comments.Sun, Feb 10, 06:05
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.Sun, Feb 10, 17:17
markblundeberg edited the summary of this revision. (Show Details)Sun, Feb 10, 17:39
markblundeberg edited the test plan for this revision. (Show Details)
jasonbcox added inline comments.Mon, Feb 11, 22:15
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.Tue, Feb 12, 00:34
markblundeberg added inline comments.
37 β†—(On Diff #7265)

let me know what you think of the new error names

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

Bunch of nits in the test

30 β†—(On Diff #7306)


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.Tue, Feb 12, 11:32

fix nits that Fabien pointed out

Fabien added inline comments.Tue, Feb 12, 17:32
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 accepted this revision.Tue, Feb 12, 23:36
jasonbcox added inline comments.
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.Wed, Feb 13, 09:09
85 β†—(On Diff #7310)

Please use format()instead

86 β†—(On Diff #7310)


rebase after D2527 landing

markblundeberg added inline comments.Wed, Feb 13, 17:36
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.Wed, Feb 13, 17:42
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 β†—(On Diff #7265)

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

markblundeberg planned changes to this revision.Thu, Feb 14, 00:17
markblundeberg marked 3 inline comments as done.Thu, Feb 14, 01:59
markblundeberg marked an inline comment as done.


  • 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)Thu, Feb 14, 02:15
markblundeberg edited the test plan for this revision. (Show Details)
Fabien accepted this revision.Thu, Feb 14, 20:26
deadalnix requested changes to this revision.Thu, Feb 14, 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?

96 β†—(On Diff #7334)

If 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.Thu, Feb 14, 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.Fri, Feb 15, 03:06
This revision is now accepted and ready to land.Fri, Feb 15, 03:06
Closed by commit rABC6bb69585f326: Schnorr signature activation (authored by Mark Lundeberg <>). Β· Explain WhyFri, Feb 15, 03:19
This revision was automatically updated to reflect the committed changes.