BIP-327: correct DeterministicSign pubnonce and key length#2066
BIP-327: correct DeterministicSign pubnonce and key length#2066lisenokdonbassenok wants to merge 2 commits intobitcoin:masterfrom
Conversation
murchandamus
added
Pending acceptance
murchandamus
left a comment
murchandamus
left a comment
There was a problem hiding this comment.
@jonasnick, @real-or-random, could either of you take a look at this?
| ** The secret signing key ''sk'': a 32-byte array | ||
| ** The aggregate public nonce ''aggothernonce'' (see [[#modifications-to-nonce-generation|above]]): a 66-byte array | ||
| ** The number ''u'' of individual public keys with ''0 < u < 2^32'' | ||
| ** The individual public keys ''pk<sub>1..u</sub>'': ''u'' 32-byte arrays |
There was a problem hiding this comment.
Given that the line right above states that keys are between 0 < u < 2^32, it seems to me that we are looking at an x-only key and the text is already correct, but maybe I’m misinterpreting that.
There was a problem hiding this comment.
maybe I’m misinterpreting that.
You are. u is the number of public keys involved in signing and not related to their length.
| * Fail if ''k<sub>1</sub> = 0'' or ''k<sub>2</sub> = 0'' | ||
| * Let ''R<sub>⁎,1</sub> = k<sub>1</sub>⋅G, R<sub>⁎,2</sub> = k<sub>2</sub>⋅G'' | ||
| * Let ''pubnonce = cbytes(R<sub>⁎,2</sub>) || cbytes(R<sub>⁎,2</sub>)'' | ||
| * Let ''pubnonce = cbytes(R<sub>⁎,1</sub>) || cbytes(R<sub>⁎,2</sub>)'' |
There was a problem hiding this comment.
This one seems right to me.
|
Thanks, not sure how we ended up with DeterministicSign so different. All of the changes proposed here are correct. But I don't think they're sufficient. DeterministicSign also doesn't take a keyagg context and doesn't support tweaking (i.e., negation of the sk). Perhaps it's better to fix all of this in a single PR and also add a changelog entry. @lisenokdonbassenok Would you be willing to do these changes too? |
So I need only to add changelog? |
I'm a bit confused here. From what I understand, DeterministicSign takes the list of public keys and tweaks, then internally computes the keyagg context and applies the tweaks. It calls Sign internally, which handles the secret key negation. What needs to fixed here? |
@real-or-random, looking at the PR Author’s GitHub profile and their response to you, I suspect that this is an LLM-assisted pull request by a PR farmer. If so, they likely don’t actually understand what you are asking for and will beat around the bush for a few weeks until someone else provides a suggested code change to incorporate. Please feel free to save us all some time and make your own PR that incorporates these changes and makes the additional fixes you propose. |
You're right! No additional changes need to be done.
See #2071. |
4 participants
The DeterministicSign specification currently describes pk_1..u as u 32-byte arrays and sets pubnonce = cbytes(R*_2) || cbytes(R*_2). Both statements conflict with the rest of BIP-0327, the Python reference implementation and the published test vectors. Individual public keys are plain compressed points of length 33 bytes everywhere else in the BIP, and the reference code derives pubnonce as cbytes(R*_1) || cbytes(R*_2), which is the format expected by NonceAgg and PartialSigVerify. This change updates the DeterministicSign section to use 33-byte plain public keys and to define pubnonce as (R*_1, R*_2), aligning the written specification with the existing reference implementation and test vectors without changing any executable code.