add duplicate block proof sigverify #5

AshwinSekar · 2025-01-09T17:33:02Z

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

program/Cargo.toml

AshwinSekar · 2025-01-09T20:29:25Z

program/src/instruction.rs

@@ -79,10 +115,11 @@ pub(crate) fn decode_instruction_type(input: &[u8]) -> Result<SlashingInstructio

 /// Utility function for decoding instruction data
 pub(crate) fn decode_instruction_data<T: Pod>(input_with_type: &[u8]) -> Result<&T, ProgramError> {
-    if input_with_type.len() != pod_get_packed_len::<T>().saturating_add(1) {
+    let data_len = pod_get_packed_len::<T>().saturating_add(1);
+    if input_with_type.len() < data_len {


Changed this to < to allow storage of the sigverify data in slashing instruction.

If DuplicateBlockProofSigverifyData is directly included in DuplicateBlockProofInstructionData, then this shouldn't be an issue!

program/src/sigverify.rs

joncinque

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

scripts/solana.dic

joncinque · 2025-01-15T22:28:12Z

program/src/instruction.rs

+    slot: Slot,
+    node_pubkey: Pubkey,
+    sigverify_data: &DuplicateBlockProofSigverifyData,
+    slashing_instruction_index: u16,


nit: I'm thinking this should be left out for now for easier usage, but the doc comment can certainly call out that these must be the first two instructions in a transaction. What do you think?

ya good suggestion, updated here 074e0b3

joncinque · 2025-01-16T13:34:10Z

program/src/instruction.rs

+/// Utility struct for packaging the signature verification data required
+/// for `SlashingInstruction::DuplicateBlockProof`
+#[repr(C)]
+#[derive(Clone, Copy, Debug, PartialEq, Pod, Zeroable)]
+pub struct DuplicateBlockProofSigverifyData {
+    /// The first shred's merkle root (the message of the first sigverify
+    /// instruction)
+    pub shred_1_merkle_root: Hash,
+    /// The first shred's signature (the signature of the first sigverify
+    /// instruction)
+    pub shred_1_signature: [u8; SIGNATURE_BYTES],
+    /// The second shred's merkle root (the message of the second sigverify
+    /// instruction)
+    pub shred_2_merkle_root: Hash,
+    /// The second shred's signature (the signature of the second sigverify
+    /// instruction)
+    pub shred_2_signature: [u8; SIGNATURE_BYTES],
+}


Is there a reason for not including this in DuplicateBlockProofInstructionData? It would be clearer to show that this needs to be part of the instruction data. Or at the very least, add a comment in DuplicateBlockProofInstructionData saying that it will be followed by this type

duh that's so much easier haha, updated here 884d9ee

joncinque · 2025-01-16T13:35:08Z

program/src/instruction.rs

@@ -79,10 +115,11 @@ pub(crate) fn decode_instruction_type(input: &[u8]) -> Result<SlashingInstructio

 /// Utility function for decoding instruction data
 pub(crate) fn decode_instruction_data<T: Pod>(input_with_type: &[u8]) -> Result<&T, ProgramError> {
-    if input_with_type.len() != pod_get_packed_len::<T>().saturating_add(1) {
+    let data_len = pod_get_packed_len::<T>().saturating_add(1);
+    if input_with_type.len() < data_len {


If DuplicateBlockProofSigverifyData is directly included in DuplicateBlockProofInstructionData, then this shouldn't be an issue!

joncinque · 2025-01-16T14:08:43Z

program/src/duplicate_block_proof.rs

+    pub(crate) expected_shred1_merkle_root: &'a Hash,
+    pub(crate) expected_shred2_merkle_root: &'a Hash,
+    pub(crate) expected_shred1_signature: &'a [u8; SIGNATURE_BYTES],
+    pub(crate) expected_shred2_signature: &'a [u8; SIGNATURE_BYTES],


tiny nit: can this be laid out in the same order as the instruction? That way you could eventually just cast the instruction data to this type and avoid doing any work to create it, ie

Suggested change

pub(crate) expected_shred1_merkle_root: &'a Hash,

pub(crate) expected_shred2_merkle_root: &'a Hash,

pub(crate) expected_shred1_signature: &'a [u8; SIGNATURE_BYTES],

pub(crate) expected_shred2_signature: &'a [u8; SIGNATURE_BYTES],

pub(crate) expected_shred1_merkle_root: &'a Hash,

pub(crate) expected_shred1_signature: &'a [u8; SIGNATURE_BYTES],

pub(crate) expected_shred2_merkle_root: &'a Hash,

pub(crate) expected_shred2_signature: &'a [u8; SIGNATURE_BYTES],

yep done here b5b5b0f

just cast the instruction data to this type and avoid doing any work to create it

For this are you suggesting that we don't actually copy the data over when doing the instruction introspection but instead just cast the instruction data?

joncinque · 2025-01-16T14:16:03Z

program/src/duplicate_block_proof.rs

@@ -243,17 +320,82 @@ fn check_shreds(slot: Slot, shred1: &Shred, shred2: &Shred) -> Result<(), Slashi
    Err(SlashingError::InvalidErasureMetaConflict)
 }

+/// Verify that `shred1` and `shred2` are correctly signed by `node_pubkey`.
+/// Leader's sign the merkle root of each shred with their pubkey.


micro nit typo

Suggested change

/// Leader's sign the merkle root of each shred with their pubkey.

/// Leaders sign the merkle root of each shred with their pubkey.

joncinque · 2025-01-16T14:25:10Z

program/src/duplicate_block_proof.rs

+        // Hack to simulate the merkle roots being stored in instruction data
+        let expected_shred1_merkle_root =
+            unsafe { &*Box::into_raw(Box::new(shred1.merkle_root().unwrap_or_default())) };
+        let expected_shred2_merkle_root =
+            unsafe { &*Box::into_raw(Box::new(shred2.merkle_root().unwrap_or_default())) };


I know it's only testing, but this is a bit brittle, no? If the underlying Hash ever gets cleaned up we'll get hard-to-reproduce memory issues during testing.

I appreciate it's more annoying to use, but do you mind passing in the merkle root refs as extra parameters to the function? That way we can avoid doing this.

yep good call I was just lazy haha ff6e4de

joncinque · 2025-01-16T14:39:00Z

program/src/duplicate_block_proof.rs

+        assert!(proof_data
+            .verify_proof(context, SLOT, &leader_pubkey)
+            .is_ok());


nit: I typically prefer doing an unwrap() to get a better error message if something goes wrong, ie

Suggested change

assert!(proof_data

.verify_proof(context, SLOT, &leader_pubkey)

.is_ok());

proof_data

.verify_proof(context, SLOT, &leader_pubkey)

.unwrap();

joncinque · 2025-01-16T14:40:16Z

program/tests/duplicate_block_proof.rs

-    assert!(
-        ErasureMeta::check_erasure_consistency(&shred1, &shred2),
-        "Expected erasure consistency failure",
-    );


Did you mean to remove this check?

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 force-pushed the sigverify branch 10 times, most recently from b28a6da to 37888b9 Compare January 9, 2025 20:35

AshwinSekar commented Jan 9, 2025

View reviewed changes

AshwinSekar requested a review from joncinque January 9, 2025 20:56

add duplicate block proof sigverify

50f1118

AshwinSekar force-pushed the sigverify branch from 37888b9 to 50f1118 Compare January 10, 2025 20:51

joncinque reviewed Jan 16, 2025

View reviewed changes

AshwinSekar added 9 commits January 23, 2025 18:36

pr feedback: unify sigverify data and instruction data

884d9ee

pr feedback: remove slashing instruction index

074e0b3

pr feedback: store num signatures as u16

9472a75

pr feedback: use maybe uninit instead of array vec

c87e985

pr feedback: compare num_signatures as a u16

47e5ee2

pr feedback: ordering nits and spelling

b5b5b0f

pr feedback: remove unsafe testing code, pass merkle roots in

ff6e4de

remove arrayvec

350dc2b

pr feedback: update test check

12e6572

AshwinSekar requested a review from joncinque January 24, 2025 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add duplicate block proof sigverify #5

add duplicate block proof sigverify #5

AshwinSekar commented Jan 9, 2025 •

edited

Loading

AshwinSekar Jan 9, 2025

joncinque Jan 16, 2025

joncinque left a comment

joncinque Jan 15, 2025

AshwinSekar Jan 23, 2025

joncinque Jan 16, 2025

AshwinSekar Jan 23, 2025

joncinque Jan 16, 2025

joncinque Jan 16, 2025

AshwinSekar Jan 23, 2025

joncinque Jan 16, 2025

joncinque Jan 16, 2025

AshwinSekar Jan 23, 2025

joncinque Jan 16, 2025

joncinque Jan 16, 2025

AshwinSekar Jan 23, 2025 •

edited

Loading

	/// Leader's sign the merkle root of each shred with their pubkey.
	/// Leaders sign the merkle root of each shred with their pubkey.

add duplicate block proof sigverify #5

Are you sure you want to change the base?

add duplicate block proof sigverify #5

Conversation

AshwinSekar commented Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AshwinSekar Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

AshwinSekar commented Jan 9, 2025 •

edited

Loading

AshwinSekar Jan 23, 2025 •

edited

Loading