Skip to content
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 duplicate block proof sigverify #5

Merged
merged 11 commits into from
Jan 29, 2025

Conversation

AshwinSekar
Copy link
Contributor

@AshwinSekar AshwinSekar commented Jan 9, 2025

Adds signature verification of the shreds included in a duplicate block proof to the slashing program.
Due to the limitations of zerocopying from the instructions sysvar the requirements are as follows:

  • The signature verification instruction to the Ed25519 program must be in the index immediately before the Slashing instruction
  • This instruction must specify 2 verifications, the first one for shred1 and the second for shred2 as indicated by the ordering in the proof_account specified to the slashing instruciton
  • The (pubkey, message, signature) data indicated by the Ed25519 ix's offsets must be in the instruction data for the slashing instruction.

If each (pubkey, message, signature) matches the (node_pubkey, shredx.merkle_root, shredx.signature) for both shreds, then the signature verification is considered successful.

Additionally we expose a utility for the user to create both the sigverify instruction and the slashing instruction together.

Will update the SIMD if this approach is satisfactory

@AshwinSekar AshwinSekar force-pushed the sigverify branch 10 times, most recently from b28a6da to 37888b9 Compare January 9, 2025 20:35
program/Cargo.toml Show resolved Hide resolved
program/src/instruction.rs Outdated Show resolved Hide resolved
program/src/sigverify.rs Show resolved Hide resolved
@AshwinSekar AshwinSekar requested a review from joncinque January 9, 2025 20:56
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great work! The design makes sense of including the sigverify offsets directly in the slashing instruction, and you've matched everything up without duplication. My comments are mostly minor

program/Cargo.toml Show resolved Hide resolved
scripts/solana.dic Show resolved Hide resolved
program/src/instruction.rs Outdated Show resolved Hide resolved
program/src/instruction.rs Outdated Show resolved Hide resolved
program/src/instruction.rs Outdated Show resolved Hide resolved
program/src/duplicate_block_proof.rs Outdated Show resolved Hide resolved
program/src/duplicate_block_proof.rs Outdated Show resolved Hide resolved
program/src/duplicate_block_proof.rs Outdated Show resolved Hide resolved
program/src/duplicate_block_proof.rs Outdated Show resolved Hide resolved
Comment on lines 257 to 260
assert!(
ErasureMeta::check_erasure_consistency(&shred1, &shred2),
"Expected erasure consistency failure",
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to remove this check?

Copy link
Contributor Author

@AshwinSekar AshwinSekar Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's not completely accurate to say it's an erasure consistency failure here, the real failure is the merkle root of shred1 and shred2 will not be consistent. i've updated the condition here.

@AshwinSekar AshwinSekar requested a review from joncinque January 24, 2025 17:28
*verification =
MaybeUninit::new(SignatureVerification::new(pubkey, message, signature)?);
}
unsafe { std::mem::transmute_copy(&verifications) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than copying, let's just transmute directly, like in the array exmaple: https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#initializing-an-array-element-by-element

We can also add a comment to use array_assume_init once it stabilizes https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#method.array_assume_init

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I was running into problems with transmute which is why I opted for the copy. rust-lang/rust#61956
I think I could manually assume_init on each element instead, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I'd prefer to use the approach recommended directly in the docs, ie unsafe { mem::transmute::<_, [SignatureVerification; NUM_VERIFICATIONS]>(verifications) }

https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#initializing-an-array-element-by-element

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem is the const generic NUM_VERIFICATIONS see rust-lang/rust#61956. It was fixed for [T; N] but does not work for [MaybeUninit<T>; N] -> [T; N] rust-lang/rust#61956 (comment)
So for example this:

Ok(unsafe { std::mem::transmute::<_, [SignatureVerification; NUM_VERIFICATIONS]>(verifications) })

results in

error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
   --> program/src/sigverify.rs:187:21
    |
187 |         Ok(unsafe { std::mem::transmute::<_, [SignatureVerification; NUM_VERIFICATIONS]>(verifications) })
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: source type: `[std::mem::MaybeUninit<sigverify::SignatureVerification<'_>>; NUM_VERIFICATIONS]` (this type does not have a fixed size)
    = note: target type: `[sigverify::SignatureVerification<'_>; NUM_VERIFICATIONS]` (this type does not have a fixed size)

However if I remove the NUM_VERIFICATIONS and just use 2 it compiles

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhhh gotcha, sorry about that, I didn't realize it didn't work with const generics. In that case, it's not even clear to me that array_assume_init will work.

Either way, let's go with your solution, thanks for the explanation

program/src/sigverify.rs Outdated Show resolved Hide resolved
program/src/duplicate_block_proof.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of bits on the use of MaybeUninit, then this should be good to go!

@AshwinSekar AshwinSekar merged commit 9d8118c into solana-program:main Jan 29, 2025
7 checks passed
@AshwinSekar AshwinSekar deleted the sigverify branch January 29, 2025 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants