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

feat: Validate globals in public kernel #6031

Merged
merged 2 commits into from
Apr 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions l1-contracts/src/core/libraries/ConstantsGen.sol
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,9 @@ library Constants {
+ MAX_PUBLIC_CALL_STACK_LENGTH_PER_CALL + (SIDE_EFFECT_LENGTH * MAX_NEW_NOTE_HASHES_PER_CALL)
+ (SIDE_EFFECT_LINKED_TO_NOTE_HASH_LENGTH * MAX_NEW_NULLIFIERS_PER_CALL)
+ (L2_TO_L1_MESSAGE_LENGTH * MAX_NEW_L2_TO_L1_MSGS_PER_CALL) + 2
+ (SIDE_EFFECT_LENGTH * MAX_UNENCRYPTED_LOGS_PER_CALL) + 1 + HEADER_LENGTH + AZTEC_ADDRESS_LENGTH /* revert_code */
+ 1 + 2 * GAS_LENGTH /* transaction_fee */ + 1;
+ (SIDE_EFFECT_LENGTH * MAX_UNENCRYPTED_LOGS_PER_CALL) + 1 + HEADER_LENGTH
+ GLOBAL_VARIABLES_LENGTH + AZTEC_ADDRESS_LENGTH /* revert_code */ + 1 + 2 * GAS_LENGTH /* transaction_fee */
+ 1;
uint256 internal constant PRIVATE_CALL_STACK_ITEM_LENGTH =
AZTEC_ADDRESS_LENGTH + FUNCTION_DATA_LENGTH + PRIVATE_CIRCUIT_PUBLIC_INPUTS_LENGTH;
uint256 internal constant ENQUEUE_PUBLIC_FUNCTION_CALL_RETURN_LENGTH =
Expand Down
3 changes: 2 additions & 1 deletion noir-projects/aztec-nr/aztec/src/context/private_context.nr
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::{
};
use dep::protocol_types::{
abis::{
gas::Gas, call_context::CallContext, function_data::FunctionData,
global_variables::GlobalVariables, gas::Gas, call_context::CallContext, function_data::FunctionData,
function_selector::FunctionSelector, max_block_number::MaxBlockNumber,
nullifier_key_validation_request::NullifierKeyValidationRequest,
private_call_stack_item::PrivateCallStackItem,
Expand Down Expand Up @@ -500,6 +500,7 @@ impl PrivateContext {
unencrypted_logs_hashes: [SideEffect::empty(); MAX_UNENCRYPTED_LOGS_PER_CALL],
unencrypted_log_preimages_length: 0,
historical_header: Header::empty(),
global_variables: GlobalVariables::empty(),
prover_address: AztecAddress::zero(),
revert_code: 0,
start_gas_left: Gas::empty(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ impl PublicContext {
unencrypted_logs_hashes: self.unencrypted_logs_hashes.storage,
unencrypted_log_preimages_length,
historical_header: self.inputs.historical_header,
global_variables: self.inputs.public_global_variables,
prover_address: self.prover_address,
revert_code: 0,
start_gas_left: self.inputs.gas_left,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ struct PrivateKernelInitCircuitPrivateInputs {

impl PrivateKernelInitCircuitPrivateInputs {
fn initialize_end_values(self, public_inputs: &mut PrivateKernelCircuitPublicInputsBuilder) {
public_inputs.constants = CombinedConstantData {
historical_header: self.private_call.call_stack_item.public_inputs.historical_header,
tx_context: self.tx_request.tx_context,
};
public_inputs.constants = CombinedConstantData::private(
self.private_call.call_stack_item.public_inputs.historical_header,
self.tx_request.tx_context,
);
public_inputs.min_revertible_side_effect_counter = self.private_call.call_stack_item.public_inputs.min_revertible_side_effect_counter;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ use dep::types::{
kernel_circuit_public_inputs::PublicKernelCircuitPublicInputsBuilder, kernel_data::PublicKernelData,
public_call_data::PublicCallData, public_data_read::PublicDataRead,
public_data_update_request::PublicDataUpdateRequest, read_request::ReadRequestContext,
side_effect::{SideEffect, SideEffectLinkedToNoteHash}
side_effect::{SideEffect, SideEffectLinkedToNoteHash}, global_variables::GlobalVariables,
combined_constant_data::CombinedConstantData
},
address::AztecAddress,
contrakt::{storage_read::StorageRead, storage_update_request::StorageUpdateRequest},
Expand Down Expand Up @@ -33,6 +34,34 @@ pub fn validate_inputs(public_call: PublicCallData) {
assert(public_call.bytecode_hash != 0, "Bytecode hash cannot be zero");
}

pub fn validate_public_call_global_variables(public_call: PublicCallData, constants: CombinedConstantData) {
let public_call_globals = public_call.call_stack_item.public_inputs.global_variables;
assert(
public_call_globals == constants.global_variables, "Global variables injected into the public call do not match constants"
);
}

// Validates constants injected into the public call are correct.
// Note that the previous_kernel.public_inputs.constants returned from the private kernel tail
// will be empty, so in the first run on of this circuit we load them from the first public
// call, following the same pattern as in the private_kernel_init.
// TODO(@spalladino): This can be a security risk since it allows a sequencer to run public
// circuits with empty global variables. This must be patched by having a differentiated init public
// circuit that runs only once, or by having a way to differentiate when we're coming from a private
// kernel tail vs from another public run.
pub fn initialize_from_or_validate_public_call_variables(
previous_kernel: PublicKernelData,
public_call: PublicCallData,
public_inputs: &mut PublicKernelCircuitPublicInputsBuilder
) {
if public_inputs.constants.global_variables.is_empty() {
let public_call_global_variables = public_call.call_stack_item.public_inputs.global_variables;
public_inputs.constants.global_variables = public_call_global_variables;
} else {
validate_public_call_global_variables(public_call, previous_kernel.public_inputs.constants);
}
}

pub fn validate_public_call_non_revert(public_call: PublicCallData) {
assert(public_call.call_stack_item.public_inputs.revert_code == 0, "Public call cannot be reverted");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ impl PublicKernelAppLogicCircuitPrivateInputs {
// validate the inputs common to all invocation circumstances
common::validate_inputs(self.public_call);

// validate constants injected into the public call are correct or set them if this is the first public call
common::initialize_from_or_validate_public_call_variables(self.previous_kernel, self.public_call, &mut public_inputs);

// validate the inputs unique to having a previous public kernel
self.validate_inputs();

Expand Down Expand Up @@ -485,4 +488,25 @@ mod tests {

builder.failed();
}

#[test]
fn propagates_global_variables_if_empty() {
let mut builder = PublicKernelAppLogicCircuitPrivateInputsBuilder::new();

builder.public_call.public_inputs.global_variables.block_number = 11;

let public_inputs = builder.execute();

assert_eq(public_inputs.constants.global_variables.block_number, 11);
}

#[test(should_fail_with="Global variables injected into the public call do not match constants")]
fn validates_global_variables() {
let mut builder = PublicKernelAppLogicCircuitPrivateInputsBuilder::new();

builder.previous_kernel.global_variables.block_number = 10;
builder.public_call.public_inputs.global_variables.block_number = 11;

builder.failed();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ struct PublicKernelSetupCircuitPrivateInputs {
// Note: One might think that our previous_kernel ought to be
// a PrivateKernelTailData. However, we instead supply a PublicKernelData.
// This is because PrivateKernelTailData is a subset of PublicKernelData.
// And we just initialize the missing values to zero in TS before passing it to the circuit.
// And we just initialize the missing values to zero in TS before passing it to the circuit,
// except for the constants.global_variables which we populate with the current block values.
// This is a bit of a hack, but it allows us to reuse the setup circuit until
// the setup phase of the public kernel is complete. Maybe in a perfect world we would
// have a SetupInit, SetupInner, etc, but this will change anyway once the public VM is able to
Expand Down Expand Up @@ -36,6 +37,9 @@ impl PublicKernelSetupCircuitPrivateInputs {
// validate the inputs common to all invocation circumstances
common::validate_inputs(self.public_call);

// validate constants injected into the public call are correct or set them if this is the first public call
common::initialize_from_or_validate_public_call_variables(self.previous_kernel, self.public_call, &mut public_inputs);

// validate the inputs unique to having a previous private kernel
self.validate_inputs();

Expand Down Expand Up @@ -540,4 +544,25 @@ mod tests {

builder.failed();
}

#[test]
fn propagates_global_variables_if_empty() {
let mut builder = PublicKernelSetupCircuitPrivateInputsBuilder::new();

builder.public_call.public_inputs.global_variables.block_number = 11;

let public_inputs = builder.execute();

assert_eq(public_inputs.constants.global_variables.block_number, 11);
}

#[test(should_fail_with="Global variables injected into the public call do not match constants")]
fn validates_global_variables() {
let mut builder = PublicKernelSetupCircuitPrivateInputsBuilder::new();

builder.previous_kernel.global_variables.block_number = 10;
builder.public_call.public_inputs.global_variables.block_number = 11;

builder.failed();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ impl PublicKernelTeardownCircuitPrivateInputs {
let transaction_fee = self.public_call.call_stack_item.public_inputs.transaction_fee;
// Note that teardown_gas is already included in end.gas_used as it was injected by the private kernel
let total_gas_used = self.previous_kernel.public_inputs.end.gas_used.add(self.previous_kernel.public_inputs.end_non_revertible.gas_used);
// TODO(palla/gas): Load gas fees from a PublicConstantData struct that's currently missing
let block_gas_fees = GasFees::default();
let block_gas_fees = self.previous_kernel.public_inputs.constants.global_variables.gas_fees;
let inclusion_fee = self.previous_kernel.public_inputs.constants.tx_context.gas_settings.inclusion_fee;
let computed_transaction_fee = total_gas_used.compute_fee(block_gas_fees) + inclusion_fee;

Expand Down Expand Up @@ -74,6 +73,9 @@ impl PublicKernelTeardownCircuitPrivateInputs {
// validate the inputs common to all invocation circumstances
common::validate_inputs(self.public_call);

// validate constants injected into the public call are correct or set them if this is the first public call
common::initialize_from_or_validate_public_call_variables(self.previous_kernel, self.public_call, &mut public_inputs);

// validate the inputs unique to having a previous private kernel
self.validate_inputs();

Expand Down Expand Up @@ -425,4 +427,25 @@ mod tests {

builder.failed();
}

#[test]
fn propagates_global_variables_if_empty() {
let mut builder = PublicKernelTeardownCircuitPrivateInputsBuilder::new();

builder.public_call.public_inputs.global_variables.block_number = 11;

let public_inputs = builder.execute();

assert_eq(public_inputs.constants.global_variables.block_number, 11);
}

#[test(should_fail_with="Global variables injected into the public call do not match constants")]
fn validates_global_variables() {
let mut builder = PublicKernelTeardownCircuitPrivateInputsBuilder::new();

builder.previous_kernel.global_variables.block_number = 10;
builder.public_call.public_inputs.global_variables.block_number = 11;

builder.failed();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,14 @@ impl BaseRollupInputs {
== self.constants.global_variables.version, "kernel version does not match the rollup version"
);

// Verify the kernel global variables if set, note these can be empty if this is a request coming directly from the private kernel tail.
// TODO(@spalladino) How can we check that this is a request coming from the private kernel tail?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if I can update this when I implement recursion from the kernels to the base rollup. We will know which verification key has been provided.

assert(
self.kernel_data.public_inputs.constants.global_variables.is_empty()
| (self.kernel_data.public_inputs.constants.global_variables
== self.constants.global_variables), "kernel global variables do not match the rollup global variables"
);

self.validate_kernel_start_state();

let rollup_validation_requests = self.kernel_data.public_inputs.rollup_validation_requests;
Expand Down Expand Up @@ -983,6 +991,14 @@ mod tests {
builder.fails();
}

#[test(should_fail_with = "kernel global variables do not match the rollup global variables")]
unconstrained fn constants_global_variables_dont_match_kernels() {
let mut builder = BaseRollupInputsBuilder::new();
builder.kernel_data.global_variables.block_number = 6;
builder.constants.global_variables.block_number = 7;
builder.fails();
}

#[test(should_fail_with = "kernel max_block_number is smaller than block number")]
unconstrained fn constants_dont_satisfy_smaller_max_block_number() {
let mut builder = BaseRollupInputsBuilder::new();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::transaction::tx_context::TxContext;
use crate::header::Header;
use crate::traits::Empty;
use crate::abis::global_variables::GlobalVariables;

struct CombinedConstantData {
historical_header: Header,
Expand All @@ -9,13 +10,22 @@ struct CombinedConstantData {
// a situation we could be using header from a block before the upgrade took place but be using the updated
// protocol to execute and prove the transaction.
tx_context: TxContext,

global_variables: GlobalVariables,
}

impl CombinedConstantData {
pub fn private(historical_header: Header, tx_context: TxContext) -> CombinedConstantData {
CombinedConstantData { historical_header, tx_context, global_variables: GlobalVariables::empty() }
}
}

impl Empty for CombinedConstantData {
fn empty() -> Self {
CombinedConstantData {
historical_header: Header::empty(),
tx_context: TxContext::empty(),
global_variables: GlobalVariables::empty()
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ impl GasFees {
pub fn default() -> Self {
GasFees::new(1, 1, 1)
}

pub fn is_empty(self) -> bool {
(self.fee_per_da_gas == 0) & (self.fee_per_l1_gas == 0) & (self.fee_per_l2_gas == 0)
}
}

impl Serialize<GAS_FEES_LENGTH> for GasFees {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,18 @@ struct GlobalVariables {
}
// docs:end:global-variables

impl GlobalVariables {
fn is_empty(self) -> bool {
(self.chain_id == 0)
& (self.version == 0)
& (self.block_number == 0)
& (self.timestamp == 0)
& (self.coinbase.is_zero())
& (self.fee_recipient.is_zero())
& (self.gas_fees.is_empty())
}
}

impl Serialize<GLOBAL_VARIABLES_LENGTH> for GlobalVariables {
fn serialize(self) -> [Field; GLOBAL_VARIABLES_LENGTH] {
let mut serialized: BoundedVec<Field, GLOBAL_VARIABLES_LENGTH> = BoundedVec::new();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ mod tests {
let call_stack_item = PublicCallStackItem { contract_address, public_inputs, is_execution_request: true, function_data };

// Value from public_call_stack_item.test.ts "Computes a callstack item request hash" test
let test_data_call_stack_item_request_hash = 0x1b06f4a4960455e9f01c20d4cb01afbf8c8f39eb50094c5d1ad6725ced0f7d08;
let test_data_call_stack_item_request_hash = 0x22848497ff97ff3a4517aec32454059030fb5a3ef4f3ca533ee40132d7a63aea;
assert_eq(call_stack_item.hash(), test_data_call_stack_item_request_hash);
}

Expand All @@ -87,7 +87,7 @@ mod tests {
let call_stack_item = PublicCallStackItem { contract_address, public_inputs, is_execution_request: false, function_data };

// Value from public_call_stack_item.test.ts "Computes a callstack item hash" test
let test_data_call_stack_item_hash = 0x1f3f1902ca41ffd6fd7191fa5a52edd677444a9b6ae8f4448336fa71a4b2d5cc;
let test_data_call_stack_item_hash = 0x0e18ddd9aaddae02d45598f0278d925e289913384d6e15057ce5b4a9e8e7488d;
assert_eq(call_stack_item.hash(), test_data_call_stack_item_hash);
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::{
abis::{
call_context::CallContext, read_request::ReadRequest,
side_effect::{SideEffect, SideEffectLinkedToNoteHash}, gas::Gas
side_effect::{SideEffect, SideEffectLinkedToNoteHash}, gas::Gas, global_variables::GlobalVariables
},
address::AztecAddress,
constants::{
Expand Down Expand Up @@ -46,6 +46,9 @@ struct PublicCircuitPublicInputs {
// previous to the one in which the tx is included.
historical_header: Header,

// Global variables injected into this circuit
global_variables: GlobalVariables,

prover_address: AztecAddress,

revert_code: u8,
Expand Down Expand Up @@ -99,6 +102,7 @@ impl Serialize<PUBLIC_CIRCUIT_PUBLIC_INPUTS_LENGTH> for PublicCircuitPublicInput
}
fields.push(self.unencrypted_log_preimages_length);
fields.extend_from_array(self.historical_header.serialize());
fields.extend_from_array(self.global_variables.serialize());
fields.push(self.prover_address.to_field());
fields.push(self.revert_code as Field);
fields.extend_from_array(self.start_gas_left.serialize());
Expand Down Expand Up @@ -129,6 +133,7 @@ impl Deserialize<PUBLIC_CIRCUIT_PUBLIC_INPUTS_LENGTH> for PublicCircuitPublicInp
unencrypted_logs_hashes: reader.read_struct_array(SideEffect::deserialize, [SideEffect::empty(); MAX_UNENCRYPTED_LOGS_PER_CALL]),
unencrypted_log_preimages_length: reader.read(),
historical_header: reader.read_struct(Header::deserialize),
global_variables: reader.read_struct(GlobalVariables::deserialize),
prover_address: reader.read_struct(AztecAddress::deserialize),
revert_code: reader.read() as u8,
start_gas_left: reader.read_struct(Gas::deserialize),
Expand Down Expand Up @@ -166,6 +171,7 @@ impl Empty for PublicCircuitPublicInputs {
unencrypted_logs_hashes: [SideEffect::empty(); MAX_UNENCRYPTED_LOGS_PER_CALL],
unencrypted_log_preimages_length: 0,
historical_header: Header::empty(),
global_variables: GlobalVariables::empty(),
prover_address: AztecAddress::zero(),
revert_code: 0 as u8,
start_gas_left: Gas::empty(),
Expand All @@ -189,6 +195,6 @@ fn empty_hash() {
let hash = inputs.hash();

// Value from public_circuit_public_inputs.test.ts "computes empty item hash" test
let test_data_empty_hash = 0x237c89f8b29c3fb169b889940a714b3c72017cb2941d0724d4668a030794d2fb;
let test_data_empty_hash = 0x2d91debc43bd6354caef4fd152975e7c6dd44e8623b6b62c21b9f547f2fabd32;
assert_eq(hash, test_data_empty_hash);
}
Loading
Loading