Taproot wallet test vectors (generation+tests) #23394

pull sipa wants to merge 7 commits into bitcoin:master from sipa:202110_taprootunit changing 13 files +943 −55
  1. sipa commented at 2:21 pm on October 30, 2021: member

    This PR adds code to test/functional/feature_taproot.py which runs through a (deterministic) scenario covering several aspects of the wallet side of BIP341 (scriptPubKey computation from keys/scripts, control block computation, key path spending), with the ability to output test vectors in mediawiki format based on this scenario. The generated tests are then also included directly in src/test/script_tests.cpp and src/test/script_standard_tests.cpp.

    The test vectors generated here were added to BIP341 in https://github.com/bitcoin/bips/pull/1225

  2. sipa force-pushed on Oct 30, 2021
  3. DrahtBot added the label Consensus on Oct 30, 2021
  4. DrahtBot added the label Utils/log/libs on Oct 30, 2021
  5. sipa force-pushed on Oct 30, 2021
  6. DrahtBot commented at 10:37 pm on October 30, 2021: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    No conflicts as of last run.

  7. in test/functional/test_framework/key.py:382 in b9de4f1ce0 outdated 
    378@@ -368,15 +379,18 @@ def get_pubkey(self):
379         ret.compressed = self.compressed
380         return ret
381 
382-    def sign_ecdsa(self, msg, low_s=True):
383+    def sign_ecdsa(self, msg, low_s=True, rfc6979=False):
    junderw commented at 2:57 am on November 2, 2021:
    Why was this added? It isn’t used anywhere in the changes.
    sipa commented at 3:55 am on November 2, 2021:
    It is, indirectly through the “deterministic” flag in the feature_taproot’s context, which on its turn is used in the test vector scenario (to make sure the fully signed transaction is exactly the same every run).
    junderw commented at 4:47 am on November 2, 2021:
    My mistake. I see it now. Thanks.
  8. sipa marked this as a draft on Nov 2, 2021
  9. sipa commented at 10:34 pm on November 2, 2021: member
    I’m going to convert this to construct tests in JSON format rather than Mediawiki, and also actually run it in that form directly.
  10. sipa force-pushed on Nov 5, 2021
  11. sipa marked this as ready for review on Nov 5, 2021
  12. sipa force-pushed on Nov 6, 2021
  13. in test/functional/feature_taproot.py:1532 in 1b466de0c1 outdated 
    1527+        script_lists = [
1528+            None,
1529+            [("0", CScript([pubs[50], OP_CHECKSIG]), 0xc0)],
1530+            [("0", CScript([pubs[51], OP_CHECKSIG]), 0xc0)],
1531+            [("0", CScript([pubs[52], OP_CHECKSIG]), 0xc0), ("1", CScript([b"BIP341"]), pubs[99][0] & 0xfe)],
1532+            [("0", CScript([pubs[53], OP_CHECKSIG]), 0xc0), ("1", CScript([b"Taproot"]), pubs[99][1] & 0xfe)],
    sanket1729 commented at 3:57 pm on November 6, 2021:
    It just happens that pubs[99][0] &0xfe and pubs[99][1] & 0xfe are not colliding with annex tag (0x50). Maybe add an assert along with the other asserts that the above is not the case?
    sipa commented at 5:27 pm on November 7, 2021:
    Fixed, by restricting the selection to the set of leaf versions recommended in the BIP.
  14. sipa force-pushed on Nov 6, 2021
  15. sipa force-pushed on Nov 7, 2021
  16. sipa commented at 8:47 pm on November 7, 2021: member
    Updated to address https://github.com/bitcoin/bips/pull/1225#pullrequestreview-799478503
  17. in src/key.cpp:264 in 5c4550cdd6 outdated 
    262@@ -263,6 +263,7 @@ bool CKey::SignCompact(const uint256 &hash, std::vector<unsigned char>& vchSig)
263 
264 bool CKey::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint256* merkle_root, const uint256* aux) const
    laanwj commented at 3:47 pm on November 8, 2021:

    Instead of changing the meaning of aux==nullptr here, what about:

    • Remove the default arguments from CKey::SignSchnorr (they’re unused)
    • Disallow nullptr by changing the argument to a const uint256& aux
    • Pass zero in from the caller if it wants to use zero

    This seems more explicit and clear to me? (The behavior is not documented in the doc comment, in any case, right now)

    sipa commented at 4:56 pm on November 8, 2021:
    Good idea, done.
  18. sipa force-pushed on Nov 8, 2021
  19. sipa force-pushed on Nov 8, 2021
  20. DrahtBot added the label Needs rebase on Nov 9, 2021
  21. sipa force-pushed on Nov 10, 2021
  22. sipa commented at 10:38 pm on November 10, 2021: member
    Rebased.
  23. DrahtBot removed the label Needs rebase on Nov 11, 2021
  24. Make signing follow BIP340 exactly w.r.t. aux randomness 
    libsecp256k1's secp256k1_schnorrsig_sign only follows BIP340 exactly
if an aux_rand32 argument is passed. When no randomness is used
(as is the case in the current codebase here), there is no impact
on security between not providing aux_rand32 at all, or providing
an empty one. Yet, for repeatability/testability it is simpler
to always use an all-zero one.
    2478c6730a
  25. tests: add more fields to TaprootInfo 5140825096
  26. tests: give feature_taproot access to sighash preimages a5bde018b4
  27. tests: abstract out precomputed BIP341 signature hash elements c98c53f20c
  28. tests: add deterministic signing mode to ECDSA 
    This does the following:
* Adds a rfc6979 argument to test_framework/key.py's sign_ecdsa to
  select (deterministic) RFC6979-based nonce generation.
* Add a flag in feature_taproot.py's framework called "deterministic".
* Make the Schnorr signing in feature_taproot.py randomized by default,
  reverting to the old deterministic (aux_rnd=0x0000...00) behavior
  if the deterministic context flag is set.
* Make the ECDSA signing in feature_taproot.py use RFC6979-based nonces
  when the deterministic context flag is set (keeping the old randomized
  behavior otherwise).
    ca83ffc2ea
  29. tests: BIP341 test vector generation ac3037df11
  30. tests: implement BIP341 test vectors f1c33ee4ac
  31. sipa force-pushed on Nov 12, 2021
  32. sipa commented at 5:09 pm on November 12, 2021: member
    Updated to address https://github.com/bitcoin/bips/pull/1225#discussion_r747460936.
  33. sipa commented at 0:03 am on November 13, 2021: member
    Corresponding BIP change was merged: https://github.com/bitcoin/bips/pull/1225
  34. laanwj commented at 6:38 pm on November 15, 2021: member
    Code review ACK f1c33ee4ac1056289f2e67b75755388549ada4ca Also checked that src/test/data/bip341_wallet_vectors.json matches the file in bip341.
  35. laanwj referenced this in commit 5ccab7187b on Nov 15, 2021
  36. laanwj commented at 8:36 pm on November 15, 2021: member
    Closing: PR was merged, github didn’t detect it
  37. laanwj closed this on Nov 15, 2021
  38. sidhujag referenced this in commit d9a9afd8d4 on Nov 16, 2021
  39. DrahtBot locked this on Nov 15, 2022


sipa DrahtBot junderw sanket1729 laanwj

Labels
Consensus Utils/log/libs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me