From 650fda0201d5307cc2dd811e3fc2510c8f5d57a8 Mon Sep 17 00:00:00 2001 From: Leila Wang Date: Thu, 28 Nov 2024 12:30:59 +0000 Subject: [PATCH 01/10] Check mas_fees_per_gas in base rollup. --- .../base_or_merge_rollup_public_inputs.nr | 2 +- .../crates/rollup-lib/src/abis/mod.nr | 1 - .../src/base/components/constants.nr | 5 ++-- .../rollup-lib/src/base/components/mod.nr | 7 +++++ .../components/private_tube_data_validator.nr | 28 +++++++++++++++++++ .../components/public_tube_data_validator.nr | 28 +++++++++++++++++++ .../src/base/components/validate_tube_data.nr | 12 ++++++++ .../crates/rollup-lib/src/base/mod.nr | 1 + .../src/base/private_base_rollup.nr | 26 +++++++---------- .../rollup-lib/src/base/public_base_rollup.nr | 21 ++++++-------- .../crates/rollup-lib/src/base/tests/mod.nr | 1 + .../mod.nr | 24 ++++++++++++++++ .../validate_with_rollup_data.nr | 27 ++++++++++++++++++ .../src/abis/constant_rollup_data.nr | 2 +- .../crates/types/src/abis/gas_settings.nr | 8 +++--- .../crates/types/src/abis/mod.nr | 1 + .../crates/types/src/abis/tube.nr | 2 +- .../crates/types/src/tests/fixture_builder.nr | 19 +++++++++++-- 18 files changed, 174 insertions(+), 41 deletions(-) create mode 100644 noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/private_tube_data_validator.nr create mode 100644 noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/public_tube_data_validator.nr create mode 100644 noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/validate_tube_data.nr create mode 100644 noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/mod.nr create mode 100644 noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/private_tube_data_validator_builder/mod.nr create mode 100644 noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/private_tube_data_validator_builder/validate_with_rollup_data.nr rename noir-projects/noir-protocol-circuits/crates/{rollup-lib => types}/src/abis/constant_rollup_data.nr (99%) diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/abis/base_or_merge_rollup_public_inputs.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/abis/base_or_merge_rollup_public_inputs.nr index 40a7a09f8fba..0c5f8647c089 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/abis/base_or_merge_rollup_public_inputs.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/abis/base_or_merge_rollup_public_inputs.nr @@ -1,5 +1,5 @@ -use crate::abis::constant_rollup_data::ConstantRollupData; use dep::types::{ + abis::constant_rollup_data::ConstantRollupData, constants::BASE_OR_MERGE_PUBLIC_INPUTS_LENGTH, partial_state_reference::PartialStateReference, traits::{Deserialize, Empty, Serialize}, diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/abis/mod.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/abis/mod.nr index 716b67bf953e..0bcaaa80b27d 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/abis/mod.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/abis/mod.nr @@ -1,4 +1,3 @@ -pub(crate) mod constant_rollup_data; pub(crate) mod base_or_merge_rollup_public_inputs; pub(crate) mod block_root_or_block_merge_public_inputs; pub(crate) mod previous_rollup_data; diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/constants.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/constants.nr index 0e7f459fecdf..dbe2eb34fd6d 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/constants.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/constants.nr @@ -1,5 +1,6 @@ -use crate::abis::constant_rollup_data::ConstantRollupData; -use types::abis::combined_constant_data::CombinedConstantData; +use types::abis::{ + combined_constant_data::CombinedConstantData, constant_rollup_data::ConstantRollupData, +}; pub(crate) fn validate_combined_constant_data( constants: CombinedConstantData, diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/mod.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/mod.nr index 9e3e70f9433b..128c7bed5bdf 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/mod.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/mod.nr @@ -3,3 +3,10 @@ pub(crate) mod constants; pub(crate) mod fees; pub(crate) mod nullifier_tree; pub(crate) mod public_data_tree; + +mod private_tube_data_validator; +mod public_tube_data_validator; +pub(crate) mod validate_tube_data; + +pub(crate) use private_tube_data_validator::PrivateTubeDataValidator; +pub(crate) use public_tube_data_validator::PublicTubeDataValidator; diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/private_tube_data_validator.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/private_tube_data_validator.nr new file mode 100644 index 000000000000..64f27feb546a --- /dev/null +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/private_tube_data_validator.nr @@ -0,0 +1,28 @@ +use super::validate_tube_data::validate_max_fees_per_gas; +use dep::types::abis::{constant_rollup_data::ConstantRollupData, tube::PrivateTubeData}; + +pub struct PrivateTubeDataValidator { + pub data: PrivateTubeData, +} + +// TODO: Move relevant verifications here. +impl PrivateTubeDataValidator { + pub fn new(data: PrivateTubeData) -> Self { + PrivateTubeDataValidator { data } + } + + pub fn verify_proof(self, _allowed_previous_circuits: [u32; N]) { + if !dep::std::runtime::is_unconstrained() { + self.data.verify(); + // TODO(#7410) + // self.tube_data.vk_data.validate_in_vk_tree(self.tube_data.public_inputs.constants.vk_tree_root, ALLOWED_PREVIOUS_CIRCUITS); + } + } + + pub fn validate_with_rollup_data(self, constants: ConstantRollupData) { + validate_max_fees_per_gas( + self.data.public_inputs.constants.tx_context.gas_settings.max_fees_per_gas, + constants.global_variables.gas_fees, + ); + } +} diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/public_tube_data_validator.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/public_tube_data_validator.nr new file mode 100644 index 000000000000..341d04f424e6 --- /dev/null +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/public_tube_data_validator.nr @@ -0,0 +1,28 @@ +use super::validate_tube_data::validate_max_fees_per_gas; +use dep::types::abis::{constant_rollup_data::ConstantRollupData, tube::PublicTubeData}; + +pub struct PublicTubeDataValidator { + pub data: PublicTubeData, +} + +// TODO: Move relevant verifications here. +impl PublicTubeDataValidator { + pub fn new(data: PublicTubeData) -> Self { + PublicTubeDataValidator { data } + } + + pub fn verify_proof(self) { + if !dep::std::runtime::is_unconstrained() { + self.data.verify(); + // TODO(#7410) + // self.tube_data.vk_data.validate_in_vk_tree(self.tube_data.public_inputs.constants.vk_tree_root, ALLOWED_PREVIOUS_CIRCUITS); + } + } + + pub fn validate_with_rollup_data(self, constants: ConstantRollupData) { + validate_max_fees_per_gas( + self.data.public_inputs.constants.tx_context.gas_settings.max_fees_per_gas, + constants.global_variables.gas_fees, + ); + } +} diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/validate_tube_data.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/validate_tube_data.nr new file mode 100644 index 000000000000..5820fcfa953a --- /dev/null +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/validate_tube_data.nr @@ -0,0 +1,12 @@ +use dep::types::abis::gas_fees::GasFees; + +pub fn validate_max_fees_per_gas(max_fees_per_gas: GasFees, gas_fees: GasFees) { + assert( + !gas_fees.fee_per_da_gas.lt(max_fees_per_gas.fee_per_da_gas), + "fee_per_da_gas in settings must not be less than the global value", + ); + assert( + !gas_fees.fee_per_l2_gas.lt(max_fees_per_gas.fee_per_l2_gas), + "fee_per_l2_gas in settings must not be less than the global value", + ); +} diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/mod.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/mod.nr index 03b442b4eaa0..9b9bc52e9bd8 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/mod.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/mod.nr @@ -2,6 +2,7 @@ pub(crate) mod components; pub(crate) mod state_diff_hints; mod private_base_rollup; mod public_base_rollup; +mod tests; pub use crate::abis::base_or_merge_rollup_public_inputs::BaseOrMergeRollupPublicInputs; pub use private_base_rollup::PrivateBaseRollupInputs; diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/private_base_rollup.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/private_base_rollup.nr index e1c000a7b781..b5d2c1b649d2 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/private_base_rollup.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/private_base_rollup.nr @@ -1,13 +1,11 @@ use crate::{ - abis::{ - base_or_merge_rollup_public_inputs::{BASE_ROLLUP_TYPE, BaseOrMergeRollupPublicInputs}, - constant_rollup_data::ConstantRollupData, - }, + abis::base_or_merge_rollup_public_inputs::{BASE_ROLLUP_TYPE, BaseOrMergeRollupPublicInputs}, base::{ components::{ archive::perform_archive_membership_check, constants::validate_combined_constant_data, fees::compute_fee_payer_fee_juice_balance_leaf_slot, - nullifier_tree::nullifier_tree_batch_insert, public_data_tree::public_data_tree_insert, + nullifier_tree::nullifier_tree_batch_insert, PrivateTubeDataValidator, + public_data_tree::public_data_tree_insert, }, state_diff_hints::PrivateBaseStateDiffHints, }, @@ -15,7 +13,7 @@ use crate::{ }; use dep::types::{ abis::{ - append_only_tree_snapshot::AppendOnlyTreeSnapshot, + append_only_tree_snapshot::AppendOnlyTreeSnapshot, constant_rollup_data::ConstantRollupData, nullifier_leaf_preimage::NullifierLeafPreimage, public_data_write::PublicDataWrite, tube::PrivateTubeData, }, @@ -55,11 +53,9 @@ impl PrivateBaseRollupInputs { } pub fn execute(self) -> BaseOrMergeRollupPublicInputs { - if !dep::std::runtime::is_unconstrained() { - self.tube_data.verify(); - // TODO(#7410) - // self.tube_data.vk_data.validate_in_vk_tree(self.tube_data.public_inputs.constants.vk_tree_root, ALLOWED_PREVIOUS_CIRCUITS); - } + let tube_data_validator = PrivateTubeDataValidator::new(self.tube_data); + tube_data_validator.verify_proof(ALLOWED_PREVIOUS_CIRCUITS); + tube_data_validator.validate_with_rollup_data(self.constants); let transaction_fee = self.compute_transaction_fee(); @@ -221,10 +217,7 @@ impl PrivateBaseRollupInputs { mod tests { use crate::{ - abis::{ - base_or_merge_rollup_public_inputs::BaseOrMergeRollupPublicInputs, - constant_rollup_data::ConstantRollupData, - }, + abis::base_or_merge_rollup_public_inputs::BaseOrMergeRollupPublicInputs, base::{ components::fees::compute_fee_payer_fee_juice_balance_leaf_slot, private_base_rollup::PrivateBaseRollupInputs, @@ -234,7 +227,8 @@ mod tests { }; use dep::types::{ abis::{ - append_only_tree_snapshot::AppendOnlyTreeSnapshot, gas::Gas, gas_fees::GasFees, + append_only_tree_snapshot::AppendOnlyTreeSnapshot, + constant_rollup_data::ConstantRollupData, gas::Gas, gas_fees::GasFees, kernel_circuit_public_inputs::KernelCircuitPublicInputs, nullifier_leaf_preimage::NullifierLeafPreimage, }, diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/public_base_rollup.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/public_base_rollup.nr index c7a689749da8..ee97bd479c91 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/public_base_rollup.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/public_base_rollup.nr @@ -1,13 +1,11 @@ use crate::{ - abis::{ - base_or_merge_rollup_public_inputs::{BASE_ROLLUP_TYPE, BaseOrMergeRollupPublicInputs}, - constant_rollup_data::ConstantRollupData, - }, + abis::base_or_merge_rollup_public_inputs::{BASE_ROLLUP_TYPE, BaseOrMergeRollupPublicInputs}, base::{ components::{ archive::perform_archive_membership_check, constants::validate_combined_constant_data, fees::compute_fee_payer_fee_juice_balance_leaf_slot, nullifier_tree::nullifier_tree_batch_insert, public_data_tree::public_data_tree_insert, + PublicTubeDataValidator, }, state_diff_hints::PublicBaseStateDiffHints, }, @@ -19,6 +17,7 @@ use dep::types::{ append_only_tree_snapshot::AppendOnlyTreeSnapshot, avm_circuit_public_inputs::AvmProofData, combined_constant_data::CombinedConstantData, + constant_rollup_data::ConstantRollupData, log_hash::{LogHash, ScopedLogHash}, nullifier_leaf_preimage::NullifierLeafPreimage, public_data_write::PublicDataWrite, @@ -109,11 +108,9 @@ impl PublicBaseRollupInputs { } pub fn execute(self) -> BaseOrMergeRollupPublicInputs { - if !dep::std::runtime::is_unconstrained() { - self.tube_data.verify(); - // TODO(#7410) - // self.tube_data.vk_data.validate_in_vk_tree([TUBE_VK_INDEX]); - } + let tube_data_validator = PublicTubeDataValidator::new(self.tube_data); + tube_data_validator.verify_proof(); + tube_data_validator.validate_with_rollup_data(self.constants); // TODO(#8470) // if !dep::std::runtime::is_unconstrained() { @@ -363,10 +360,7 @@ impl PublicBaseRollupInputs { mod tests { use crate::{ - abis::{ - base_or_merge_rollup_public_inputs::BaseOrMergeRollupPublicInputs, - constant_rollup_data::ConstantRollupData, - }, + abis::base_or_merge_rollup_public_inputs::BaseOrMergeRollupPublicInputs, base::{ components::fees::compute_fee_payer_fee_juice_balance_leaf_slot, public_base_rollup::PublicBaseRollupInputs, state_diff_hints::PublicBaseStateDiffHints, @@ -376,6 +370,7 @@ mod tests { use dep::types::{ abis::{ append_only_tree_snapshot::AppendOnlyTreeSnapshot, + constant_rollup_data::ConstantRollupData, nullifier_leaf_preimage::NullifierLeafPreimage, public_data_write::PublicDataWrite, }, address::{AztecAddress, EthAddress}, diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/mod.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/mod.nr new file mode 100644 index 000000000000..7e88fe33e7df --- /dev/null +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/mod.nr @@ -0,0 +1 @@ +mod private_tube_data_validator_builder; diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/private_tube_data_validator_builder/mod.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/private_tube_data_validator_builder/mod.nr new file mode 100644 index 000000000000..cefbf64d37db --- /dev/null +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/private_tube_data_validator_builder/mod.nr @@ -0,0 +1,24 @@ +mod validate_with_rollup_data; + +use crate::base::components::PrivateTubeDataValidator; +use dep::types::tests::fixture_builder::FixtureBuilder; + +pub struct PrivateTubeDataValidatorBuilder { + pub tube_data: FixtureBuilder, + pub rollup_data: FixtureBuilder, +} + +impl PrivateTubeDataValidatorBuilder { + pub fn new() -> Self { + PrivateTubeDataValidatorBuilder { + tube_data: FixtureBuilder::new(), + rollup_data: FixtureBuilder::new(), + } + } + + pub fn validate_with_rollup_data(self) { + let tube_data = self.tube_data.to_private_tube_data(); + let rollup_data = self.rollup_data.to_constant_rollup_data(); + PrivateTubeDataValidator::new(tube_data).validate_with_rollup_data(rollup_data); + } +} diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/private_tube_data_validator_builder/validate_with_rollup_data.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/private_tube_data_validator_builder/validate_with_rollup_data.nr new file mode 100644 index 000000000000..e5119533bde4 --- /dev/null +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/private_tube_data_validator_builder/validate_with_rollup_data.nr @@ -0,0 +1,27 @@ +use super::PrivateTubeDataValidatorBuilder; + +#[test] +fn validate_with_rollup_data_success() { + let builder = PrivateTubeDataValidatorBuilder::new(); + builder.validate_with_rollup_data(); +} + +#[test(should_fail_with = "fee_per_da_gas in settings must not be less than the global value")] +fn validate_with_rollup_data_not_enough_fee_per_da_gas_fails() { + let mut builder = PrivateTubeDataValidatorBuilder::new(); + + builder.tube_data.tx_context.gas_settings.max_fees_per_gas.fee_per_da_gas = 4; + builder.rollup_data.global_variables.gas_fees.fee_per_da_gas = 3; + + builder.validate_with_rollup_data(); +} + +#[test(should_fail_with = "fee_per_l2_gas in settings must not be less than the global value")] +fn validate_with_rollup_data_not_enough_fee_per_l2_gas_fails() { + let mut builder = PrivateTubeDataValidatorBuilder::new(); + + builder.tube_data.tx_context.gas_settings.max_fees_per_gas.fee_per_l2_gas = 4; + builder.rollup_data.global_variables.gas_fees.fee_per_l2_gas = 3; + + builder.validate_with_rollup_data(); +} diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/abis/constant_rollup_data.nr b/noir-projects/noir-protocol-circuits/crates/types/src/abis/constant_rollup_data.nr similarity index 99% rename from noir-projects/noir-protocol-circuits/crates/rollup-lib/src/abis/constant_rollup_data.nr rename to noir-projects/noir-protocol-circuits/crates/types/src/abis/constant_rollup_data.nr index 9eab56a2092d..0a8d6919164b 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/abis/constant_rollup_data.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/abis/constant_rollup_data.nr @@ -1,4 +1,4 @@ -use dep::types::{ +use crate::{ abis::{append_only_tree_snapshot::AppendOnlyTreeSnapshot, global_variables::GlobalVariables}, constants::CONSTANT_ROLLUP_DATA_LENGTH, traits::{Deserialize, Empty, Serialize}, diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/abis/gas_settings.nr b/noir-projects/noir-protocol-circuits/crates/types/src/abis/gas_settings.nr index 457f1fb5a8af..42301eb096d5 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/abis/gas_settings.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/abis/gas_settings.nr @@ -1,14 +1,14 @@ use crate::{ abis::{gas::Gas, gas_fees::GasFees}, - constants::{DEFAULT_GAS_LIMIT, DEFAULT_TEARDOWN_GAS_LIMIT, GAS_SETTINGS_LENGTH}, + constants::GAS_SETTINGS_LENGTH, traits::{Deserialize, Empty, Serialize}, utils::reader::Reader, }; pub struct GasSettings { - gas_limits: Gas, - teardown_gas_limits: Gas, - max_fees_per_gas: GasFees, + pub gas_limits: Gas, + pub teardown_gas_limits: Gas, + pub max_fees_per_gas: GasFees, } impl GasSettings { diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/abis/mod.nr b/noir-projects/noir-protocol-circuits/crates/types/src/abis/mod.nr index 890d2d4429de..fb8e97d5de9b 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/abis/mod.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/abis/mod.nr @@ -13,6 +13,7 @@ pub mod nullifier_leaf_preimage; pub mod tx_constant_data; pub mod combined_constant_data; +pub mod constant_rollup_data; pub mod side_effect; pub mod read_request; diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/abis/tube.nr b/noir-projects/noir-protocol-circuits/crates/types/src/abis/tube.nr index e36c2ead478d..3638c6bd3683 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/abis/tube.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/abis/tube.nr @@ -25,7 +25,7 @@ impl Verifiable for PublicTubeData { } pub struct PrivateTubeData { - public_inputs: KernelCircuitPublicInputs, + pub public_inputs: KernelCircuitPublicInputs, proof: TubeProof, vk_data: VkData, } diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/tests/fixture_builder.nr b/noir-projects/noir-protocol-circuits/crates/types/src/tests/fixture_builder.nr index 94294b65c160..092911b5934b 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/tests/fixture_builder.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/tests/fixture_builder.nr @@ -4,9 +4,11 @@ use crate::{ avm_accumulated_data::AvmAccumulatedData, CombinedAccumulatedData, PrivateAccumulatedData, PrivateAccumulatedDataBuilder, PrivateToPublicAccumulatedData, }, + append_only_tree_snapshot::AppendOnlyTreeSnapshot, avm_circuit_public_inputs::AvmProofData, call_context::CallContext, combined_constant_data::CombinedConstantData, + constant_rollup_data::ConstantRollupData, function_data::FunctionData, gas::Gas, gas_settings::GasSettings, @@ -159,6 +161,9 @@ pub struct FixtureBuilder { pub protocol_contract_tree_root: Field, pub protocol_contract_sibling_path: [Field; PROTOCOL_CONTRACT_TREE_HEIGHT], + // Tree snapshots. + pub archive_tree: AppendOnlyTreeSnapshot, + // Counters. pub min_revertible_side_effect_counter: u32, pub counter_start: u32, @@ -291,6 +296,15 @@ impl FixtureBuilder { } } + pub fn to_constant_rollup_data(self) -> ConstantRollupData { + ConstantRollupData { + last_archive: self.archive_tree, + vk_tree_root: self.vk_tree_root, + protocol_contract_tree_root: self.protocol_contract_tree_root, + global_variables: self.global_variables, + } + } + pub fn build_tx_request(self) -> TxRequest { TxRequest { origin: self.contract_address, @@ -1047,13 +1061,13 @@ impl FixtureBuilder { fixtures::vk_tree::get_vk_merkle_tree().get_root() } - fn to_private_tube_data(self) -> PrivateTubeData { + pub fn to_private_tube_data(self) -> PrivateTubeData { let mut result: PrivateTubeData = std::mem::zeroed(); result.public_inputs = self.to_kernel_circuit_public_inputs(); result } - fn to_public_tube_data(self) -> PublicTubeData { + pub fn to_public_tube_data(self) -> PublicTubeData { let mut result: PublicTubeData = std::mem::zeroed(); result.public_inputs = self.to_private_to_public_kernel_circuit_public_inputs(true); result @@ -1133,6 +1147,7 @@ impl Empty for FixtureBuilder { vk_tree_root: FixtureBuilder::vk_tree_root(), protocol_contract_tree_root: 0, protocol_contract_sibling_path: [0; PROTOCOL_CONTRACT_TREE_HEIGHT], + archive_tree: AppendOnlyTreeSnapshot::zero(), revert_code: 0, min_revertible_side_effect_counter: 0, counter_start: 0, From 13d85f32aea08a2e7f4db26fa7399e18cac54734 Mon Sep 17 00:00:00 2001 From: Leila Wang Date: Thu, 28 Nov 2024 16:08:28 +0000 Subject: [PATCH 02/10] Filter out txs paying enough fee per gas. --- .../src/tx_validator/gas_validator.test.ts | 26 ++++++++++++++----- .../src/tx_validator/gas_validator.ts | 23 +++++++++++++--- .../src/tx_validator/tx_validator_factory.ts | 7 ++++- 3 files changed, 46 insertions(+), 10 deletions(-) diff --git a/yarn-project/sequencer-client/src/tx_validator/gas_validator.test.ts b/yarn-project/sequencer-client/src/tx_validator/gas_validator.test.ts index 172c7ef67fdf..aa1672c42f92 100644 --- a/yarn-project/sequencer-client/src/tx_validator/gas_validator.test.ts +++ b/yarn-project/sequencer-client/src/tx_validator/gas_validator.test.ts @@ -1,6 +1,7 @@ import { type Tx, mockTx } from '@aztec/circuit-types'; -import { AztecAddress, Fr, FunctionSelector, GasSettings, PUBLIC_DISPATCH_SELECTOR } from '@aztec/circuits.js'; +import { AztecAddress, Fr, FunctionSelector, GasFees, GasSettings, PUBLIC_DISPATCH_SELECTOR } from '@aztec/circuits.js'; import { poseidon2Hash } from '@aztec/foundation/crypto'; +import { type Writeable } from '@aztec/foundation/types'; import { FeeJuiceContract } from '@aztec/noir-contracts.js'; import { ProtocolContractAddress } from '@aztec/protocol-contracts'; @@ -10,17 +11,16 @@ import { GasTxValidator, type PublicStateSource } from './gas_validator.js'; import { patchNonRevertibleFn, patchRevertibleFn } from './test_utils.js'; describe('GasTxValidator', () => { - let validator: GasTxValidator; let publicStateSource: MockProxy; let feeJuiceAddress: AztecAddress; + let enforceFees: boolean; + let gasFees: Writeable; beforeEach(() => { feeJuiceAddress = ProtocolContractAddress.FeeJuice; publicStateSource = mock({ storageRead: mockFn().mockImplementation((_address: AztecAddress, _slot: Fr) => Fr.ZERO), }); - - validator = new GasTxValidator(publicStateSource, feeJuiceAddress, false); }); let tx: Tx; @@ -31,10 +31,12 @@ describe('GasTxValidator', () => { beforeEach(() => { tx = mockTx(1, { numberOfNonRevertiblePublicCallRequests: 2 }); tx.data.feePayer = AztecAddress.random(); - tx.data.constants.txContext.gasSettings = GasSettings.default(); + gasFees = new GasFees(11, 22); + tx.data.constants.txContext.gasSettings = GasSettings.default({ maxFeesPerGas: gasFees }); payer = tx.data.feePayer; expectedBalanceSlot = poseidon2Hash([FeeJuiceContract.storage.balances.slot, payer]); feeLimit = tx.data.constants.txContext.gasSettings.getFeeLimit().toBigInt(); + enforceFees = false; }); const mockBalance = (balance: bigint) => { @@ -44,12 +46,14 @@ describe('GasTxValidator', () => { }; const expectValidateSuccess = async (tx: Tx) => { + const validator = new GasTxValidator(publicStateSource, feeJuiceAddress, enforceFees, gasFees); const result = await validator.validateTxs([tx]); expect(result[0].length).toEqual(1); expect(result).toEqual([[tx], []]); }; const expectValidateFail = async (tx: Tx) => { + const validator = new GasTxValidator(publicStateSource, feeJuiceAddress, enforceFees, gasFees); const result = await validator.validateTxs([tx]); expect(result[1].length).toEqual(1); expect(result).toEqual([[], [tx]]); @@ -96,8 +100,18 @@ describe('GasTxValidator', () => { }); it('rejects txs with no fee payer if fees are enforced', async () => { - validator.enforceFees = true; + enforceFees = true; tx.data.feePayer = AztecAddress.ZERO; await expectValidateFail(tx); }); + + it('rejects txs with not enough fee per da gas', async () => { + gasFees.feePerDaGas = gasFees.feePerDaGas.sub(new Fr(1)); + await expectValidateFail(tx); + }); + + it('rejects txs with not enough fee per l2 gas', async () => { + gasFees.feePerL2Gas = gasFees.feePerL2Gas.sub(new Fr(1)); + await expectValidateFail(tx); + }); }); diff --git a/yarn-project/sequencer-client/src/tx_validator/gas_validator.ts b/yarn-project/sequencer-client/src/tx_validator/gas_validator.ts index e7a47a7eced6..47f45c0fb748 100644 --- a/yarn-project/sequencer-client/src/tx_validator/gas_validator.ts +++ b/yarn-project/sequencer-client/src/tx_validator/gas_validator.ts @@ -1,5 +1,5 @@ import { type Tx, TxExecutionPhase, type TxValidator } from '@aztec/circuit-types'; -import { type AztecAddress, type Fr, FunctionSelector } from '@aztec/circuits.js'; +import { type AztecAddress, type Fr, FunctionSelector, type GasFees } from '@aztec/circuits.js'; import { createDebugLogger } from '@aztec/foundation/log'; import { computeFeePayerBalanceStorageSlot, getExecutionRequestsByPhase } from '@aztec/simulator'; @@ -13,7 +13,12 @@ export class GasTxValidator implements TxValidator { #publicDataSource: PublicStateSource; #feeJuiceAddress: AztecAddress; - constructor(publicDataSource: PublicStateSource, feeJuiceAddress: AztecAddress, public enforceFees: boolean) { + constructor( + publicDataSource: PublicStateSource, + feeJuiceAddress: AztecAddress, + private enforceFees: boolean, + private gasFees: GasFees, + ) { this.#publicDataSource = publicDataSource; this.#feeJuiceAddress = feeJuiceAddress; } @@ -48,8 +53,20 @@ export class GasTxValidator implements TxValidator { } } + const gasSettings = tx.data.constants.txContext.gasSettings; + + // Check that the user is willing to pay enough fee per gas. + const maxFeesPerGas = gasSettings.maxFeesPerGas; + if ( + maxFeesPerGas.feePerDaGas.lt(this.gasFees.feePerDaGas) || + maxFeesPerGas.feePerL2Gas.lt(this.gasFees.feePerL2Gas) + ) { + this.#log.warn(`Rejecting transaction ${tx.getTxHash()} due to insufficient fee per gas`); + return false; + } + // Compute the maximum fee that this tx may pay, based on its gasLimits and maxFeePerGas - const feeLimit = tx.data.constants.txContext.gasSettings.getFeeLimit(); + const feeLimit = gasSettings.getFeeLimit(); // Read current balance of the feePayer const initialBalance = await this.#publicDataSource.storageRead( diff --git a/yarn-project/sequencer-client/src/tx_validator/tx_validator_factory.ts b/yarn-project/sequencer-client/src/tx_validator/tx_validator_factory.ts index d0bab6af43ef..4d69b417c0da 100644 --- a/yarn-project/sequencer-client/src/tx_validator/tx_validator_factory.ts +++ b/yarn-project/sequencer-client/src/tx_validator/tx_validator_factory.ts @@ -45,7 +45,12 @@ export class TxValidatorFactory { new MetadataTxValidator(globalVariables.chainId, globalVariables.blockNumber), new DoubleSpendTxValidator(this.nullifierSource), new PhasesTxValidator(this.contractDataSource, setupAllowList), - new GasTxValidator(this.publicStateSource, ProtocolContractAddress.FeeJuice, this.enforceFees), + new GasTxValidator( + this.publicStateSource, + ProtocolContractAddress.FeeJuice, + this.enforceFees, + globalVariables.gasFees, + ), ); } From 5851b0a93ce9b515f79e88603c7a37455c3e8ef7 Mon Sep 17 00:00:00 2001 From: Leila Wang Date: Thu, 28 Nov 2024 20:26:11 +0000 Subject: [PATCH 03/10] Fix. --- .../src/base/components/public_tube_data_validator.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/public_tube_data_validator.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/public_tube_data_validator.nr index 341d04f424e6..6edeb1ad88b9 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/public_tube_data_validator.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/public_tube_data_validator.nr @@ -11,7 +11,7 @@ impl PublicTubeDataValidator { PublicTubeDataValidator { data } } - pub fn verify_proof(self) { + pub fn verify_proof(self) { if !dep::std::runtime::is_unconstrained() { self.data.verify(); // TODO(#7410) From 9f162cb9601e723c44f0a05b15de279d299a59ab Mon Sep 17 00:00:00 2001 From: Leila Wang Date: Thu, 28 Nov 2024 23:04:07 +0000 Subject: [PATCH 04/10] Fix. --- .../src/base/components/validate_tube_data.nr | 8 ++++---- .../validate_with_rollup_data.nr | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/validate_tube_data.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/validate_tube_data.nr index 5820fcfa953a..1a443ee53494 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/validate_tube_data.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/validate_tube_data.nr @@ -2,11 +2,11 @@ use dep::types::abis::gas_fees::GasFees; pub fn validate_max_fees_per_gas(max_fees_per_gas: GasFees, gas_fees: GasFees) { assert( - !gas_fees.fee_per_da_gas.lt(max_fees_per_gas.fee_per_da_gas), - "fee_per_da_gas in settings must not be less than the global value", + !max_fees_per_gas.fee_per_da_gas.lt(gas_fees.fee_per_da_gas), + "max fee_per_da_gas in settings must not be less than the global value", ); assert( - !gas_fees.fee_per_l2_gas.lt(max_fees_per_gas.fee_per_l2_gas), - "fee_per_l2_gas in settings must not be less than the global value", + !max_fees_per_gas.fee_per_l2_gas.lt(gas_fees.fee_per_l2_gas), + "max fee_per_l2_gas in settings must not be less than the global value", ); } diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/private_tube_data_validator_builder/validate_with_rollup_data.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/private_tube_data_validator_builder/validate_with_rollup_data.nr index e5119533bde4..72bbb8e2d041 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/private_tube_data_validator_builder/validate_with_rollup_data.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/private_tube_data_validator_builder/validate_with_rollup_data.nr @@ -6,22 +6,22 @@ fn validate_with_rollup_data_success() { builder.validate_with_rollup_data(); } -#[test(should_fail_with = "fee_per_da_gas in settings must not be less than the global value")] +#[test(should_fail_with = "max fee_per_da_gas in settings must not be less than the global value")] fn validate_with_rollup_data_not_enough_fee_per_da_gas_fails() { let mut builder = PrivateTubeDataValidatorBuilder::new(); - builder.tube_data.tx_context.gas_settings.max_fees_per_gas.fee_per_da_gas = 4; - builder.rollup_data.global_variables.gas_fees.fee_per_da_gas = 3; + builder.tube_data.tx_context.gas_settings.max_fees_per_gas.fee_per_da_gas = 3; + builder.rollup_data.global_variables.gas_fees.fee_per_da_gas = 4; builder.validate_with_rollup_data(); } -#[test(should_fail_with = "fee_per_l2_gas in settings must not be less than the global value")] +#[test(should_fail_with = "max fee_per_l2_gas in settings must not be less than the global value")] fn validate_with_rollup_data_not_enough_fee_per_l2_gas_fails() { let mut builder = PrivateTubeDataValidatorBuilder::new(); - builder.tube_data.tx_context.gas_settings.max_fees_per_gas.fee_per_l2_gas = 4; - builder.rollup_data.global_variables.gas_fees.fee_per_l2_gas = 3; + builder.tube_data.tx_context.gas_settings.max_fees_per_gas.fee_per_l2_gas = 3; + builder.rollup_data.global_variables.gas_fees.fee_per_l2_gas = 4; builder.validate_with_rollup_data(); } From f27185f9394e8f64307de201220080ee71a63a5a Mon Sep 17 00:00:00 2001 From: Leila Wang Date: Fri, 13 Dec 2024 12:50:01 +0000 Subject: [PATCH 05/10] Skip check for empty tube. --- .../base/components/private_tube_data_validator.nr | 2 +- .../crates/rollup-lib/src/base/private_base_rollup.nr | 11 +++++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/private_tube_data_validator.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/private_tube_data_validator.nr index 64f27feb546a..8027741142fe 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/private_tube_data_validator.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/private_tube_data_validator.nr @@ -15,7 +15,7 @@ impl PrivateTubeDataValidator { if !dep::std::runtime::is_unconstrained() { self.data.verify(); // TODO(#7410) - // self.tube_data.vk_data.validate_in_vk_tree(self.tube_data.public_inputs.constants.vk_tree_root, ALLOWED_PREVIOUS_CIRCUITS); + // self.data.vk_data.validate_in_vk_tree(self.data.public_inputs.constants.vk_tree_root, allowed_previous_circuits); } } diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/private_base_rollup.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/private_base_rollup.nr index bb9c0cf94880..8fd5c1496e5f 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/private_base_rollup.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/private_base_rollup.nr @@ -19,6 +19,7 @@ use dep::types::{ }, constants::{ ARCHIVE_HEIGHT, MAX_TOTAL_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX, NOTE_HASH_SUBTREE_HEIGHT, + TUBE_VK_INDEX, }, data::{hash::compute_public_data_tree_value, public_data_hint::PublicDataHint}, hash::silo_l2_to_l1_message, @@ -30,7 +31,7 @@ use dep::types::{ traits::is_empty, }; -// global ALLOWED_PREVIOUS_CIRCUITS: [u32; 2] = [PRIVATE_KERNEL_EMPTY_INDEX, TUBE_VK_INDEX]; +global ALLOWED_PREVIOUS_CIRCUITS: [u32; 1] = [TUBE_VK_INDEX]; pub struct PrivateBaseRollupInputs { tube_data: PrivateTubeData, @@ -52,9 +53,15 @@ impl PrivateBaseRollupInputs { } pub fn execute(self) -> BaseOrMergeRollupPublicInputs { + // TODO(#10311): There should be an empty base rollup. + // There's at least 1 nullifier for a tx. So gas_used won't be empty if the previous circuit is private_tail. + let is_empty_tube = self.tube_data.public_inputs.gas_used.is_empty(); + let tube_data_validator = PrivateTubeDataValidator::new(self.tube_data); tube_data_validator.verify_proof(ALLOWED_PREVIOUS_CIRCUITS); - tube_data_validator.validate_with_rollup_data(self.constants); + if !is_empty_tube { + tube_data_validator.validate_with_rollup_data(self.constants); + } let transaction_fee = self.compute_transaction_fee(); From 33727501bc9450a50577c2b7fa8bfd3538fcd4a5 Mon Sep 17 00:00:00 2001 From: Leila Wang Date: Fri, 13 Dec 2024 14:45:58 +0000 Subject: [PATCH 06/10] Fix and add tests. --- .../src/base/private_base_rollup.nr | 3 + .../rollup-lib/src/base/public_base_rollup.nr | 10 +-- .../src/abis/avm_circuit_public_inputs.nr | 2 +- .../src/tx/validator/empty_validator.ts | 4 +- .../src/tx/validator/tx_validator.ts | 2 +- .../tx_validator/aggregate_tx_validator.ts | 8 ++- .../src/sequencer/sequencer.test.ts | 44 ++++++++++++ .../src/tx_validator/gas_validator.test.ts | 71 ++++++++++--------- .../src/tx_validator/gas_validator.ts | 47 +++++++----- 9 files changed, 128 insertions(+), 63 deletions(-) diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/private_base_rollup.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/private_base_rollup.nr index 8fd5c1496e5f..b8461ec090b2 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/private_base_rollup.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/private_base_rollup.nr @@ -893,6 +893,7 @@ mod tests { // Set gas builder.tube_data.gas_used = gas_used; + builder.tube_data.tx_context.gas_settings.max_fees_per_gas = gas_fees; builder.constants.global_variables.gas_fees = gas_fees; // Set fee payer @@ -942,6 +943,7 @@ mod tests { // Set gas builder.tube_data.gas_used = gas_used; + builder.tube_data.tx_context.gas_settings.max_fees_per_gas = gas_fees; builder.constants.global_variables.gas_fees = gas_fees; // Set fee payer @@ -972,6 +974,7 @@ mod tests { // Set gas builder.tube_data.gas_used = gas_used; + builder.tube_data.tx_context.gas_settings.max_fees_per_gas = gas_fees; builder.constants.global_variables.gas_fees = gas_fees; // Set fee payer diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/public_base_rollup.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/public_base_rollup.nr index a93daf397ed1..64d16a41c864 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/public_base_rollup.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/public_base_rollup.nr @@ -277,7 +277,11 @@ mod tests { merkle_tree::MembershipWitness, messaging::l2_to_l1_message::ScopedL2ToL1Message, partial_state_reference::PartialStateReference, - tests::{fixture_builder::FixtureBuilder, fixtures, merkle_tree_utils::NonEmptyMerkleTree}, + tests::{ + fixture_builder::FixtureBuilder, + fixtures::{self, merkle_tree::generate_full_sha_tree}, + merkle_tree_utils::NonEmptyMerkleTree, + }, traits::{Empty, is_empty}, utils::{ arrays::get_sorted_tuple::get_sorted_tuple, @@ -827,9 +831,7 @@ mod tests { ); // Since we fill the tree completely, we know to expect a full tree as below - let expected_tree = dep::types::merkle_tree::variable_merkle_tree::tests::generate_full_sha_tree( - siloed_l2_to_l1_msgs.storage(), - ); + let expected_tree = generate_full_sha_tree(siloed_l2_to_l1_msgs.storage()); assert_eq(out_hash, expected_tree.get_root()); } diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/abis/avm_circuit_public_inputs.nr b/noir-projects/noir-protocol-circuits/crates/types/src/abis/avm_circuit_public_inputs.nr index 0352937e504a..88fe770eb0d4 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/abis/avm_circuit_public_inputs.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/abis/avm_circuit_public_inputs.nr @@ -214,7 +214,7 @@ impl AvmProofData { ); let mut result: [Field; 4] = [input_hash, 0, 0, 0]; - for i in 0..DUMMY_AVM_VERIFIER_NUM_ITERATIONS { + for _i in 0..DUMMY_AVM_VERIFIER_NUM_ITERATIONS { result = poseidon2_permutation(result, 4); } diff --git a/yarn-project/circuit-types/src/tx/validator/empty_validator.ts b/yarn-project/circuit-types/src/tx/validator/empty_validator.ts index 7ad6f80dcf1a..2ea10e7a55ab 100644 --- a/yarn-project/circuit-types/src/tx/validator/empty_validator.ts +++ b/yarn-project/circuit-types/src/tx/validator/empty_validator.ts @@ -1,8 +1,8 @@ import { type AnyTx, type TxValidator } from './tx_validator.js'; export class EmptyTxValidator implements TxValidator { - public validateTxs(txs: T[]): Promise<[validTxs: T[], invalidTxs: T[]]> { - return Promise.resolve([txs, []]); + public validateTxs(txs: T[]): Promise<[validTxs: T[], invalidTxs: T[], skippedTxs: T[]]> { + return Promise.resolve([txs, [], []]); } public validateTx(_tx: T): Promise { diff --git a/yarn-project/circuit-types/src/tx/validator/tx_validator.ts b/yarn-project/circuit-types/src/tx/validator/tx_validator.ts index 6669b56055a5..040d764cf3d5 100644 --- a/yarn-project/circuit-types/src/tx/validator/tx_validator.ts +++ b/yarn-project/circuit-types/src/tx/validator/tx_validator.ts @@ -5,5 +5,5 @@ export type AnyTx = Tx | ProcessedTx; export interface TxValidator { validateTx(tx: T): Promise; - validateTxs(txs: T[]): Promise<[validTxs: T[], invalidTxs: T[]]>; + validateTxs(txs: T[]): Promise<[validTxs: T[], invalidTxs: T[], skippedTxs?: T[]]>; } diff --git a/yarn-project/p2p/src/tx_validator/aggregate_tx_validator.ts b/yarn-project/p2p/src/tx_validator/aggregate_tx_validator.ts index 99dfb6c282db..21bf24ddb8db 100644 --- a/yarn-project/p2p/src/tx_validator/aggregate_tx_validator.ts +++ b/yarn-project/p2p/src/tx_validator/aggregate_tx_validator.ts @@ -10,16 +10,18 @@ export class AggregateTxValidator implements TxValid this.#validators = validators; } - async validateTxs(txs: T[]): Promise<[validTxs: T[], invalidTxs: T[]]> { + async validateTxs(txs: T[]): Promise<[validTxs: T[], invalidTxs: T[], skippedTxs: T[]]> { const invalidTxs: T[] = []; + const skippedTxs: T[] = []; let txPool = txs; for (const validator of this.#validators) { - const [valid, invalid] = await validator.validateTxs(txPool); + const [valid, invalid, skipped] = await validator.validateTxs(txPool); invalidTxs.push(...invalid); + skippedTxs.push(...(skipped ?? [])); txPool = valid; } - return [txPool, invalidTxs]; + return [txPool, invalidTxs, skippedTxs]; } async validateTx(tx: T): Promise { diff --git a/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts b/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts index 1807a42a5e1a..3102ec0bbb5f 100644 --- a/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts +++ b/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts @@ -26,6 +26,7 @@ import { EthAddress, Fr, GasFees, + type GasSettings, GlobalVariables, NUMBER_OF_L1_L2_MESSAGES_PER_ROLLUP, } from '@aztec/circuits.js'; @@ -395,6 +396,49 @@ describe('sequencer', () => { expect(publisher.proposeL2Block).toHaveBeenCalledWith(block, getSignatures(), validTxHashes, undefined); }); + it('builds a block out of several txs skipping txs not providing enough fee per gas', async () => { + const gasFees = new GasFees(10, 20); + mockedGlobalVariables.gasFees = gasFees; + + const txs = Array(5) + .fill(0) + .map((_, i) => mockTxForRollup(0x10000 * i)); + + const skippedTxIndexes = [1, 2]; + const validTxHashes: TxHash[] = []; + txs.forEach((tx, i) => { + tx.data.constants.txContext.chainId = chainId; + const maxFeesPerGas: Writeable = gasFees.clone(); + const feeToAdjust = i % 2 ? 'feePerDaGas' : 'feePerL2Gas'; + if (skippedTxIndexes.includes(i)) { + // maxFeesPerGas is less than gasFees. + maxFeesPerGas[feeToAdjust] = maxFeesPerGas[feeToAdjust].sub(new Fr(i + 1)); + } else { + // maxFeesPerGas is greater than or equal to gasFees. + maxFeesPerGas[feeToAdjust] = maxFeesPerGas[feeToAdjust].add(new Fr(i)); + validTxHashes.push(tx.getTxHash()); + } + (tx.data.constants.txContext.gasSettings as Writeable).maxFeesPerGas = maxFeesPerGas; + }); + + p2p.getPendingTxs.mockResolvedValueOnce(txs); + blockBuilder.setBlockCompleted.mockResolvedValue(block); + publisher.proposeL2Block.mockResolvedValueOnce(true); + globalVariableBuilder.buildGlobalVariables.mockResolvedValueOnce(mockedGlobalVariables); + + await sequencer.doRealWork(); + + const includedNumTxs = txs.length - skippedTxIndexes.length; + expect(blockBuilder.startNewBlock).toHaveBeenCalledWith( + includedNumTxs, + mockedGlobalVariables, + Array(NUMBER_OF_L1_L2_MESSAGES_PER_ROLLUP).fill(new Fr(0n)), + ); + expect(publisher.proposeL2Block).toHaveBeenCalledWith(block, getSignatures(), validTxHashes, undefined); + // The txs are not included. But they are not dropped from the pool either. + expect(p2p.deleteTxs).not.toHaveBeenCalled(); + }); + it('builds a block once it reaches the minimum number of transactions', async () => { const txs = times(8, i => { const tx = mockTxForRollup(i * 0x10000); diff --git a/yarn-project/sequencer-client/src/tx_validator/gas_validator.test.ts b/yarn-project/sequencer-client/src/tx_validator/gas_validator.test.ts index aa1672c42f92..eec83ad034f6 100644 --- a/yarn-project/sequencer-client/src/tx_validator/gas_validator.test.ts +++ b/yarn-project/sequencer-client/src/tx_validator/gas_validator.test.ts @@ -11,32 +11,31 @@ import { GasTxValidator, type PublicStateSource } from './gas_validator.js'; import { patchNonRevertibleFn, patchRevertibleFn } from './test_utils.js'; describe('GasTxValidator', () => { + // Vars for validator. let publicStateSource: MockProxy; let feeJuiceAddress: AztecAddress; let enforceFees: boolean; let gasFees: Writeable; - - beforeEach(() => { - feeJuiceAddress = ProtocolContractAddress.FeeJuice; - publicStateSource = mock({ - storageRead: mockFn().mockImplementation((_address: AztecAddress, _slot: Fr) => Fr.ZERO), - }); - }); - + // Vars for tx. let tx: Tx; let payer: AztecAddress; let expectedBalanceSlot: Fr; let feeLimit: bigint; beforeEach(() => { + publicStateSource = mock({ + storageRead: mockFn().mockImplementation((_address: AztecAddress, _slot: Fr) => Fr.ZERO), + }); + feeJuiceAddress = ProtocolContractAddress.FeeJuice; + enforceFees = false; + gasFees = new GasFees(11, 22); + tx = mockTx(1, { numberOfNonRevertiblePublicCallRequests: 2 }); tx.data.feePayer = AztecAddress.random(); - gasFees = new GasFees(11, 22); - tx.data.constants.txContext.gasSettings = GasSettings.default({ maxFeesPerGas: gasFees }); + tx.data.constants.txContext.gasSettings = GasSettings.default({ maxFeesPerGas: gasFees.clone() }); payer = tx.data.feePayer; expectedBalanceSlot = poseidon2Hash([FeeJuiceContract.storage.balances.slot, payer]); feeLimit = tx.data.constants.txContext.gasSettings.getFeeLimit().toBigInt(); - enforceFees = false; }); const mockBalance = (balance: bigint) => { @@ -45,23 +44,29 @@ describe('GasTxValidator', () => { ); }; - const expectValidateSuccess = async (tx: Tx) => { + const validateTx = async (tx: Tx) => { const validator = new GasTxValidator(publicStateSource, feeJuiceAddress, enforceFees, gasFees); - const result = await validator.validateTxs([tx]); - expect(result[0].length).toEqual(1); - expect(result).toEqual([[tx], []]); + return await validator.validateTxs([tx]); }; - const expectValidateFail = async (tx: Tx) => { - const validator = new GasTxValidator(publicStateSource, feeJuiceAddress, enforceFees, gasFees); - const result = await validator.validateTxs([tx]); - expect(result[1].length).toEqual(1); - expect(result).toEqual([[], [tx]]); + const expectValid = async (tx: Tx) => { + const result = await validateTx(tx); + expect(result).toEqual([[tx], [], []]); + }; + + const expectInvalid = async (tx: Tx) => { + const result = await validateTx(tx); + expect(result).toEqual([[], [tx], []]); + }; + + const expectSkipped = async (tx: Tx) => { + const result = await validateTx(tx); + expect(result).toEqual([[], [], [tx]]); }; it('allows fee paying txs if fee payer has enough balance', async () => { mockBalance(feeLimit); - await expectValidateSuccess(tx); + await expectValid(tx); }); it('allows fee paying txs if fee payer claims enough balance during setup', async () => { @@ -73,16 +78,16 @@ describe('GasTxValidator', () => { args: [selector.toField(), payer.toField(), new Fr(1n)], msgSender: ProtocolContractAddress.FeeJuice, }); - await expectValidateSuccess(tx); + await expectValid(tx); }); it('rejects txs if fee payer has not enough balance', async () => { mockBalance(feeLimit - 1n); - await expectValidateFail(tx); + await expectInvalid(tx); }); it('rejects txs if fee payer has zero balance', async () => { - await expectValidateFail(tx); + await expectInvalid(tx); }); it('rejects txs if fee payer claims balance outside setup', async () => { @@ -91,27 +96,27 @@ describe('GasTxValidator', () => { selector: FunctionSelector.fromSignature('_increase_public_balance((Field),Field)'), args: [payer.toField(), new Fr(1n)], }); - await expectValidateFail(tx); + await expectInvalid(tx); }); it('allows txs with no fee payer if fees are not enforced', async () => { tx.data.feePayer = AztecAddress.ZERO; - await expectValidateSuccess(tx); + await expectValid(tx); }); it('rejects txs with no fee payer if fees are enforced', async () => { enforceFees = true; tx.data.feePayer = AztecAddress.ZERO; - await expectValidateFail(tx); + await expectInvalid(tx); }); - it('rejects txs with not enough fee per da gas', async () => { - gasFees.feePerDaGas = gasFees.feePerDaGas.sub(new Fr(1)); - await expectValidateFail(tx); + it('skips txs with not enough fee per da gas', async () => { + gasFees.feePerDaGas = gasFees.feePerDaGas.add(new Fr(1)); + await expectSkipped(tx); }); - it('rejects txs with not enough fee per l2 gas', async () => { - gasFees.feePerL2Gas = gasFees.feePerL2Gas.sub(new Fr(1)); - await expectValidateFail(tx); + it('skips txs with not enough fee per l2 gas', async () => { + gasFees.feePerL2Gas = gasFees.feePerL2Gas.add(new Fr(1)); + await expectSkipped(tx); }); }); diff --git a/yarn-project/sequencer-client/src/tx_validator/gas_validator.ts b/yarn-project/sequencer-client/src/tx_validator/gas_validator.ts index 522629c944a7..1e3f839527d6 100644 --- a/yarn-project/sequencer-client/src/tx_validator/gas_validator.ts +++ b/yarn-project/sequencer-client/src/tx_validator/gas_validator.ts @@ -12,61 +12,70 @@ export class GasTxValidator implements TxValidator { #log = createLogger('sequencer:tx_validator:tx_gas'); #publicDataSource: PublicStateSource; #feeJuiceAddress: AztecAddress; + #enforceFees: boolean; + #gasFees: GasFees; constructor( publicDataSource: PublicStateSource, feeJuiceAddress: AztecAddress, - private enforceFees: boolean, - private gasFees: GasFees, + enforceFees: boolean, + gasFees: GasFees, ) { this.#publicDataSource = publicDataSource; this.#feeJuiceAddress = feeJuiceAddress; + this.#enforceFees = enforceFees; + this.#gasFees = gasFees; } - async validateTxs(txs: Tx[]): Promise<[validTxs: Tx[], invalidTxs: Tx[]]> { + async validateTxs(txs: Tx[]): Promise<[validTxs: Tx[], invalidTxs: Tx[], skippedTxs: Tx[]]> { const validTxs: Tx[] = []; const invalidTxs: Tx[] = []; + const skippedTxs: Tx[] = []; for (const tx of txs) { - if (await this.#validateTxFee(tx)) { + if (this.#shouldSkip(tx)) { + skippedTxs.push(tx); + } else if (await this.#validateTxFee(tx)) { validTxs.push(tx); } else { invalidTxs.push(tx); } } - return [validTxs, invalidTxs]; + return [validTxs, invalidTxs, skippedTxs]; } validateTx(tx: Tx): Promise { return this.#validateTxFee(tx); } + #shouldSkip(tx: Tx): boolean { + const gasSettings = tx.data.constants.txContext.gasSettings; + + // Skip the tx if its max fees are not enough for the current block's gas fees. + const maxFeesPerGas = gasSettings.maxFeesPerGas; + const notEnoughMaxFees = + maxFeesPerGas.feePerDaGas.lt(this.#gasFees.feePerDaGas) || + maxFeesPerGas.feePerL2Gas.lt(this.#gasFees.feePerL2Gas); + if (notEnoughMaxFees) { + this.#log.warn(`Rejecting transaction ${tx.getTxHash()} due to insufficient fee per gas`); + } + return notEnoughMaxFees; + } + async #validateTxFee(tx: Tx): Promise { const feePayer = tx.data.feePayer; // TODO(@spalladino) Eventually remove the is_zero condition as we should always charge fees to every tx if (feePayer.isZero()) { - if (this.enforceFees) { + if (this.#enforceFees) { this.#log.warn(`Rejecting transaction ${tx.getTxHash()} due to missing fee payer`); } else { return true; } } - const gasSettings = tx.data.constants.txContext.gasSettings; - - // Check that the user is willing to pay enough fee per gas. - const maxFeesPerGas = gasSettings.maxFeesPerGas; - if ( - maxFeesPerGas.feePerDaGas.lt(this.gasFees.feePerDaGas) || - maxFeesPerGas.feePerL2Gas.lt(this.gasFees.feePerL2Gas) - ) { - this.#log.warn(`Rejecting transaction ${tx.getTxHash()} due to insufficient fee per gas`); - return false; - } - // Compute the maximum fee that this tx may pay, based on its gasLimits and maxFeePerGas - const feeLimit = gasSettings.getFeeLimit(); + const feeLimit = tx.data.constants.txContext.gasSettings.getFeeLimit(); // Read current balance of the feePayer const initialBalance = await this.#publicDataSource.storageRead( From 08779bd51ee4df98a0d5b70f5e917d720bab552e Mon Sep 17 00:00:00 2001 From: Leila Wang Date: Fri, 13 Dec 2024 16:01:16 +0000 Subject: [PATCH 07/10] Fix test. --- .../aggregate_tx_validator.test.ts | 49 +++++++++++++++---- 1 file changed, 39 insertions(+), 10 deletions(-) diff --git a/yarn-project/p2p/src/tx_validator/aggregate_tx_validator.test.ts b/yarn-project/p2p/src/tx_validator/aggregate_tx_validator.test.ts index c74a0fc5e169..bd3156644456 100644 --- a/yarn-project/p2p/src/tx_validator/aggregate_tx_validator.test.ts +++ b/yarn-project/p2p/src/tx_validator/aggregate_tx_validator.test.ts @@ -6,24 +6,53 @@ describe('AggregateTxValidator', () => { it('allows txs that pass all validation', async () => { const txs = [mockTx(0), mockTx(1), mockTx(2), mockTx(3), mockTx(4)]; const agg = new AggregateTxValidator( - new TxDenyList(txs[0].getTxHash(), txs[1].getTxHash()), - new TxDenyList(txs[2].getTxHash(), txs[3].getTxHash()), + new TxDenyList([txs[0].getTxHash(), txs[1].getTxHash()], []), + new TxDenyList([txs[2].getTxHash(), txs[4].getTxHash()], []), ); - await expect(agg.validateTxs(txs)).resolves.toEqual([[txs[4]], [txs[0], txs[1], txs[2], txs[3]]]); + const validTxs = [txs[3]]; + const invalidTxs = [txs[0], txs[1], txs[2], txs[4]]; + const skippedTxs: AnyTx[] = []; + await expect(agg.validateTxs(txs)).resolves.toEqual([validTxs, invalidTxs, skippedTxs]); + }); + + it('aggregate skipped txs ', async () => { + const txs = [mockTx(0), mockTx(1), mockTx(2), mockTx(3), mockTx(4)]; + const agg = new AggregateTxValidator( + new TxDenyList([txs[0].getTxHash()], []), + new TxDenyList([], [txs[1].getTxHash(), txs[2].getTxHash()]), + new TxDenyList([], [txs[4].getTxHash()]), + ); + + const validTxs = [txs[3]]; + const invalidTxs = [txs[0]]; + const skippedTxs = [txs[1], txs[2], txs[4]]; + await expect(agg.validateTxs(txs)).resolves.toEqual([validTxs, invalidTxs, skippedTxs]); }); class TxDenyList implements TxValidator { denyList: Set; - constructor(...txHashes: TxHash[]) { - this.denyList = new Set(txHashes.map(hash => hash.toString())); + skippedList: Set; + constructor(deniedTxHashes: TxHash[], skippedTxHashes: TxHash[]) { + this.denyList = new Set(deniedTxHashes.map(hash => hash.toString())); + this.skippedList = new Set(skippedTxHashes.map(hash => hash.toString())); } - validateTxs(txs: AnyTx[]): Promise<[AnyTx[], AnyTx[]]> { - return Promise.resolve([ - txs.filter(tx => !this.denyList.has(Tx.getHash(tx).toString())), - txs.filter(tx => this.denyList.has(Tx.getHash(tx).toString())), - ]); + validateTxs(txs: AnyTx[]): Promise<[AnyTx[], AnyTx[], AnyTx[] | undefined]> { + const validTxs: AnyTx[] = []; + const invalidTxs: AnyTx[] = []; + const skippedTxs: AnyTx[] = []; + txs.forEach(tx => { + const txHash = Tx.getHash(tx).toString(); + if (this.skippedList.has(txHash)) { + skippedTxs.push(tx); + } else if (this.denyList.has(txHash)) { + invalidTxs.push(tx); + } else { + validTxs.push(tx); + } + }); + return Promise.resolve([validTxs, invalidTxs, skippedTxs.length ? skippedTxs : undefined]); } validateTx(tx: AnyTx): Promise { From 7a469dd81725e34427253149946c2e9cb9a7b72b Mon Sep 17 00:00:00 2001 From: Leila Wang Date: Mon, 16 Dec 2024 10:53:20 +0000 Subject: [PATCH 08/10] Update error messages. --- .../rollup-lib/src/base/components/validate_tube_data.nr | 4 ++-- .../validate_with_rollup_data.nr | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/validate_tube_data.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/validate_tube_data.nr index 1a443ee53494..3c815f2d2358 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/validate_tube_data.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/validate_tube_data.nr @@ -3,10 +3,10 @@ use dep::types::abis::gas_fees::GasFees; pub fn validate_max_fees_per_gas(max_fees_per_gas: GasFees, gas_fees: GasFees) { assert( !max_fees_per_gas.fee_per_da_gas.lt(gas_fees.fee_per_da_gas), - "max fee_per_da_gas in settings must not be less than the global value", + "da gas is higher than the maximum specified by the tx", ); assert( !max_fees_per_gas.fee_per_l2_gas.lt(gas_fees.fee_per_l2_gas), - "max fee_per_l2_gas in settings must not be less than the global value", + "l2 gas is higher than the maximum specified by the tx", ); } diff --git a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/private_tube_data_validator_builder/validate_with_rollup_data.nr b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/private_tube_data_validator_builder/validate_with_rollup_data.nr index 72bbb8e2d041..64756b1efb99 100644 --- a/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/private_tube_data_validator_builder/validate_with_rollup_data.nr +++ b/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/tests/private_tube_data_validator_builder/validate_with_rollup_data.nr @@ -6,7 +6,7 @@ fn validate_with_rollup_data_success() { builder.validate_with_rollup_data(); } -#[test(should_fail_with = "max fee_per_da_gas in settings must not be less than the global value")] +#[test(should_fail_with = "da gas is higher than the maximum specified by the tx")] fn validate_with_rollup_data_not_enough_fee_per_da_gas_fails() { let mut builder = PrivateTubeDataValidatorBuilder::new(); @@ -16,7 +16,7 @@ fn validate_with_rollup_data_not_enough_fee_per_da_gas_fails() { builder.validate_with_rollup_data(); } -#[test(should_fail_with = "max fee_per_l2_gas in settings must not be less than the global value")] +#[test(should_fail_with = "l2 gas is higher than the maximum specified by the tx")] fn validate_with_rollup_data_not_enough_fee_per_l2_gas_fails() { let mut builder = PrivateTubeDataValidatorBuilder::new(); From b9563de490dd0807fabcae220ae64c2789f6174e Mon Sep 17 00:00:00 2001 From: Leila Wang Date: Mon, 16 Dec 2024 12:35:47 +0000 Subject: [PATCH 09/10] Update warning. --- yarn-project/sequencer-client/src/tx_validator/gas_validator.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/yarn-project/sequencer-client/src/tx_validator/gas_validator.ts b/yarn-project/sequencer-client/src/tx_validator/gas_validator.ts index 1e3f839527d6..8b4167543c9e 100644 --- a/yarn-project/sequencer-client/src/tx_validator/gas_validator.ts +++ b/yarn-project/sequencer-client/src/tx_validator/gas_validator.ts @@ -58,7 +58,7 @@ export class GasTxValidator implements TxValidator { maxFeesPerGas.feePerDaGas.lt(this.#gasFees.feePerDaGas) || maxFeesPerGas.feePerL2Gas.lt(this.#gasFees.feePerL2Gas); if (notEnoughMaxFees) { - this.#log.warn(`Rejecting transaction ${tx.getTxHash()} due to insufficient fee per gas`); + this.#log.warn(`Skipping transaction ${tx.getTxHash()} due to insufficient fee per gas`); } return notEnoughMaxFees; } From 8ff20056aaa2b19987372e659770185c211e58db Mon Sep 17 00:00:00 2001 From: Leila Wang Date: Mon, 16 Dec 2024 13:17:38 +0000 Subject: [PATCH 10/10] Fix test. --- yarn-project/sequencer-client/src/sequencer/sequencer.test.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts b/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts index 2e4ed3bb38d8..72498b7e86cc 100644 --- a/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts +++ b/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts @@ -391,7 +391,7 @@ describe('sequencer', () => { expect(publisher.proposeL2Block).toHaveBeenCalledWith(block, getSignatures(), validTxHashes, undefined); }); - it('builds a block out of several txs skipping txs not providing enough fee per gas', async () => { + it('builds a block out of several txs skipping the ones not providing enough fee per gas', async () => { const gasFees = new GasFees(10, 20); mockedGlobalVariables.gasFees = gasFees; @@ -423,9 +423,7 @@ describe('sequencer', () => { await sequencer.doRealWork(); - const includedNumTxs = txs.length - skippedTxIndexes.length; expect(blockBuilder.startNewBlock).toHaveBeenCalledWith( - includedNumTxs, mockedGlobalVariables, Array(NUMBER_OF_L1_L2_MESSAGES_PER_ROLLUP).fill(new Fr(0n)), );