Skip to content

Conversation

theuni
Copy link
Contributor

@theuni theuni commented Oct 22, 2014

Not sure if tests are necessary here. I've probably oversimplified, but this was enough to get bitcoin's tests passing again with libsecp256k1.

Makes S negation (when above order / 2) optional.
@gmaxwell
Copy link
Contributor

What? I don't think this should be a flag. What test is failing?

@theuni
Copy link
Contributor Author

theuni commented Oct 22, 2014

<coryfields_> sipa: i've fixed up the libsecp256k1 in bitcoin (testing it in libbitcoinconsensus), and it's having trouble with a particular test. Known issue for "P2PK with high S" ?
<sipa> coryfields_: that sounds very expected
<coryfields_> ok, will just skip over it
<sipa> it needs a change in secp256k1's api, i'm afraid
<sipa> as you can't tell it to not use low-S
<coryfields_> figured as much, specifying high/low S i suppose
<coryfields_> ?
<coryfields_> right
<sipa> yup
<coryfields_> want me to take a crack at it? or is it waiting on something?
<sipa> it's trivial to do, you can if you want to

By "trivial" + api change, I assumed he was just referring to a flag similar to the one our openssl wrapper uses.

@gmaxwell
Copy link
Contributor

okay if sipa says so! seems odd to introduce an call just to get that behavior.

@theuni
Copy link
Contributor Author

theuni commented Oct 22, 2014

Not sure if this is actually what he was after or not, it was just the most trivial way I could come up with.

I suppose for a bit of future-proofing we could use a bitfield param instead, where lowS is the only valid flag for now.

@sipa
Copy link
Contributor

sipa commented Oct 26, 2014

@theuni This is indeed exactly what I meant.
@gmaxwell It's only needed for unit tests, to be able to generate cases with longer S values. Alternatively, it could be implemented by having parse/serialize/subtraction bignum functionality in the unit tests.

@sipa
Copy link
Contributor

sipa commented Nov 5, 2014

I've implemented a replacement on the bitcoin side: bitcoin/bitcoin@4a69b3b

@sipa
Copy link
Contributor

sipa commented Nov 12, 2014

Closing in favor of bitcoin/bitcoin#5256.

@sipa sipa closed this Nov 12, 2014
achow101 added a commit to bitcoin/bitcoin that referenced this pull request May 9, 2025
1ee698f test: refactor: negate signature-s using libsecp256k1 (Sebastian Falbesoner)

Pull request description:

  This small PR gets rid of manual mod-n inversion of the ECDSA signature-s part in unit tests (introduced a long time ago in #5256, triggered by bitcoin-core/secp256k1#69) by using secp256k1 instead. The function wasn't available at that time, but was introduced about three years later, see bitcoin-core/secp256k1#408. Note that as the name suggests, `secp256k1_ec_seckey_negate` is meant to be used for secret keys, but it obviously works in general for scalars modulo the group order.

ACKs for top commit:
  achow101:
    ACK 1ee698f
  laanwj:
    Code review ACK 1ee698f
  w0xlt:
    ACK 1ee698f
  rkrux:
    tACK 1ee698f

Tree-SHA512: dc36ea1572b538d11ae34e1871f310a1cda8083ffb753e93e7ee9d56e91ebd8ec78d35758dfb700254720914b734ef7a071eeef71b6239f19e1e2fb289fb5435
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants