-
Notifications
You must be signed in to change notification settings - Fork 1k
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
schnorrsig API overhaul #844
Conversation
ping :) |
Concept ACK |
Can we use an opaque "array" struct for the config instead of using malloc? this will still allow us to extend the struct while also supporting platforms without heap allocation |
@elichai Yeah good point, I agree that we should avoid dynamic memory allocation... and was hoping for suggestions :) The opaque array struct wouldn't be fully backwards compatible because it only works if the caller is linked to the correct version of libsecp. In my limited understanding, I can count three reasonable options if we want to avoid malloc and be binary backwards compatible:
In case 1 and 2 we should document that the struct size may change between versions such that no one relies on this (otherwise it seems like this would become super complex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I right that the user can still create config objects simply on the stack? This should be the case, we shouldn't force the user to have malloc.
But if the answer to this question is yes, then I think we could get rid of the create/destroy functions entirely. Config objects will be small, it's fine and natural to use the stack. And if the user really wants dynamic memory, she can call still call malloc.
Or do we think the size of the config object isn't statically given? This is true for example for the context object. Here, the size depends on the flags. (Although you could argue that there are only four types of contexts with known static sizes...) But here I don't think this will be case for config objects.
Ah sorry, I haven't seen your previous comment @jonasnick. I think what I have in mind is similar to 1. but even simpler. As long as we keep this an opaque object with manipulation functions provided by us, and we give the caller a way to figure out it's size (e.g. for FFI/dynamic alloc), versioning should not be necessary. Am I right? |
@real-or-random the problem is that the "manipulation functions provided by us" need to use malloc. |
I implemented & pushed option 1, the versioned struct, after playing with it here. The only downside is that if we add fields, we have to make sure that the new struct is in fact larger than the old one - on all supported compilers and platforms. We already make an assumption on the biggest alignment, so that should help figuring out when to add dummy fields. I also mentioned above that there would be issues with FFI, but there are actually none. The concern in the issue I linked to was essentially that a struct could change out from under the bindings, but that's exactly what versioned structs prevent. I'll add the missing tests and documentation.
@real-or-random callers can't put an opaque object on the stack because they can't statically determine its size. |
14565fb
to
07138bb
Compare
07138bb
to
dd7157a
Compare
I pushed a new version of this PR with two major changes:
I also added tests and docs. Since this PR doesn't contradict the discussed BIP-340 changes, I removed the WIP status. |
This commit is based on code changes proposed in bitcoin-core/secp256k1#844. Responds to comment: BlockstreamResearch#117 (comment)
This commit is based on code changes proposed in bitcoin-core/secp256k1#844. Responds to comment: BlockstreamResearch#117 (comment)
3be1caa
to
6450368
Compare
Rebased on top of master because cirrus wasn't included in this branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack 6450368
API looks good, code review ACK. Feel like squashing in the fixup? |
6450368
to
b073fb7
Compare
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack 439ed2f
Glad we came to a "resolution" on the forward-compat issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nits, SGTM
utACK 66ff2fa Thanks for taking the nits. |
utACK the fixup commit |
66ff2fa
to
5f6ceaf
Compare
squashed the comment fixup commit |
This makes the default sign function easier to use while allowing more granular control through sign_custom. Tests for sign_custom follow in a later commit.
Gives users the ability to hash messages to 32 byte before they are signed while allowing efficient domain separation through the tag.
Varlen message support for the default sign function comes from recommending tagged_sha256. sign_custom on the other hand gets the ability to directly sign message of any length. This also implies signing and verification support for the empty message (NULL) with msglen 0. Tests for variable lengths follow in a later commit.
This simplifies the interface of sign_custom and allows adding more parameters later in a backward compatible way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack 5f6ceaf
utACK 5f6ceaf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 5f6ceaf
…stream c020cba Squashed 'src/secp256k1/' changes from efad3506a8..be8d9c262f (Pieter Wuille) Pull request description: This updates our src/secp256k1 subtree to the lastest upstream master. Notable changes: * New schnorrsig API (bitcoin-core/secp256k1#844), which adds support for variable-length messages (not used in BIP341/342 transaction signing, so not relevant for us, but it changes the API, and makes some other simplifications). Some of our call sites had to be adapted. * Don't use asm optimizations for `gen_context` (bitcoin-core/secp256k1#965). This fixes #22441. * Various testing/CI improvements ACKs for top commit: hebasto: ACK e4ffb44 jonatack: Light ACK e4ffb44 debug built (debian clang 13.0), ran bitcoind node/tests/git-subtree-check.sh, lightly reviewed the diff and API changes fanquake: ACK e4ffb44 Tree-SHA512: 89a5c3019ec010d578e84bcef756d2c679420c5c768bcdece673405c4e10955179c5a1339aafc68b8b74b1e3912e147bf2f392f44f15af73791d93f6537960b3
c020cba Squashed 'src/secp256k1/' changes from efad350..be8d9c2 (Pieter Wuille) Pull request description: This updates our src/secp256k1 subtree to the lastest upstream master. Notable changes: * New schnorrsig API (bitcoin-core/secp256k1#844), which adds support for variable-length messages (not used in BIP341/342 transaction signing, so not relevant for us, but it changes the API, and makes some other simplifications). Some of our call sites had to be adapted. * Don't use asm optimizations for `gen_context` (bitcoin-core/secp256k1#965). This fixes bitcoin#22441. * Various testing/CI improvements ACKs for top commit: hebasto: ACK e4ffb44 jonatack: Light ACK e4ffb44 debug built (debian clang 13.0), ran bitcoind node/tests/git-subtree-check.sh, lightly reviewed the diff and API changes fanquake: ACK e4ffb44 Tree-SHA512: 89a5c3019ec010d578e84bcef756d2c679420c5c768bcdece673405c4e10955179c5a1339aafc68b8b74b1e3912e147bf2f392f44f15af73791d93f6537960b3
…stream c020cba Squashed 'src/secp256k1/' changes from efad350..be8d9c2 (Pieter Wuille) Pull request description: This updates our src/secp256k1 subtree to the lastest upstream master. Notable changes: * New schnorrsig API (bitcoin-core/secp256k1#844), which adds support for variable-length messages (not used in BIP341/342 transaction signing, so not relevant for us, but it changes the API, and makes some other simplifications). Some of our call sites had to be adapted. * Don't use asm optimizations for `gen_context` (bitcoin-core/secp256k1#965). This fixes bitcoin#22441. * Various testing/CI improvements ACKs for top commit: hebasto: ACK e4ffb44 jonatack: Light ACK e4ffb44 debug built (debian clang 13.0), ran bitcoind node/tests/git-subtree-check.sh, lightly reviewed the diff and API changes fanquake: ACK e4ffb44 Tree-SHA512: 89a5c3019ec010d578e84bcef756d2c679420c5c768bcdece673405c4e10955179c5a1339aafc68b8b74b1e3912e147bf2f392f44f15af73791d93f6537960b3
…stream c020cba Squashed 'src/secp256k1/' changes from efad350..be8d9c2 (Pieter Wuille) Pull request description: This updates our src/secp256k1 subtree to the lastest upstream master. Notable changes: * New schnorrsig API (bitcoin-core/secp256k1#844), which adds support for variable-length messages (not used in BIP341/342 transaction signing, so not relevant for us, but it changes the API, and makes some other simplifications). Some of our call sites had to be adapted. * Don't use asm optimizations for `gen_context` (bitcoin-core/secp256k1#965). This fixes bitcoin#22441. * Various testing/CI improvements ACKs for top commit: hebasto: ACK e4ffb44 jonatack: Light ACK e4ffb44 debug built (debian clang 13.0), ran bitcoind node/tests/git-subtree-check.sh, lightly reviewed the diff and API changes fanquake: ACK e4ffb44 Tree-SHA512: 89a5c3019ec010d578e84bcef756d2c679420c5c768bcdece673405c4e10955179c5a1339aafc68b8b74b1e3912e147bf2f392f44f15af73791d93f6537960b3
…stream c020cba Squashed 'src/secp256k1/' changes from efad350..be8d9c2 (Pieter Wuille) Pull request description: This updates our src/secp256k1 subtree to the lastest upstream master. Notable changes: * New schnorrsig API (bitcoin-core/secp256k1#844), which adds support for variable-length messages (not used in BIP341/342 transaction signing, so not relevant for us, but it changes the API, and makes some other simplifications). Some of our call sites had to be adapted. * Don't use asm optimizations for `gen_context` (bitcoin-core/secp256k1#965). This fixes bitcoin#22441. * Various testing/CI improvements ACKs for top commit: hebasto: ACK e4ffb44 jonatack: Light ACK e4ffb44 debug built (debian clang 13.0), ran bitcoind node/tests/git-subtree-check.sh, lightly reviewed the diff and API changes fanquake: ACK e4ffb44 Tree-SHA512: 89a5c3019ec010d578e84bcef756d2c679420c5c768bcdece673405c4e10955179c5a1339aafc68b8b74b1e3912e147bf2f392f44f15af73791d93f6537960b3
…stream c020cba Squashed 'src/secp256k1/' changes from efad350..be8d9c2 (Pieter Wuille) Pull request description: This updates our src/secp256k1 subtree to the lastest upstream master. Notable changes: * New schnorrsig API (bitcoin-core/secp256k1#844), which adds support for variable-length messages (not used in BIP341/342 transaction signing, so not relevant for us, but it changes the API, and makes some other simplifications). Some of our call sites had to be adapted. * Don't use asm optimizations for `gen_context` (bitcoin-core/secp256k1#965). This fixes bitcoin#22441. * Various testing/CI improvements ACKs for top commit: hebasto: ACK e4ffb44 jonatack: Light ACK e4ffb44 debug built (debian clang 13.0), ran bitcoind node/tests/git-subtree-check.sh, lightly reviewed the diff and API changes fanquake: ACK e4ffb44 Tree-SHA512: 89a5c3019ec010d578e84bcef756d2c679420c5c768bcdece673405c4e10955179c5a1339aafc68b8b74b1e3912e147bf2f392f44f15af73791d93f6537960b3
Summary: Bitcoin Core changed the C API for BIP340 Schnorr signatures within secp256k1. We don't use them in Bitcoin ABC, so thus far this backport was not needed. This is a backport of [[bitcoin-core/secp256k1#844 | secp256k1#844]]. Now we do need this backport mostly for `secp256k1_schnorrsig_sign_custom` for porting `rust-secp256k1` to the secp256k1 library of this repo. Depends on D16957. Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D16958
Summary: Bitcoin Core changed the C API for BIP340 Schnorr signatures within secp256k1. We don't use them in Bitcoin ABC, so thus far this backport was not needed. This is a backport of [[bitcoin-core/secp256k1#844 | secp256k1#844]]. Now we do need this backport mostly for `secp256k1_schnorrsig_sign_custom` for porting `rust-secp256k1` to the secp256k1 library of this repo. Depends on D16957. Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D16958
Summary: Bitcoin Core changed the C API for BIP340 Schnorr signatures within secp256k1. We don't use them in Bitcoin ABC, so thus far this backport was not needed. This is a backport of [[bitcoin-core/secp256k1#844 | secp256k1#844]]. Now we do need this backport mostly for `secp256k1_schnorrsig_sign_custom` for porting `rust-secp256k1` to the secp256k1 library of this repo. Depends on D16957. Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D16958
This is a work in progress because I wanted to put this up for discussion before writing tests. It addresses the TODOs that didn't make it in the schnorrsig PR and changes the APIs of
schnorrsig_sign
,schnorrsig_verify
andhardened_nonce_function
.aux_rand32
argument forsign
would be const, but didn't find a solution I was happy with.(EDIT: see below)sign_custom
with its opaque config object allows adding more arguments later without having to change the API again. Perhaps there are other sensible customization options, but I'm thinking of sign-to-contract/covert-channel in particular. It would require adding the fieldsunsigned char *s2c_data32
andsecp256k1_s2c_opening *s2c_opening
to the config struct. The former is the data to commit to and the latter is written to bysign_custom
.