BIP458: Half-Aggregation of BIP 340 Signatures#2205
Conversation
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
Looks quite complete. Might be good to add a backward compatibility section (if not needed, that could be explicitly stated).
Code is based on the secp256k1-zkp implementation. Test vectors based on the draft BIP: bitcoin/bips#2205 Co-authored-by: Benedikt <crypto@benedikt-wagner.dev>
Code is based on the secp256k1-zkp implementation. Test vectors based on the draft BIP: bitcoin/bips#2205 Co-authored-by: Benedikt <crypto@benedikt-wagner.dev>
Code is based on the secp256k1-zkp implementation. Test vectors based on the draft BIP: bitcoin/bips#2205 Co-authored-by: Benedikt <crypto@benedikt-wagner.dev>
|
Addressed your comments @jonatack , thanks!
I added a "section not needed" section ;) |
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
Nice! Left some notation and style nits below (mostly related to secp256k1lab usage).
|
|
||
| ==== Notation ==== | ||
|
|
||
| The following conventions are used, with constants as defined for [https://www.secg.org/sec2-v2.pdf secp256k1]. We note that adapting this specification to other elliptic curves is not straightforward and can result in an insecure scheme<ref>Among other pitfalls, using the specification with a curve whose order is not close to the size of the range of the nonce derivation function is insecure.</ref>. |
There was a problem hiding this comment.
Fwiw, in BIP-445 a different notation is used (for the first time, as far as I'm aware) that leans more to secp256k1lab, with a table showing how the notation maps to the lab functions: https://github.com/siv2r/bips/blob/bip-frost-signing/bip-0445.md?plain=1#L255
(I found that quite a nice idea for improved readability and hopefully easier navigation through the reference implementation; but that's of course more a nit than a requirement; ultimately it's up to the BIP authors).
There was a problem hiding this comment.
Thanks, I didn't quite pick up on this despite looking at the BIP445 draft for some inspiration. But I am not sure it's the best idea to adapt this notation fully here. I think there is also some value in having the notation closer to BIP340 which is a prerequisite to this BIP here while BIP445 is not directly related. I did steal the idea of the mapping table though, please let me know if you agree with this decision and think the mapping table on it's own is helpful.
There was a problem hiding this comment.
Yeah good point, keeping the notation intentionally close to BIP340 seems reasonable. I think the small mapping table you added is definitely helpful for (reference implementation) readers.
|
Addressed feedback from @theStack , thanks a lot! |
murchandamus
left a comment
murchandamus
left a comment
There was a problem hiding this comment.
Thanks, that was a splendid read. I especially enjoyed the ventilated prose. Looks like you have all the sections covered, already have a reference implementation, and the text is very polished already. I don’t have any improvement suggestions at this time, so let’s just go right ahead: assigned BIP454. Please move the document and auxiliary files to the corresponding file paths, update the BIP and Assigned headers, and add a table entry to the README.mediawiki.
murchandamus
changed the title
Nice, 99% of that praise goes to @jonasnick and @real-or-random !
Thanks! I applied the necessary changes. |
murchandamus
left a comment
murchandamus
left a comment
There was a problem hiding this comment.
Changes LGTM. Since this just got opened, I suggest we let it simmer for at least a couple weeks to see if there is any more useful review, but otherwise, this seems pretty much ready for publication to me. What do you think?
Yeah, that sounds good for me. I will publish the draft for fullagg shortly and maybe there will be some additional feedback here from having the two side by side. |
murchandamus
changed the title
|
Assigned new number per request of @fjahr. Let’s refer to this as 458 instead of 454. Please update the table and preamble accordingly. |
Add a vendored copy of the secp256k1lab library (master branch, commit a265da139aea27386085a2a8760f8698e1bda64e) that the half-aggregation reference implementation and test vector scripts depend on.
Done! Thanks and sorry for the inconvenience. |
4 participants
This is the BIP draft for half-aggregation of BIP340 Schnorr signatures, which has marinated at https://github.com/BlockstreamResearch/cross-input-aggregation/blob/master/half-aggregation.mediawiki long enough.
Mailing list discussion: https://gnusha.org/pi/bitcoindev/33f275c2-06b1-4b4a-2a75-cafe36836503@gmail.com/
Compared to the master version of the BIP linked above and the state of things at the time of sharing this on the mailing list, this version has a few minor typo fixes as well as three significant changes:
Given the Python reference implementation, I am vendoring the latest master of secp256k1lab with the BIP, as seems to be the standard with other BIPs (seen merged at 374 and in the pull for 445).