Skip to content

Commit

Permalink
chore!: replace usage of vector in keccakf1600 input with array (#9350)
Browse files Browse the repository at this point in the history
We're currently using a vector to represent the state array for
keccakf1600 opcodes in brillig. This is unnecessary as it's only defined
for inputs of size 25 so we should use an array.

@dbanks12 This impacts AVM as you're also using a vector here. We should
be able to remove this extra operand from the AVM bytecode but I'm not
sure how to propagate this through the rest of the AVM stack.

---------

Co-authored-by: dbanks12 <david@aztecprotocol.com>
  • Loading branch information
TomAFrench and dbanks12 authored Oct 23, 2024
1 parent a166203 commit cb58490
Show file tree
Hide file tree
Showing 16 changed files with 78 additions and 107 deletions.
12 changes: 5 additions & 7 deletions avm-transpiler/src/transpile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1012,9 +1012,9 @@ fn handle_black_box_function(avm_instrs: &mut Vec<AvmInstruction>, operation: &B
..Default::default()
});
}
BlackBoxOp::Keccakf1600 { message, output } => {
let message_offset = message.pointer.to_usize();
let message_size_offset = message.size.to_usize();
BlackBoxOp::Keccakf1600 { input, output } => {
let input_offset = input.pointer.to_usize();
assert_eq!(input.size, 25, "Keccakf1600 input size must be 25!");
let dest_offset = output.pointer.to_usize();
assert_eq!(output.size, 25, "Keccakf1600 output size must be 25!");

Expand All @@ -1023,14 +1023,12 @@ fn handle_black_box_function(avm_instrs: &mut Vec<AvmInstruction>, operation: &B
indirect: Some(
AddressingModeBuilder::default()
.indirect_operand(&output.pointer)
.indirect_operand(&message.pointer)
.direct_operand(&message.size)
.indirect_operand(&input.pointer)
.build(),
),
operands: vec![
AvmOperand::U16 { value: dest_offset as u16 },
AvmOperand::U16 { value: message_offset as u16 },
AvmOperand::U16 { value: message_size_offset as u16 },
AvmOperand::U16 { value: input_offset as u16 },
],
..Default::default()
});
Expand Down
12 changes: 6 additions & 6 deletions barretenberg/cpp/src/barretenberg/dsl/acir_format/serde/acir.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ struct BlackBoxOp {
};

struct Keccakf1600 {
Program::HeapVector message;
Program::HeapArray input;
Program::HeapArray output;

friend bool operator==(const Keccakf1600&, const Keccakf1600&);
Expand Down Expand Up @@ -424,8 +424,8 @@ struct BlackBoxOp {
};

struct Sha256Compression {
Program::HeapVector input;
Program::HeapVector hash_values;
Program::HeapArray input;
Program::HeapArray hash_values;
Program::HeapArray output;

friend bool operator==(const Sha256Compression&, const Sha256Compression&);
Expand Down Expand Up @@ -4061,7 +4061,7 @@ namespace Program {

inline bool operator==(const BlackBoxOp::Keccakf1600& lhs, const BlackBoxOp::Keccakf1600& rhs)
{
if (!(lhs.message == rhs.message)) {
if (!(lhs.input == rhs.input)) {
return false;
}
if (!(lhs.output == rhs.output)) {
Expand Down Expand Up @@ -4094,7 +4094,7 @@ template <typename Serializer>
void serde::Serializable<Program::BlackBoxOp::Keccakf1600>::serialize(const Program::BlackBoxOp::Keccakf1600& obj,
Serializer& serializer)
{
serde::Serializable<decltype(obj.message)>::serialize(obj.message, serializer);
serde::Serializable<decltype(obj.input)>::serialize(obj.input, serializer);
serde::Serializable<decltype(obj.output)>::serialize(obj.output, serializer);
}

Expand All @@ -4104,7 +4104,7 @@ Program::BlackBoxOp::Keccakf1600 serde::Deserializable<Program::BlackBoxOp::Kecc
Deserializer& deserializer)
{
Program::BlackBoxOp::Keccakf1600 obj;
obj.message = serde::Deserializable<decltype(obj.message)>::deserialize(deserializer);
obj.input = serde::Deserializable<decltype(obj.input)>::deserialize(deserializer);
obj.output = serde::Deserializable<decltype(obj.output)>::deserialize(deserializer);
return obj;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1000,7 +1000,7 @@ TEST_F(AvmExecutionTests, keccakf1600OpCode)

std::string bytecode_preamble;
// Set operations for keccak state
for (uint8_t i = 0; i < 25; i++) {
for (uint8_t i = 0; i < KECCAKF1600_INPUT_SIZE; i++) {
bytecode_preamble += to_hex(OpCode::SET_64) + // opcode SET
"00" // Indirect flag
+ to_hex(AvmMemoryTag::U64) + to_hex<uint64_t>(state[i]) + // val i
Expand All @@ -1012,13 +1012,8 @@ TEST_F(AvmExecutionTests, keccakf1600OpCode)
to_hex(OpCode::SET_8) + // opcode SET for indirect src (input)
"00" // Indirect flag
+ to_hex(AvmMemoryTag::U32) +
"01" // value 1 (i.e. where the src will be read from)
"24" // input_offset 36
+ to_hex(OpCode::SET_8) + //
"00" // Indirect flag
+ to_hex(AvmMemoryTag::U32) +
"19" // value 25 (i.e. where the length parameter is stored)
"25" // input_offset 37
"01" // value 1 (i.e. where the src will be read from)
"24" // input_offset 36
+ to_hex(OpCode::SET_16) + // opcode SET for indirect dst (output)
"00" // Indirect flag
+ to_hex(AvmMemoryTag::U32) +
Expand All @@ -1028,7 +1023,6 @@ TEST_F(AvmExecutionTests, keccakf1600OpCode)
"03" // Indirect flag (first 2 operands indirect)
"0023" // output offset (indirect 35)
"0024" // input offset (indirect 36)
"0025" // length offset 37
+ to_hex(OpCode::RETURN) + // opcode RETURN
"00" // Indirect flag
"0100" // ret offset 256
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ const std::unordered_map<OpCode, std::vector<OperandType>> OPCODE_WIRE_FORMAT =
{ OpCode::POSEIDON2PERM, { OperandType::INDIRECT8, OperandType::UINT16, OperandType::UINT16 } },
{ OpCode::SHA256COMPRESSION,
{ OperandType::INDIRECT8, OperandType::UINT16, OperandType::UINT16, OperandType::UINT16 } },
{ OpCode::KECCAKF1600, { OperandType::INDIRECT8, OperandType::UINT16, OperandType::UINT16, OperandType::UINT16 } },
{ OpCode::KECCAKF1600, { OperandType::INDIRECT8, OperandType::UINT16, OperandType::UINT16 } },
// TEMP ECADD without relative memory
{ OpCode::ECADD,
{ OperandType::INDIRECT16,
Expand Down
3 changes: 1 addition & 2 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -710,8 +710,7 @@ std::vector<Row> Execution::gen_trace(std::vector<Instruction> const& instructio
case OpCode::KECCAKF1600:
trace_builder.op_keccakf1600(std::get<uint8_t>(inst.operands.at(0)),
std::get<uint16_t>(inst.operands.at(1)),
std::get<uint16_t>(inst.operands.at(2)),
std::get<uint16_t>(inst.operands.at(3)));
std::get<uint16_t>(inst.operands.at(2)));

break;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,26 +18,27 @@ void AvmKeccakTraceBuilder::reset()
keccak_trace.shrink_to_fit(); // Reclaim memory.
}

std::array<uint64_t, 25> AvmKeccakTraceBuilder::keccakf1600(uint32_t clk, std::array<uint64_t, 25> input)
std::array<uint64_t, KECCAKF1600_INPUT_SIZE> AvmKeccakTraceBuilder::keccakf1600(
uint32_t clk, std::array<uint64_t, KECCAKF1600_INPUT_SIZE> input)
{
// BB's Eth hash function uses C-style arrays, while we like to use std::array
// We do a few conversions for here but maybe we will update later.
uint64_t state[25] = {};
uint64_t state[KECCAKF1600_INPUT_SIZE] = {};
std::copy(input.begin(), input.end(), state);
std::vector<uint64_t> input_vector(input.begin(), input.end());
// This function mutates state
ethash_keccakf1600(state);
std::array<uint64_t, 25> output = {};
for (size_t i = 0; i < 25; i++) {
std::array<uint64_t, KECCAKF1600_INPUT_SIZE> output = {};
for (size_t i = 0; i < KECCAKF1600_INPUT_SIZE; i++) {
output[i] = state[i];
}
std::vector<uint64_t> output_vector(output.begin(), output.end());
keccak_trace.push_back(KeccakTraceEntry{
.clk = clk,
.input = input_vector,
.output = output_vector,
.input_size = 25,
.output_size = 25,
.input_size = KECCAKF1600_INPUT_SIZE,
.output_size = KECCAKF1600_INPUT_SIZE,
});
return output;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

namespace bb::avm_trace {

constexpr uint32_t KECCAKF1600_INPUT_SIZE = 25;

class AvmKeccakTraceBuilder {
public:
struct KeccakTraceEntry {
Expand All @@ -23,7 +25,8 @@ class AvmKeccakTraceBuilder {
// Finalize the trace
std::vector<KeccakTraceEntry> finalize();

std::array<uint64_t, 25> keccakf1600(uint32_t clk, std::array<uint64_t, 25> input);
std::array<uint64_t, KECCAKF1600_INPUT_SIZE> keccakf1600(uint32_t clk,
std::array<uint64_t, KECCAKF1600_INPUT_SIZE> input);

private:
std::vector<KeccakTraceEntry> keccak_trace;
Expand Down
25 changes: 8 additions & 17 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "barretenberg/vm/avm/trace/fixed_gas.hpp"
#include "barretenberg/vm/avm/trace/fixed_powers.hpp"
#include "barretenberg/vm/avm/trace/gadgets/cmp.hpp"
#include "barretenberg/vm/avm/trace/gadgets/keccak.hpp"
#include "barretenberg/vm/avm/trace/gadgets/slice_trace.hpp"
#include "barretenberg/vm/avm/trace/helper.hpp"
#include "barretenberg/vm/avm/trace/opcode.hpp"
Expand Down Expand Up @@ -2950,27 +2951,17 @@ void AvmTraceBuilder::op_sha256_compression(uint8_t indirect,

/**
* @brief Keccakf1600 with direct or indirect memory access.
* This function temporarily has the same interface as the kecccak opcode for compatibility, when the keccak
* migration is complete (to keccakf1600) We will update this function call as we will not likely need
* input_size_offset
* @param indirect byte encoding information about indirect/direct memory access.
* @param output_offset An index in memory pointing to where the first u64 value of the output array should be
* stored.
* @param input_offset An index in memory pointing to the first u64 value of the input array to be used in the next
* instance of poseidon2 permutation.
* @param input_size offset An index in memory pointing to the size of the input array. Temporary while we maintain
* the same interface as keccak (this is fixed to 25)
* instance of keccakf1600.
*/
void AvmTraceBuilder::op_keccakf1600(uint8_t indirect,
uint32_t output_offset,
uint32_t input_offset,
[[maybe_unused]] uint32_t input_size_offset)
void AvmTraceBuilder::op_keccakf1600(uint8_t indirect, uint32_t output_offset, uint32_t input_offset)
{
// What happens if the input_size_offset is > 25 when the state is more that that?
auto clk = static_cast<uint32_t>(main_trace.size()) + 1;
auto [resolved_output_offset, resolved_input_offset, _] =
Addressing<3>::fromWire(indirect, call_ptr)
.resolve({ output_offset, input_offset, input_size_offset }, mem_trace_builder);
auto [resolved_output_offset, resolved_input_offset] =
Addressing<2>::fromWire(indirect, call_ptr).resolve({ output_offset, input_offset }, mem_trace_builder);
auto input_read = constrained_read_from_memory(
call_ptr, clk, resolved_input_offset, AvmMemoryTag::U64, AvmMemoryTag::FF, IntermRegister::IA);
auto output_read = constrained_read_from_memory(
Expand Down Expand Up @@ -3002,13 +2993,13 @@ void AvmTraceBuilder::op_keccakf1600(uint8_t indirect,
// Array input is fixed to 1600 bits
std::vector<uint64_t> input_vec;
// Read results are written to input array
read_slice_from_memory<uint64_t>(resolved_input_offset, 25, input_vec);
std::array<uint64_t, 25> input = vec_to_arr<uint64_t, 25>(input_vec);
read_slice_from_memory<uint64_t>(resolved_input_offset, KECCAKF1600_INPUT_SIZE, input_vec);
std::array<uint64_t, KECCAKF1600_INPUT_SIZE> input = vec_to_arr<uint64_t, KECCAKF1600_INPUT_SIZE>(input_vec);

// Now that we have read all the values, we can perform the operation to get the resulting witness.
// Note: We use the keccak_op_clk to ensure that the keccakf1600 operation is performed at the same clock cycle
// as the main trace that has the selector
std::array<uint64_t, 25> result = keccak_trace_builder.keccakf1600(clk, input);
std::array<uint64_t, KECCAKF1600_INPUT_SIZE> result = keccak_trace_builder.keccakf1600(clk, input);
// Write the result to memory after
write_slice_to_memory(resolved_output_offset, AvmMemoryTag::U64, result);
}
Expand Down
3 changes: 1 addition & 2 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ class AvmTraceBuilder {
std::vector<FF> op_revert(uint8_t indirect, uint32_t ret_offset, uint32_t ret_size);

// Gadgets
void op_keccak(uint8_t indirect, uint32_t output_offset, uint32_t input_offset, uint32_t input_size_offset);
void op_poseidon2_permutation(uint8_t indirect, uint32_t input_offset, uint32_t output_offset);
void op_ec_add(uint16_t indirect,
uint32_t lhs_x_offset,
Expand All @@ -167,7 +166,7 @@ class AvmTraceBuilder {

// Future Gadgets -- pending changes in noir
void op_sha256_compression(uint8_t indirect, uint32_t output_offset, uint32_t state_offset, uint32_t inputs_offset);
void op_keccakf1600(uint8_t indirect, uint32_t output_offset, uint32_t input_offset, uint32_t input_size_offset);
void op_keccakf1600(uint8_t indirect, uint32_t output_offset, uint32_t input_offset);

std::vector<Row> finalize();
void reset();
Expand Down
12 changes: 6 additions & 6 deletions noir/noir-repo/acvm-repo/acir/codegen/acir.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ namespace Program {
};

struct Keccakf1600 {
Program::HeapVector message;
Program::HeapArray input;
Program::HeapArray output;

friend bool operator==(const Keccakf1600&, const Keccakf1600&);
Expand Down Expand Up @@ -424,8 +424,8 @@ namespace Program {
};

struct Sha256Compression {
Program::HeapVector input;
Program::HeapVector hash_values;
Program::HeapArray input;
Program::HeapArray hash_values;
Program::HeapArray output;

friend bool operator==(const Sha256Compression&, const Sha256Compression&);
Expand Down Expand Up @@ -3498,7 +3498,7 @@ Program::BlackBoxOp::Blake3 serde::Deserializable<Program::BlackBoxOp::Blake3>::
namespace Program {

inline bool operator==(const BlackBoxOp::Keccakf1600 &lhs, const BlackBoxOp::Keccakf1600 &rhs) {
if (!(lhs.message == rhs.message)) { return false; }
if (!(lhs.input == rhs.input)) { return false; }
if (!(lhs.output == rhs.output)) { return false; }
return true;
}
Expand All @@ -3523,15 +3523,15 @@ namespace Program {
template <>
template <typename Serializer>
void serde::Serializable<Program::BlackBoxOp::Keccakf1600>::serialize(const Program::BlackBoxOp::Keccakf1600 &obj, Serializer &serializer) {
serde::Serializable<decltype(obj.message)>::serialize(obj.message, serializer);
serde::Serializable<decltype(obj.input)>::serialize(obj.input, serializer);
serde::Serializable<decltype(obj.output)>::serialize(obj.output, serializer);
}

template <>
template <typename Deserializer>
Program::BlackBoxOp::Keccakf1600 serde::Deserializable<Program::BlackBoxOp::Keccakf1600>::deserialize(Deserializer &deserializer) {
Program::BlackBoxOp::Keccakf1600 obj;
obj.message = serde::Deserializable<decltype(obj.message)>::deserialize(deserializer);
obj.input = serde::Deserializable<decltype(obj.input)>::deserialize(deserializer);
obj.output = serde::Deserializable<decltype(obj.output)>::deserialize(deserializer);
return obj;
}
Expand Down
6 changes: 3 additions & 3 deletions noir/noir-repo/acvm-repo/brillig/src/black_box.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ pub enum BlackBoxOp {
},
/// Keccak Permutation function of 1600 width
Keccakf1600 {
message: HeapVector,
input: HeapArray,
output: HeapArray,
},
/// Verifies a ECDSA signature over the secp256k1 curve.
Expand Down Expand Up @@ -102,8 +102,8 @@ pub enum BlackBoxOp {
len: MemoryAddress,
},
Sha256Compression {
input: HeapVector,
hash_values: HeapVector,
input: HeapArray,
hash_values: HeapArray,
output: HeapArray,
},
ToRadix {
Expand Down
8 changes: 4 additions & 4 deletions noir/noir-repo/acvm-repo/brillig_vm/src/black_box.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ pub(crate) fn evaluate_black_box<F: AcirField, Solver: BlackBoxFunctionSolver<F>
memory.write_slice(memory.read_ref(output.pointer), &to_value_vec(&bytes));
Ok(())
}
BlackBoxOp::Keccakf1600 { message, output } => {
let state_vec: Vec<u64> = read_heap_vector(memory, message)
BlackBoxOp::Keccakf1600 { input, output } => {
let state_vec: Vec<u64> = read_heap_array(memory, input)
.iter()
.map(|&memory_value| memory_value.try_into().unwrap())
.collect();
Expand Down Expand Up @@ -292,7 +292,7 @@ pub(crate) fn evaluate_black_box<F: AcirField, Solver: BlackBoxFunctionSolver<F>
}
BlackBoxOp::Sha256Compression { input, hash_values, output } => {
let mut message = [0; 16];
let inputs = read_heap_vector(memory, input);
let inputs = read_heap_array(memory, input);
if inputs.len() != 16 {
return Err(BlackBoxResolutionError::Failed(
BlackBoxFunc::Sha256Compression,
Expand All @@ -303,7 +303,7 @@ pub(crate) fn evaluate_black_box<F: AcirField, Solver: BlackBoxFunctionSolver<F>
message[i] = input.try_into().unwrap();
}
let mut state = [0; 8];
let values = read_heap_vector(memory, hash_values);
let values = read_heap_array(memory, hash_values);
if values.len() != 8 {
return Err(BlackBoxResolutionError::Failed(
BlackBoxFunc::Sha256Compression,
Expand Down
Loading

0 comments on commit cb58490

Please sign in to comment.