-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add frost-secp256k1-tr crate (BIP340/BIP341) #584
base: main
Are you sure you want to change the base?
Conversation
zebra-lucky
commented
Nov 20, 2023
- add frost-secp256k1-tr crate with support of BIP0340, BIP0341
Thank you, this is great. I'll take a closer look soon and may make some adjustments. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #584 +/- ##
==========================================
- Coverage 82.18% 81.07% -1.11%
==========================================
Files 31 34 +3
Lines 3188 3694 +506
==========================================
+ Hits 2620 2995 +375
- Misses 568 699 +131 ☔ View full report in Codecov by Sentry. |
After a closer look, here are some things that we'd prefer to change before merging this PR:
I realize these might be big changes. Let me know if this makes sense and if you are able to work on this. Otherwise we can eventually incorporate these changes to the PR, but might not be able to get that done shortly. Also - what did you use to generate the test vectors? Do you have a separate reference implementation for this? I'd like to take a look if possible. |
I'll try to add changes to generation process to use only even VerifyingKey On the "emtpy merkle root" -- need to do more investigations, but current code is working. As a proof I'll add a link on the repo with testing code to sign tx (in the next post).
On testing vectors -- I just added the values from the working code so the tests wouldn't fail. |
Some proofs that the current code works Testing code on the Testnet To use there is a need to fix paths in the
Where last part is a hex of transaction which makes output on generated address. Some dirty but working code. Latest transactions on the Testnet: |
Just FYI we're testing this library on Bitcoin and getting flaky results (sometimes signature fails). I'm investigating on our side to understand if it's us or if there's some issue here (my current intuition is that the issue is here).
In addition, I had another issue which led me to spend quite some time reviewing this PR, so here are some notes and some reverse-engineering notes (for others who are trying to understand this like me):
Nevermind, I wrote my understanding here: https://hackmd.io/li6ByhmfQUucZOqdboF1-g?view I'm still unsure as to why we're seeing issues on our side atm. |
I fixed all the comments I pointed out in zebra-lucky#1 and everything now works on our end! (bonus, the art that helped me find the issues) |
Thanks!, I'll read it to realize. |
1f4b852
to
6fdd873
Compare
I am looking at the changes proposed in zebra-lucky#1 |
Merged fix on tweaked public key processing from @mimoo. I'll add DKG/trusted dealer code which generates only |
Some problems on |
actually that might be overkill since all the checks must happen anyway no? |
yep... thinking on that... currently stuck on batch test... signing with |
0e5e083
to
9ca5ad9
Compare
Added:
There is a question about only even Note about merkle root (as in BIP341 code example):
For key path spending |
p.s.: rebased PR branch on current main branch |
frost-secp256k1-tr/src/lib.rs
Outdated
/// Generates the challenge as is required for Schnorr signatures. | ||
fn challenge(R: &Element<S>, verifying_key: &VerifyingKey, msg: &[u8]) -> Challenge<S> { | ||
let mut preimage = vec![]; | ||
let tweaked_public_key = tweaked_public_key(&verifying_key.to_element(), &[]); |
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.
Should the merkel root be empty here?
i.e does this code assume that we only spend from the default key path? Not a tapscript path?
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.
Possibly my understanding is wrong, but in BIP341, in
code example for taproot_sign_key
I've seen next:
if script_tree is None:
h = bytes() # empty bytes for merkle root
In the case when script path is not used (used only key path),
seems empty bytes for merkle root is correct.
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.
yeah this code only works with empty commitment, and it works fine for us (https://github.com/sigma0-xyz/zkbitcoin). I think it's a good first step, but it'd be nice to expose that feature later
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.
Thank you. I'll look to code in zkbitcoin in a few days.
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.
In the case when script path is not used (used only key path),
Ah yes you're correct. In the case of a script path spend (non-frost spend) the spender can tweak the verifying key outside the context of this library.
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.
Thank you. I'll look to code in zkbitcoin in a few days.
Only looking in the current day...
Hey all! I hope I it's okay to post this here, but let me know if I should ask this somewhere else, or make an issue. My question about signing Schnorr that I think might be related to this PR. I'm encountering an issue when broadcasting a PSBT transaction signed using FROST for aggregated Schnorr signatures. While individual Schnorr signatures work correctly, the aggregated signature fails with the following error:
We generate a hash from the PSBT's unsigned transaction and sign it using a keypair. This process completes without errors. let hash = SighashCache::new(&psbt.unsigned_tx.clone())
.taproot_script_spend_signature_hash(
0,
&Prevouts::All(&[tx_out.clone()]),
ScriptPath::with_defaults(&multisig_script),
SchnorrSighashType::Default,
)
.unwrap_throw(); let sig = secp.sign_schnorr(
&Message::from_slice(&hash).unwrap_throw(),
&keypair,
); When using FROST to sign the transaction, we provide the same hash as the message parameter for the SigningPackage. let signing_package =
frost::SigningPackage::new(signing_state.signing_commitments.clone(), hash); The process creates an aggregated signature, but broadcasting the transaction results in the above error. Given the transition from successful individual signing to the failure of the FROST aggregated signature, is there a specific aspect of message handling or signature aggregation with FROST that needs to be adjusted for these transactions? Any insights or suggestions on resolving the "Invalid Schnorr signature" error would be greatly appreciated. Also I'm wondering if I need the _tr library for the signing process I'm doing, or if it would be enough to just use the frost_secp256k1 library? Thanks! |
Hello! Need to clarify details. On the first commits to the this PR there was Do you use the latest commit of frost-secp256k1-tr b360496? There is some testing code on the testnet to check how it's works: |
frost-secp256k1 is a general on a curve, frost-secp256k1-tr adds |
@zebra-lucky I changed the verify function to accept the
And this works, though it breaks passing a message directly into the verify function, so I'm not sure how you'd want to handle that. |
@MatthewLM Good catch. Looks like we don't have any unit test coverage for tweaked signatures yet. I'll fix that alongside the bug you found there. (my code is what's buggy, so i ought to fix it 😅) |
Integration tests and a fix for the bug pointed out by @MatthewLM have been submitted to this PR in zebra-lucky#4 |
Thank you, @conduition for your help. I'm sorry for a pause, but i'm ill currently. |
@conduition @zebra-lucky I've not experienced any further issues using 20c2c98, thank you for the fix. I've been able to create successful Taproot key-path and script-path spends. I look forward to seeing this finished and merged in. Given Taproot support, I'm curious about deriving unhardened BIP32 keys. Clearly hardened derivation is not possible, except perhaps by modifying the scheme somehow. A straight-forward method for unhardened derivation might be to first derive a master chaincode from the participant's public key shares. Then the same tweaks can be applied to the group key, public share and private shares. I wonder if there are any security implications to deriving BIP32 keys this way though. I don't see how there would be but I know there might be some nuance that I'm unaware of. |
@MatthewLM Intriguing idea. I hadn't considered BIP32 support. Yes, hardened derivation wouldn't be possible without some special DKG-like ritual, which would probably be more trouble than it's worth (At that point you could just run the DKG again to make a fresh FROST key).
This approach should be safe. The chain code is public data (public in the sense of "it reveals nothing that would help an observer forge signatures"), so deriving it from the FROST group's public verification shares should be acceptable. Group members would not be concerned that the chaincode was chosen unfairly, because each member is presumed to have participated in the DKG process which produced the verification shares (and thus the chain code). There are down-sides to that approach though. If the full set of verification shares must be exposed for any reason (e.g to prove each signer's membership in the group), this would also result in exposing the full extended public key for the group. Also, this approach would not be practically compatible with Verifiable Secret Resharing (VSR) or a Repairable Threshold Scheme, because those techniques involve adding or otherwise changing the set of shares. Doing so would also change the chain code derived from those shares, and thus the group extended pubkey would need to change. I imagine many use cases would prefer that not to happen. I have a few alternative suggestions for how to combine FROST and BIP32, but i think for the sake of scope-reduction we should keep this PR limited to a single static group key. Additive tweaks on signing shares are easy for downstream callers to add themselves, by using the |
@conduition Thanks for your response. I agree with what you say. The risk of leaking all of the public shares could perhaps be mitigated if membership can be proved with zero-knowledge. Deriving the chaincode from the public shares would be trivial but I agree in some (perhaps many) cases it would not be desirable. A chain-code could be separately generated between all participants through an extension of the DKG. I agree that it complicates this PR, so maybe I could create a discussion on this repo for BIP32 considerations? |
I think that would be a swell idea 😄 |
@conduition I started a discussion here: #636 |
@zebra-lucky I realized we were still serializing taproot signatures as 65-byte arrays with compressed R points. To be compatible with BIP340, callers would need to serialize and manually trim that leading byte. For usability, i have submitted a PR which fixes this, ensuring signatures are always computed with even R points, and serialized as BIP340-compatible 64-byte arrays with x-only R points. zebra-lucky#5 |
I'm sorry for a delayed reaction. |
BIP340 signatures are usually represented with x-only (even parity) nonce points. As a step towards normalizing this for the frost-secp256k1-tr crate, we should ensure all Signature struct instances always use the effective nonce point, including the DKG proof-of-knowledge.
BIP340 signatures are supposed to be serialized as a 64-byte array: 32 bytes for the x-only nonce point 'R', and 32 bytes for the signature component 's'. This commit customizes the frost-secp256k1-tr crate so that signatures are serialized with x-only nonces, omitting the leading parity byte.
Not delayed at all, man. Thanks very much for your quick attention 😄 |
Hi @zebra-lucky and @conduition, I'm just following up on this thread as it's been quiet for a while. I see This library released a 2.0.0-rc recently, and I wonder if this feature might be part of the full 2.0.0 release. We're not really blocked as we're working off the branch, but it would be great to get merged into the upstream code. Thanks for all the hard work on this one! |
|
@sosaucily I would love to get this into the 2.0.0 release. Unfortunately i'm not sure when this PR will be merged - I imagine the maintainers of the i also have an additional feature branch, |
Hi! At the first I very thankful to the people who gave a ideas on the PR:
need to think how to transfer PR code, to not do a new PR |
my productivity currently is at very low, I'm sorry |
Thanks everyone, I really appreciate the work that went on this. I want to get another look into this and I might do some refactorings to avoid changing the frost_core code so much. I think it's unlikely that this will make into the 2.0.0 release, but we'll probably do a 2.1.0 release not much time after that and this might be included then. Don't worry about the conflicts, I'll solve them. |
We are hosting our send FROST library implementors workshop on September 18th https://developer.blockchaincommons.com/frost/meeting2/ So far scheduled are some discussion from bitcoin devs on their FROST implementations in C++. One of the critical issues to me are the issues of x-only pubkey compatibility and the tweaked signatures for use on-chain. All of Blockchain Common's own code base has been moving from C++ to Rust and we'd prefer a rust implementation. Could we get someone from this team to participate, or even present a brief status update / roadmap on this PR? Contact me at ChristopherA@LifeWithAlacrity.com if you are interested. Thanks! |
@ChristopherA Thanks for organizing that 🙂 I've just sent you an email @zebra-lucky It looks like it's not possible to transfer a pull request on github unfortunately, we'd have to close this PR and open a new one if we wanted to change the PR owner. Would you prefer I open a new PR from my fork of |
Hi! Make it in your preferred way. |