-
Notifications
You must be signed in to change notification settings - Fork 100
feat(l2): remove public inputs from verify() function in OnChainProposer #4147
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
base: main
Are you sure you want to change the base?
feat(l2): remove public inputs from verify() function in OnChainProposer #4147
Conversation
…ice; align interfaces
…e sequencer and prover; drop calldata public values
…r to 256-byte PIs
publicInputs[192 + i] = bytes1(uint8(uint256(chainIdBytes) >> (8 * (31 - i)))); | ||
} | ||
|
||
// Non-privileged transactions count = 0 (bytes 224..256 already zeroed) |
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.
this shouldn't be fixed to zero right? the field processedPrivilegedTransactionsRollingHash
in the batch commitment contains this value: it's the first two bytes (it's used in line 280 of this file too)
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.
Thanks for flagging this. The last 32 bytes in the contract public inputs are intentionally zero, right? The privileged transaction count is taken from a different place, it’s encoded in the first two bytes of processedPrivilegedTransactionsRollingHash (which we already read to remove pending privileged transactions). Separately, we enforce onchain that if privileged transactions have expired, then the non‑privileged count must be zero. I clarified this intent in comments and kept the last 32 bytes zero by design. Could you confirm you agree the privileged count comes from the rolling‑hash prefix and that the final 32‑byte slot should remai zero?
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.
Sorry! I mixed privileged count
up with non-privileged count
. Yes it's correct that the non-privileged count should be 0, but in the case where the privileged transaction inclusion time has expired, else the non-privileged count can be any other number, and because it's part of the public inputs of the proof, we need it for verification.
Problem here is that we don't have this number as part of the block commitment, or anywhere in the L1 (we do have the privileged count but we can't derive the non-privileged from that alone), so by removing the public inputs parameter from the verify() function we don't have a way to access this value.
We talked internally with @avilagaston9 and the solution here should be to add the non-privileged count as a new field of the batch commitment. This would require, if I didn't miss anything:
- Adding this new field to the
BatchCommitmentInfo
struct in Solidity - Adding the field as a parameter of the
commitBatch()
function - Update the
send_commitment()
function of the L1 commiter module in Rust to add the new field
I think this is necessary to finish this PR so those changes could be added in this same branch. If you have any questions don't hesitate on asking us!
(about the "expire time" thing: this is a mechanism in which we give the sequencer some amount of time to process privileged transactions, else it won't be able to settle non-privileged transactions, so no new transactions until it processes the pending privileged transactions, this is a way to avoid the sequencer to censor L1->L2 messages. For example, if a user deposits some ETH from L1 to L2, they know that the sequencer will include that deposit and their funds won't be lost, else the whole L2 stops)
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.
Thanks for the clarifications sir! All clear, pushing the update in a sec
/// Encode public inputs exactly as expected by the on-chain contract (256 bytes total). | ||
/// Layout: | ||
/// - 0..32: initial_state_hash | ||
/// - 32..64: final_state_hash | ||
/// - 64..96: l1messages_merkle_root (withdrawals root) | ||
/// - 96..128: privileged_transactions_hash (processed privileged rolling hash) | ||
/// - 128..160: blob_versioned_hash | ||
/// - 160..192: last_block_hash | ||
/// - 192..224: chain_id (big-endian, 32 bytes) | ||
/// - 224..256: zeroes (force non-privileged count to 0) | ||
#[cfg(feature = "l2")] | ||
pub fn encode_contract_pis(&self) -> Vec<u8> { | ||
[ | ||
self.initial_state_hash.to_fixed_bytes(), | ||
self.final_state_hash.to_fixed_bytes(), | ||
self.l1messages_merkle_root.to_fixed_bytes(), | ||
self.privileged_transactions_hash.to_fixed_bytes(), | ||
self.blob_versioned_hash.to_fixed_bytes(), | ||
self.last_block_hash.to_fixed_bytes(), | ||
self.chain_id.to_big_endian(), | ||
[0u8; 32], | ||
] | ||
.concat() | ||
} |
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.
this is part of the other comment, I see that the reason this function exists is to hardcode the non-privileged tx count to 0
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.
Hi @pedrobergamini! Thanks for your contribution. Left some comments
); | ||
} | ||
|
||
if (SP1VERIFIER != DEV_MODE) { | ||
// If the verification fails, it will revert. | ||
_verifyPublicData(batchNumber, sp1PublicValues[8:]); | ||
_verifyPublicData(batchNumber, publicInputs); |
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.
Given that we are constructing the publicInputs
from the values of BatchCommitmentInfo
there is no need to call _verifyPublicData()
as it will always pass. We can remove the function. The only check that should remain is the hasExpiredPrivilegedTransactions()
call.
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.
The comment applies for both based
and non-based
contract.
// Commit the contract public inputs | ||
sp1_zkvm::io::commit_slice(&output.encode_contract_pis()); |
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.
Let's restore this, remove the encode_contract_pis()
function, and update the ProgramOutput::encode()
method to reflect the new changes instead. This change will break the proving of L1 blocks.
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.
I would keep the commit_slice() function though, so not entirely restore the line but just change it to commit_slice(output.encode())
// Compute non-privileged transactions count for the batch | ||
let mut total_txs: u64 = 0; | ||
let mut privileged_txs: u64 = 0; | ||
|
||
for i in batch.first_block..=batch.last_block { | ||
let block_body = self | ||
.store | ||
.get_block_body(i) | ||
.await | ||
.map_err(CommitterError::from)? | ||
.ok_or(CommitterError::FailedToRetrieveDataFromStorage)?; | ||
|
||
let txs_in_block: u64 = block_body | ||
.transactions | ||
.len() | ||
.try_into() | ||
.unwrap_or(u64::MAX); | ||
total_txs = total_txs.saturating_add(txs_in_block); | ||
let priv_in_block = get_block_privileged_transactions(&block_body.transactions); | ||
let priv_in_block_count: u64 = priv_in_block.len().try_into().unwrap_or(u64::MAX); | ||
privileged_txs = privileged_txs.saturating_add(priv_in_block_count); | ||
} | ||
let non_privileged_count = total_txs.saturating_sub(privileged_txs); | ||
|
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.
We can add a new field, non_privileged_count
, to our Batch
struct and return its value as the result of prepare_batch_from_block() to avoid fetching and iterating over the blocks again.
// Insert count before encoded blocks for based ABI | ||
calldata_values.push(Value::Uint(U256::from(non_privileged_count))); | ||
calldata_values.push(Value::Array( | ||
encoded_blocks.into_iter().map(Value::Bytes).collect(), | ||
)); | ||
|
||
(COMMIT_FUNCTION_SIGNATURE_BASED, calldata_values) | ||
} else { | ||
// Append count as last arg for non-based ABI | ||
calldata_values.push(Value::Uint(U256::from(non_privileged_count))); |
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.
We can call calldata_values.push(Value::Uint(U256::from(non_privileged_count)));
only once before line 460.
// Note: the last 32 bytes encode the non-privileged transaction count. | ||
// The privileged transaction count is encoded separately in the first two bytes of | ||
// `processedPrivilegedTransactionsRollingHash` and is handled during verification. | ||
return abi.encodePacked( |
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 non-based version you used bytes.concat() here, we should use the same function for both implementations, I think bytes.concat() is clearer
Motivation
This PR implements the removal of public inputs from the
verifyBatch()
function in theOnChainProposer
contract. Public inputs are now retrieved from the committed batch data instead of being passed as parameters.Description
This PR removes public inputs from the
verifyBatch
function signature and reconstructs them from already-committed batch data. This simplifies the interface and reduces calldata size.OnChainProposer.sol
verifyBatch
signature: Removed the RISC0 journal parameter. Final signature:verifyBatch(uint256 batchNumber, bytes risc0BlockProof, bytes sp1ProofBytes, bytes tdxSignature)
sp1ProofBytes
is provided. SP1 public values are constructed on-chain asabi.encodePacked(uint64(publicInputs.length), publicInputs)
, and public data checks usesp1PublicValues[8:]
.tdxSignature
is provided._getPublicInputsFromCommitment
: Internal helper that reconstructs 256-byte public inputs from committed batch data:sha256(publicInputs)
sp1PublicValues = 8-byte length prefix + publicInputs
; validation oversp1PublicValues[8:]
publicInputs
verified againsttdxSignature
zkVM Interface Updates
commit_slice(output.encode_contract_pis())
.commit_slice(output.encode_contract_pis())
.encode_contract_pis()
).l1_proof_sender.rs
VERIFY_FUNCTION_SIGNATURE
to"verifyBatch(uint256,bytes,bytes,bytes)"
.Proving systems
empty_calldata()
for proving systems:sp1ProofBytes
.Previous PR
Closes #2804