Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ctime_test: move context randomization test to the end #894

Merged
merged 1 commit into from
Feb 22, 2021

Conversation

jonasnick
Copy link
Contributor

I noticed this while reviewing BlockstreamResearch/secp256k1-zkp#117 and finding some seemingly unnecessary VALGRIND_MAKE_MEM_DEFINED that I couldn't remove until I saw the bug.

@gmaxwell
Copy link
Contributor

gmaxwell commented Feb 5, 2021

ah, indeed the comment said it needed to be last. :) Did you look into why it wasn't causing failures with the schnorrsig test?

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

nice catch and good idea to extract a function

edit: Ah, I didn't want to github-"approve". Anyway, we don't give attention to this.

src/valgrind_ctime_test.c Outdated Show resolved Hide resolved
@jonasnick
Copy link
Contributor Author

Did you look into why it wasn't causing failures with the schnorrsig test?

The reason is that in ctime_test.c we declassify public keys after creating them. In the case of the schnorrsig & extrakeys tests, they only operate on keypairs, and the public part of a keypair is declassified on keypair_load because it needs to branch on it.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 7d3497c diff looks good

@jonasnick jonasnick merged commit 3a8b47b into bitcoin-core:master Feb 22, 2021
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 23, 2021
Summary:
the idea is to have a markdown convention that will link to the two repos we already backport from and include the bitcoin-core/gui repo

after this diff I establish the following convention:

summary text in the format LOWERCASE_SUPPORTED_REPO_PREFIX#PR_NUMBER will be replaced with a link to the appropriate PR if LOWERCASE_SUPPORTED_REPO_PREFIX is in the relevant dictionary in server.py

examples:

core#17234 should replace to [[bitcoin/bitcoin#17234 | core#17234]]

core-gui#9 to [[bitcoin-core/gui#9 | core-gui#9]]

and secp256k1#894 to [[bitcoin-core/secp256k1#894 | secp256k1#894]]

Test Plan:
  pytest

Reviewers: Fabien, #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D9258
jesseposner added a commit to jesseposner/secp256k1-zkp that referenced this pull request Mar 5, 2021
This commit adds test coverage including Travis scripts, Valgrind
constant time tests for secret data, API tests, nonce function tests,
and test vectors from the spec.

This commit includes changes from
bitcoin-core/secp256k1#894 that fix a bug
upstream.
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Apr 8, 2021
Summary:
```
I noticed this while reviewing BlockstreamResearch/secp256k1-zkp#117 and
finding some seemingly unnecessary VALGRIND_MAKE_MEM_DEFINED that I
couldn't remove until I saw the bug.
```

Backport of [[bitcoin-core/secp256k1#894 | secp256k1#894]]

Test Plan:
  ninja check-secp256k1

  libtool --mode=execute valgrind ./valgrind_ctime_test

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9389
deadalnix pushed a commit to Bitcoin-ABC/secp256k1 that referenced this pull request Apr 9, 2021
Summary:
```
I noticed this while reviewing BlockstreamResearch/secp256k1-zkp#117 and
finding some seemingly unnecessary VALGRIND_MAKE_MEM_DEFINED that I
couldn't remove until I saw the bug.
```

Backport of [[bitcoin-core/secp256k1#894 | secp256k1#894]]

Test Plan:
  ninja check-secp256k1

  libtool --mode=execute valgrind ./valgrind_ctime_test

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9389
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