-
Notifications
You must be signed in to change notification settings - Fork 76
ed25519: add instruction builder for multiple verifications #13
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
Conversation
aed06c3
to
90372ba
Compare
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.
Looks good to me overall! Let me know what you think about the two comments below.
ed25519-program/src/lib.rs
Outdated
/// Convenience function to convert signature offsets into a single ed25519 instruction | ||
/// The caller can choose to extend the data buffer and write the verification data at | ||
/// `DATA_START` or store the verification data in a different instruction. |
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.
Is DATA_START
index correct when there are multiple offsets? Maybe we can omit mention of it altogether? For example:
/// Convenience function to convert signature offsets into a single ed25519 instruction | |
/// The caller can choose to extend the data buffer and write the verification data at | |
/// `DATA_START` or store the verification data in a different instruction. | |
/// Encode just the signature offsets in a single ed25519 instruction. | |
/// | |
/// This is a convenience function for cases when verification data is to be read from a different instruction | |
/// or acount. The caller can optionally choose to extend the instruction buffer with the verification data to | |
/// form a properly verifiable instruction. |
I also reworded some bits to avoid confusion on how to use the ed25519 program. Let me know what you think.
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 good call, DATA_START
is incorrect. questions about your comment:
verification data is to be read from a different instruction or acount
If understand correctly we can't read the verification data from an account right? Has to be in an instruction? Also couldn't someone use this helper to put the verification data in the same instruction, does it have to be in a different instruction?
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.
If understand correctly we can't read the verification data from an account right? Has to be in an instruction?
Oh yeah I was thinking about client side applications where a user would generate an instruction not for the purpose of submitting to the ed25519 program, but perhaps for different purposes. But I guess such applications are quite rare. Let's remove mention about accounts. Good call!
Also couldn't someone use this helper to put the verification data in the same instruction, does it have to be in a different instruction?
That is right. Re-reading what I wrote, it is actually a bit confusing 😅 . I did mention it above, but just in the last sentence because I thought if we were to sign a single message, then we would generally use the new_ed25519_instruction
function and signing multiple messages in a single instruction is relatively rare (as mentioned in the comment below).
Please feel free to completely reword what I wrote.
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.
Updated the comment and included your suggestion to use new_ed25519_instruction
if the signer is the same
ed25519-program/src/lib.rs
Outdated
/// Creates an ed25519 instruction for verifying multiple messages signed by `keypair` | ||
/// The verification is stored in the instruction data, so it is up to the caller to check | ||
/// that the messages will fit within the maximum transaction size. | ||
pub fn new_multi_ed25519_instruction( |
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.
Technically speaking, if there are multiple messages to sign with a single keypair, then we would concatenate all the messages into a single buffer and then sign the entire buffer at once. This would make the signing and verification of the messages to be cheaper and also make the instruction data more compact. Given this, I am afraid that this function may encourage a wrong convention in the way people use the ed25519 program.
This function might make more sense if we take in &[&Keypair]
as input, but I think it is also rare for a single user/application needing to sign with multiple keypairs. I am happy either way, but what do you think about just restricting to offsets_to_ed25519_instruction
function in this PR?
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.
yep that makes sense, will remove this.
90372ba
to
4411fc0
Compare
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.
Looks good to me! Thanks!
follow up from solana-program/slashing#5 (comment)
we add an instruction builder for doing multiple signature verifications in one verify instruction and expose a helper function.