Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a crucial verification step to ensure data fetched from a fallback node matches the on-chain commitment. The implementation is mostly correct, but I've found a critical issue in the validation logic, along with some leftover debugging code and a missed validation case. My review includes suggestions to fix the critical bug, clean up the code, and ensure the verification is applied consistently.
| pub fn validate_commitment(&self, blob_id: &str, data: &[u8]) -> anyhow::Result<(), DAError> { | ||
| let commitment = | ||
| Commitment::from_blob(self.namespace, &data, SHARE_VERSION_ZERO).map_err(|error| { | ||
| DAError { | ||
| error: anyhow!("Error to create commitment: {}", error.to_string()), | ||
| is_retriable: false, | ||
| } | ||
| })?; | ||
|
|
||
| // let commitment_result = | ||
| // match Commitment::from_blob(self.namespace, SHARE_VERSION_ZERO, data) { | ||
| // Ok(commit) => commit, | ||
| // Err(error) => { | ||
| // return Err(DAError { | ||
| // error: error.into(), | ||
| // is_retriable: false, | ||
| // }) | ||
| // } | ||
| // }; | ||
|
|
||
| let blob_id_bytes = hex::decode(blob_id).map_err(|error| DAError { | ||
| error: error.into(), | ||
| is_retriable: true, | ||
| })?; | ||
|
|
||
| // Prepend the block height to the blob_id | ||
| // let mut blob_id_with_block_height = Vec::with_capacity(8 + 32); | ||
| // blob_id_with_block_height.extend_from_slice(&(0 as u64).to_be_bytes()); | ||
| // blob_id_with_block_height.extend_from_slice(&blob_id_bytes); | ||
| // let blob_id_str = hex::encode(blob_id_with_block_height.clone()); | ||
|
|
||
| // println!( | ||
| // "*****************************: {:?}", | ||
| // blob_id_with_block_height | ||
| // ); | ||
|
|
||
| let (commitment, _) = parse_blob_id(&blob_id).map_err(|error| DAError { | ||
| error: error.into(), | ||
| is_retriable: true, | ||
| })?; | ||
|
|
||
| println!( | ||
| "-----------------------------------------------: {:?}", | ||
| commitment.0 | ||
| ); | ||
|
|
||
| println!( | ||
| "-----------------------------------------------: {:?}", | ||
| commitment_result.0 | ||
| ); | ||
|
|
||
| if commitment.0 != commitment_result.0 { | ||
| return Err(DAError { | ||
| error: anyhow::anyhow!("Commitment mismatch"), | ||
| is_retriable: false, | ||
| }); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The validate_commitment function has several issues that need to be addressed:
- Incorrect Argument Order: The arguments to
Commitment::from_blobappear to be swapped. Based on its usage elsewhere, the signature is likely(Namespace, u8, &[u8]). The current implementation will fail to compile or produce an incorrect commitment, defeating the purpose of this validation. - Leftover Debug Code: The function contains
println!statements and large blocks of commented-out code, which should be removed. - Redundant Logic: The function decodes
blob_idmanually, even thoughparse_blob_idis also called and performs the same operation. - Uncompilable Code: The function references a
commitment_resultvariable that is not defined in the uncommented code, which will cause a compilation error.
I suggest refactoring the entire function for clarity and correctness.
pub fn validate_commitment(&self, blob_id: &str, data: &[u8]) -> Result<(), DAError> {
let calculated_commitment =
Commitment::from_blob(self.namespace, SHARE_VERSION_ZERO, data).map_err(|error| {
DAError {
error: error.into(),
is_retriable: false,
}
})?;
let (expected_commitment, _) = parse_blob_id(blob_id).map_err(|error| DAError {
error: error.into(),
is_retriable: true,
})?;
if calculated_commitment.0 != expected_commitment.0 {
return Err(DAError {
error: anyhow::anyhow!("Commitment mismatch"),
is_retriable: false,
});
}
Ok(())
}| // self.validate_commitment(blob_id, &data) | ||
| // .map_err(|e| DAError { | ||
| // error: e.into(), | ||
| // is_retriable: false, | ||
| // })?; |
There was a problem hiding this comment.
The commitment validation is commented out for blobs that are proofs (blob.is_proof). To ensure all data from the external node is verified against its on-chain commitment, this validation should be enabled for all blob types.
self.validate_commitment(blob_id, &data)
.map_err(|e| DAError {
error: e.into(),
is_retriable: false,
})?;| println!( | ||
| "-----------------------------------------------{:?}", | ||
| blob.pub_data | ||
| ); |
There was a problem hiding this comment.
This println! statement appears to be leftover debugging code and should be removed.
| println!( | ||
| "----------------------PUBDATA-------------------------: {:?}", | ||
| hex::encode(batch.pubdata.clone()) | ||
| ); |
There was a problem hiding this comment.
This println! statement appears to be for debugging and should be removed.
2 participants
What ❔
Why ❔
When the verifier falls back to the node to fetch DA, it must verify that the returned data matches the on-chain commitment hash stored on Celestia, rather than trusting the node to provide honest and valid data.
Checklist
zkstack dev fmtandzkstack dev lint.