Skip to content

Conversation

theStack
Copy link
Contributor

@theStack theStack commented Feb 7, 2021

The function xor_bytes was introduced in commit 3c22663 (#19953, BIP340-342 validation), even code-reviewed, but actually never used. The default signing algorithm in BIP340 needs a xor operation, but this step is currently done by a single xor operation on large integer operands:

t = (sec ^ int.from_bytes(TaggedHash("BIP0340/aux", aux), 'big')).to_bytes(32, 'big')

Alternatively, we could keep the function and as well use it:

--- a/test/functional/test_framework/key.py
+++ b/test/functional/test_framework/key.py
@@ -492,7 +492,7 @@ def sign_schnorr(key, msg, aux=None, flip_p=False, flip_r=False):
     P = SECP256K1.affine(SECP256K1.mul([(SECP256K1_G, sec)]))
     if SECP256K1.has_even_y(P) == flip_p:
         sec = SECP256K1_ORDER - sec
-    t = (sec ^ int.from_bytes(TaggedHash("BIP0340/aux", aux), 'big')).to_bytes(32, 'big')
+    t = xor_bytes(sec.to_bytes(32, 'big'), TaggedHash("BIP0340/aux", aux))
     kp = int.from_bytes(TaggedHash("BIP0340/nonce", t + P[0].to_bytes(32, 'big') + msg), 'big') % SECP256K1_ORDER
     assert kp != 0
     R = SECP256K1.affine(SECP256K1.mul([(SECP256K1_G, kp)]))

@fanquake
Copy link
Member

fanquake commented Feb 9, 2021

According to vulture there is more unused code in the same file:

bitcoin/test/functional/test_framework/key.py:27: unused function 'xor_bytes' (60% confidence)
bitcoin/test/functional/test_framework/key.py:277: unused method 'verify_ecdsa' (60% confidence)
bitcoin/test/functional/test_framework/key.py:517: unused variable 'sign_pubkey' (60% confidence)

@theStack
Copy link
Contributor Author

theStack commented Feb 9, 2021

@fanquake: Thanks for pointing out. verify_ecdsa is indeed unused, but was in the dead-code linters whitelist since its introduction (removed recently in f4beb49), so I guess the community's wish is to keep it (to have a complete Python ECDSA implementation)? Force-pushed now with the change of replacing sign_pubkey with an underscore.

@theStack theStack force-pushed the 2021-test-remove_unused_xor_bytes branch from 15c275c to f64adc1 Compare February 9, 2021 21:45
@practicalswift
Copy link
Contributor

cr ACK f64adc1: untested unused code should be removed

@maflcko maflcko merged commit 8d6994f into bitcoin:master Feb 15, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 15, 2021
f64adc1 test: remove unused function xor_bytes (Sebastian Falbesoner)

Pull request description:

  The function `xor_bytes` was introduced in commit 3c22663 (bitcoin#19953, BIP340-342 validation), even [code-reviewed](https://github.com/bitcoin/bitcoin/pull/19953/files#r509383731), but actually never used. The [default signing algorithm in BIP340](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki#Default_Signing) needs a xor operation, but this step is currently done by a single xor operation on large integer operands:

  ```
  t = (sec ^ int.from_bytes(TaggedHash("BIP0340/aux", aux), 'big')).to_bytes(32, 'big')
  ```

  Alternatively, we could keep the function and as well use it:
  ```diff
  --- a/test/functional/test_framework/key.py
  +++ b/test/functional/test_framework/key.py
  @@ -492,7 +492,7 @@ def sign_schnorr(key, msg, aux=None, flip_p=False, flip_r=False):
       P = SECP256K1.affine(SECP256K1.mul([(SECP256K1_G, sec)]))
       if SECP256K1.has_even_y(P) == flip_p:
           sec = SECP256K1_ORDER - sec
  -    t = (sec ^ int.from_bytes(TaggedHash("BIP0340/aux", aux), 'big')).to_bytes(32, 'big')
  +    t = xor_bytes(sec.to_bytes(32, 'big'), TaggedHash("BIP0340/aux", aux))
       kp = int.from_bytes(TaggedHash("BIP0340/nonce", t + P[0].to_bytes(32, 'big') + msg), 'big') % SECP256K1_ORDER
       assert kp != 0
       R = SECP256K1.affine(SECP256K1.mul([(SECP256K1_G, kp)]))
  ```

ACKs for top commit:
  practicalswift:
    cr ACK f64adc1: untested unused code should be removed

Tree-SHA512: e9afae303488f19c6f6f44874d3537ed1c8164a197490e2b4e34853db886b858825b719648fa1a30b95177dcee9cf241f94ee9b835f0a2cae07024ce38a8c090
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants