Skip to content

Commit

Permalink
chore(avm): Zero initialization in avm public inputs and execution te…
Browse files Browse the repository at this point in the history 
…st fixes (#10238)

The uninitialized members of the public inputs created some segmentation
fault in AvmExecutionTests.basicAddReturn and other unit test failures
in AvmExecutionTests.l2GasLeft and AvmExecutionTests.daGasLeft tests.
  • Loading branch information
@jeanmon
jeanmon authored Nov 27, 2024
1 parent 7b2e343 commit 0c7c4c9
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 37 deletions.
3 changes: 3 additions & 0 deletions barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ class AvmExecutionTests : public ::testing::Test {
srs::init_crs_factory("../srs_db/ignition");
public_inputs.gas_settings.gas_limits.l2_gas = DEFAULT_INITIAL_L2_GAS;
public_inputs.gas_settings.gas_limits.da_gas = DEFAULT_INITIAL_DA_GAS;
public_inputs.start_gas_used.l2_gas = 0;
public_inputs.start_gas_used.da_gas = 0;

// These values are magic because of how some tests work! Don't change them
PublicCallRequest dummy_request = {
/* msg_sender */ FF::one(),
Expand Down
5 changes: 4 additions & 1 deletion barretenberg/cpp/src/barretenberg/vm/avm/trace/gas_trace.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,17 @@ void AvmGasTraceBuilder::set_initial_gas(uint32_t l2_gas, uint32_t da_gas)

uint32_t AvmGasTraceBuilder::get_l2_gas_left() const
{
if (gas_trace.size() == 0) {
if (gas_trace.empty()) {
return initial_l2_gas;
}
return gas_trace.back().remaining_l2_gas;
}

uint32_t AvmGasTraceBuilder::get_da_gas_left() const
{
if (gas_trace.empty()) {
return initial_da_gas;
}
return gas_trace.back().remaining_da_gas;
}

Expand Down
72 changes: 36 additions & 36 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/public_inputs.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@
using FF = bb::AvmFlavorSettings::FF;

struct EthAddress {
std::array<uint8_t, 20> value;
std::array<uint8_t, 20> value{};
};

struct Gas {
uint32_t l2_gas;
uint32_t da_gas;
uint32_t l2_gas = 0;
uint32_t da_gas = 0;
};

inline void read(uint8_t const*& it, Gas& gas)
Expand All @@ -23,8 +23,8 @@ inline void read(uint8_t const*& it, Gas& gas)
}

struct GasFees {
FF fee_per_da_gas;
FF fee_per_l2_gas;
FF fee_per_da_gas{};
FF fee_per_l2_gas{};
};

inline void read(uint8_t const*& it, GasFees& gas_fees)
Expand All @@ -50,20 +50,20 @@ inline void read(uint8_t const*& it, GasSettings& gas_settings)

struct GlobalVariables {
/** ChainId for the L2 block. */
FF chain_id;
FF chain_id{};
/** Version for the L2 block. */
FF version;
FF version{};
/** Block number of the L2 block. */
FF block_number;
FF block_number{};
/** Slot number of the L2 block */
FF slot_number;
FF slot_number{};
/** Timestamp of the L2 block. */
FF timestamp;
FF timestamp{};
/** Recipient of block reward */
// This is an eth address so it's actually only 20 bytes
FF coinbase;
FF coinbase{};
/** Address to receive fees. */
FF fee_recipient;
FF fee_recipient{};
/** Global gas prices for this block. */
GasFees gas_fees;
};
Expand All @@ -85,8 +85,8 @@ inline void read(uint8_t const*& it, GlobalVariables& global_variables)
}

struct AppendOnlyTreeSnapshot {
FF root;
uint32_t size;
FF root{};
uint32_t size = 0;
};

inline void read(uint8_t const*& it, AppendOnlyTreeSnapshot& tree_snapshot)
Expand Down Expand Up @@ -116,20 +116,20 @@ struct PublicCallRequest {
/**
* Address of the account which represents the entity who invoked the call.
*/
FF msg_sender;
FF msg_sender{};
/**
* The contract address being called.
*/
FF contract_address;
FF contract_address{};
/**
* Function selector of the function being called.
*/
uint32_t function_selector;
uint32_t function_selector = 0;
/**
* Determines whether the call is modifying state.
*/
bool is_static_call;
FF args_hash;
bool is_static_call = false;
FF args_hash{};
};

inline void read(uint8_t const*& it, PublicCallRequest& public_call_request)
Expand All @@ -143,9 +143,9 @@ inline void read(uint8_t const*& it, PublicCallRequest& public_call_request)
}

struct PrivateToAvmAccumulatedDataArrayLengths {
uint32_t note_hashes;
uint32_t nullifiers;
uint32_t l2_to_l1_msgs;
uint32_t note_hashes = 0;
uint32_t nullifiers = 0;
uint32_t l2_to_l1_msgs = 0;
};

inline void read(uint8_t const*& it, PrivateToAvmAccumulatedDataArrayLengths& lengths)
Expand All @@ -157,8 +157,8 @@ inline void read(uint8_t const*& it, PrivateToAvmAccumulatedDataArrayLengths& le
}

struct ScopedL2ToL1Message {
FF l2_to_l1_message;
FF contract_address;
FF l2_to_l1_message{};
FF contract_address{};
};

inline void read(uint8_t const*& it, ScopedL2ToL1Message& l2_to_l1_message)
Expand All @@ -169,8 +169,8 @@ inline void read(uint8_t const*& it, ScopedL2ToL1Message& l2_to_l1_message)
}

struct PrivateToAvmAccumulatedData {
std::array<FF, MAX_NOTE_HASHES_PER_TX> note_hashes;
std::array<FF, MAX_NULLIFIERS_PER_CALL> nullifiers;
std::array<FF, MAX_NOTE_HASHES_PER_TX> note_hashes{};
std::array<FF, MAX_NULLIFIERS_PER_CALL> nullifiers{};
std::array<ScopedL2ToL1Message, MAX_L2_TO_L1_MSGS_PER_CALL> l2_to_l1_msgs;
};

Expand All @@ -189,9 +189,9 @@ inline void read(uint8_t const*& it, PrivateToAvmAccumulatedData& accumulated_da
}

struct LogHash {
FF value;
FF counter;
FF length;
FF value{};
FF counter{};
FF length{};
};

inline void read(uint8_t const*& it, LogHash& log_hash)
Expand All @@ -204,7 +204,7 @@ inline void read(uint8_t const*& it, LogHash& log_hash)

struct ScopedLogHash {
LogHash log_hash;
FF contract_address;
FF contract_address{};
};

inline void read(uint8_t const*& it, ScopedLogHash& scoped_log_hash)
Expand All @@ -215,8 +215,8 @@ inline void read(uint8_t const*& it, ScopedLogHash& scoped_log_hash)
}

struct PublicDataWrite {
FF leaf_slot;
FF value;
FF leaf_slot{};
FF value{};
};

inline void read(uint8_t const*& it, PublicDataWrite& public_data_write)
Expand All @@ -230,11 +230,11 @@ struct AvmAccumulatedData {
/**
* The note hashes from private combining with those made in the AVM execution.
*/
std::array<FF, MAX_NOTE_HASHES_PER_TX> note_hashes;
std::array<FF, MAX_NOTE_HASHES_PER_TX> note_hashes{};
/**
* The nullifiers from private combining with those made in the AVM execution.
*/
std::array<FF, MAX_NULLIFIERS_PER_CALL> nullifiers;
std::array<FF, MAX_NULLIFIERS_PER_CALL> nullifiers{};
/**
* The L2 to L1 messages from private combining with those made in the AVM execution.
*/
Expand Down Expand Up @@ -285,8 +285,8 @@ class AvmPublicInputs {
TreeSnapshots end_tree_snapshots;
Gas end_gas_used;
AvmAccumulatedData accumulated_data;
FF transaction_fee;
bool reverted;
FF transaction_fee{};
bool reverted = false;

AvmPublicInputs() = default;
static AvmPublicInputs from(const std::vector<uint8_t>& data)
Expand Down

1 comment on commit 0c7c4c9

@AztecBot
Copy link
Collaborator

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'C++ Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.

Benchmark suite Current: 0c7c4c9 Previous: 49b4a6c Ratio
nativeconstruct_proof_ultrahonk_power_of_2/20 5067.496634999998 ms/iter 4719.945643000003 ms/iter 1.07

This comment was automatically generated by workflow using github-action-benchmark.

CC: @ludamad @codygunton

Please sign in to comment.