diff --git a/barretenberg/cpp/scripts/test_chonk_standalone_vks_havent_changed.sh b/barretenberg/cpp/scripts/test_chonk_standalone_vks_havent_changed.sh index 006aa12d6626..c4f081a01633 100755 --- a/barretenberg/cpp/scripts/test_chonk_standalone_vks_havent_changed.sh +++ b/barretenberg/cpp/scripts/test_chonk_standalone_vks_havent_changed.sh @@ -13,7 +13,7 @@ cd .. # - Generate a hash for versioning: sha256sum bb-chonk-inputs.tar.gz # - Upload the compressed results: aws s3 cp bb-chonk-inputs.tar.gz s3://aztec-ci-artifacts/protocol/bb-chonk-inputs-[hash(0:8)].tar.gz # Note: In case of the "Test suite failed to run ... Unexpected token 'with' " error, need to run: docker pull aztecprotocol/build:3.0 -pinned_short_hash="53ce2d4f" +pinned_short_hash="189f0026" pinned_chonk_inputs_url="https://aztec-ci-artifacts.s3.us-east-2.amazonaws.com/protocol/bb-chonk-inputs-${pinned_short_hash}.tar.gz" script_path="$(cd "$(dirname "${BASH_SOURCE[0]}")/scripts" && pwd)/$(basename "${BASH_SOURCE[0]}")" @@ -85,13 +85,13 @@ function prove_and_verify_inputs { echo "Running proof test for $1..." $bb prove --scheme chonk --ivc_inputs_path "$flow_folder/ivc-inputs.msgpack" > /dev/null 2>&1 || prove_exit_code=$? - # if [[ $proof_exit_code -ne 0 ]]; then + if [[ $proof_exit_code -ne 0 ]]; then echo "Proof test failed for flow $1. Please re-run the script with flag --update_inputs." cp "$flow_folder/ivc-inputs.msgpack" "$root/yarn-project/end-to-end/example-app-ivc-inputs-out/$1/ivc-inputs.msgpack" echo "Inputs copied in yarn-project for debugging" exit 1 - # fi + fi } export -f prove_and_verify_inputs diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/ecdsa_constraints.test.cpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/ecdsa_constraints.test.cpp index ce0ceb8a5bc8..dcefaf4429d7 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_format/ecdsa_constraints.test.cpp +++ b/barretenberg/cpp/src/barretenberg/dsl/acir_format/ecdsa_constraints.test.cpp @@ -58,11 +58,13 @@ template class EcdsaTestingFunctions { WitnessVector witness_values, const InvalidWitness::Target& invalid_witness_target) { - // For most ECDSA invalidation cases, we set result=0 to ensure that the failure mode caught by the test is - // specific to the particular case being tested, not just simple verification failure. - // For the "Result" case we test the mismatch between actual and claimed result. - if (invalid_witness_target != InvalidWitness::Target::None && - invalid_witness_target != InvalidWitness::Target::Result) { + + // The ECDSA verification algorithm never makes the circuit fail, it just returns a boolean bearing witness to + // whether the verification succeeded or not. The only exception is HashIsNotAByteArray, in which case the + // byte_array constructors raises an error. To ensure that the failure mode caught by the test is specific to + // the particular case being tested, not just simple verification failure, we set the verification result to + // false for HashIsNotAByteArray and to true for every other case. + if (invalid_witness_target == InvalidWitness::Target::HashIsNotAByteArray) { witness_values[ecdsa_constraints.result] = bb::fr(0); } @@ -98,7 +100,6 @@ template class EcdsaTestingFunctions { case InvalidWitness::Target::Result: // Test enforcement of verification result: tamper signature but claim it's valid witness_values[ecdsa_constraints.signature[31]] = bb::fr(0); - witness_values[ecdsa_constraints.result] = bb::fr(1); break; case InvalidWitness::Target::None: break; diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/gate_count_constants.hpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/gate_count_constants.hpp index e16e1e89a41d..2e9442c984dc 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_format/gate_count_constants.hpp +++ b/barretenberg/cpp/src/barretenberg/dsl/acir_format/gate_count_constants.hpp @@ -34,9 +34,9 @@ template inline constexpr size_t AES128_ENCRYPTION = 1559 + Z // overlap with the values added for ECCVM. secp256k1 uses table of size 16 whose indices contain all the 4 values // set for ECCVM (hence the same value for Ultra and Mega builders). secp256r1 uses ROM tables of size 4, which // contain only 2 of the values set for ECCVM (hence the difference of two gates between Ultra and Mega builders). -template inline constexpr size_t ECDSA_SECP256K1 = 42541 + ZERO_GATE; +template inline constexpr size_t ECDSA_SECP256K1 = 42839 + ZERO_GATE; template -inline constexpr size_t ECDSA_SECP256R1 = 72313 + ZERO_GATE + (IsMegaBuilder ? 2 : 0); +inline constexpr size_t ECDSA_SECP256R1 = 72614 + ZERO_GATE + (IsMegaBuilder ? 2 : 0); template inline constexpr size_t BLAKE2S = 2952 + ZERO_GATE + MEGA_OFFSET; template inline constexpr size_t BLAKE3 = 2158 + ZERO_GATE + MEGA_OFFSET; @@ -45,7 +45,7 @@ template inline constexpr size_t POSEIDON2_PERMUTATION = 73 + template inline constexpr size_t MULTI_SCALAR_MUL = 3559 + ZERO_GATE; template inline constexpr size_t EC_ADD = 80 + ZERO_GATE + MEGA_OFFSET; template inline constexpr size_t BLOCK_ROM_READ = 9 + ZERO_GATE + MEGA_OFFSET; -template inline constexpr size_t BLOCK_RAM_READ = 18 + ZERO_GATE + MEGA_OFFSET; +template inline constexpr size_t BLOCK_RAM_READ = 9 + ZERO_GATE + MEGA_OFFSET; template inline constexpr size_t BLOCK_RAM_WRITE = 18 + ZERO_GATE + MEGA_OFFSET; template inline constexpr size_t BLOCK_CALLDATA = 1 + ZERO_GATE + MEGA_OFFSET; template inline constexpr size_t BLOCK_RETURNDATA = 11 + ZERO_GATE + MEGA_OFFSET; diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/opcode_gate_count.test.cpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/opcode_gate_count.test.cpp index 279b34ee4253..03661792e467 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_format/opcode_gate_count.test.cpp +++ b/barretenberg/cpp/src/barretenberg/dsl/acir_format/opcode_gate_count.test.cpp @@ -8,6 +8,7 @@ #include "barretenberg/dsl/acir_format/gate_count_constants.hpp" #include "barretenberg/dsl/acir_format/utils.hpp" #include "barretenberg/op_queue/ecc_op_queue.hpp" +#include "test_class.hpp" #include "barretenberg/serialize/test_helper.hpp" @@ -65,13 +66,9 @@ TYPED_TEST(OpcodeGateCountTests, Quad) WitnessVector witness(4, 0); - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 2, - .public_inputs = {}, - .quad_constraints = { quad, quad }, // Test that gate counting works for multiple constraints - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .quad_constraints = { 0, 1 } }, - }; + // Test that gate counting works for multiple constraints + std::vector> constraints = { quad, quad }; + AcirFormat constraint_system = constraint_to_acir_format(constraints); AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -94,13 +91,7 @@ TYPED_TEST(OpcodeGateCountTests, LogicXor32) WitnessVector witness{ 5, 10, 15 }; - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, - .public_inputs = {}, - .logic_constraints = { logic_constraint }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .logic_constraints = { 0 } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(logic_constraint); AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -125,13 +116,7 @@ TYPED_TEST(OpcodeGateCountTests, LogicAnd32) WitnessVector witness{ 5, 10, 0 }; - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, - .public_inputs = {}, - .logic_constraints = { logic_constraint }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .logic_constraints = { 0 } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(logic_constraint); AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -153,13 +138,7 @@ TYPED_TEST(OpcodeGateCountTests, Range32) WitnessVector witness{ 100 }; - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, - .public_inputs = {}, - .range_constraints = { range_constraint }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .range_constraints = { 0 } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(range_constraint); AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -187,13 +166,7 @@ TYPED_TEST(OpcodeGateCountTests, KeccakPermutation) witness.emplace_back(state); } - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, - .public_inputs = {}, - .keccak_permutations = { keccak_permutation }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .keccak_permutations = { 0 } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(keccak_permutation); AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -211,18 +184,12 @@ TYPED_TEST(OpcodeGateCountTests, Poseidon2Permutation) for (size_t idx = 0; idx < 4; idx++) { poseidon2_constraint.state.emplace_back(WitnessOrConstant::from_index(static_cast(idx))); - poseidon2_constraint.result.emplace_back(static_cast(idx) + 5); + poseidon2_constraint.result.emplace_back(static_cast(idx) + 4); } WitnessVector witness(8, 0); - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, - .public_inputs = {}, - .poseidon2_constraints = { poseidon2_constraint }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .poseidon2_constraints = { 0 } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(poseidon2_constraint); AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -247,13 +214,7 @@ TYPED_TEST(OpcodeGateCountTests, Sha256Compression) WitnessVector witness(32, 0); - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, - .public_inputs = {}, - .sha256_compression = { sha256_compression }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .sha256_compression = { 0 } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(sha256_compression); AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -285,13 +246,7 @@ TYPED_TEST(OpcodeGateCountTests, Aes128Encryption) WitnessVector witness(64, fr(0)); - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, - .public_inputs = {}, - .aes128_constraints = { aes128_constraint }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .aes128_constraints = { 0 } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(aes128_constraint); AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -322,7 +277,7 @@ TYPED_TEST(OpcodeGateCountTests, EcdsaSecp256k1) ecdsa_constraint.predicate = WitnessOrConstant::from_index(static_cast(160)); ecdsa_constraint.result = static_cast(161); - WitnessVector witness(163, fr(0)); + WitnessVector witness(162, fr(0)); // Override public key values to avoid failures auto point = bb::curve::SECP256K1::AffineElement::one(); auto x_buffer = point.x.to_buffer(); @@ -332,13 +287,7 @@ TYPED_TEST(OpcodeGateCountTests, EcdsaSecp256k1) witness[idx + 128] = y_buffer[idx]; } - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, - .public_inputs = {}, - .ecdsa_k1_constraints = { ecdsa_constraint }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .ecdsa_k1_constraints = { 0 } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(ecdsa_constraint); AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -369,7 +318,7 @@ TYPED_TEST(OpcodeGateCountTests, EcdsaSecp256r1) ecdsa_constraint.predicate = WitnessOrConstant::from_index(static_cast(160)); ecdsa_constraint.result = static_cast(161); - WitnessVector witness(163, fr(0)); + WitnessVector witness(162, fr(0)); // Override public key values to avoid failures auto point = bb::curve::SECP256K1::AffineElement::one(); auto x_buffer = point.x.to_buffer(); @@ -379,13 +328,7 @@ TYPED_TEST(OpcodeGateCountTests, EcdsaSecp256r1) witness[idx + 128] = y_buffer[idx]; } - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, - .public_inputs = {}, - .ecdsa_r1_constraints = { ecdsa_constraint }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .ecdsa_r1_constraints = { 0 } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(ecdsa_constraint); AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -406,13 +349,7 @@ TYPED_TEST(OpcodeGateCountTests, Blake2s) WitnessVector witness(33, fr(0)); - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, - .public_inputs = {}, - .blake2s_constraints = { blake2s_constraint }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .blake2s_constraints = { 0 } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(blake2s_constraint); AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -433,13 +370,7 @@ TYPED_TEST(OpcodeGateCountTests, Blake3) WitnessVector witness(33, fr(0)); - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, - .public_inputs = {}, - .blake3_constraints = { blake3_constraint }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .blake3_constraints = { 0 } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(blake3_constraint); AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -480,13 +411,7 @@ TYPED_TEST(OpcodeGateCountTests, MultiScalarMul) witness[7] = point.y; witness[8] = fr(0); - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, - .public_inputs = {}, - .multi_scalar_mul_constraints = { msm_constraint }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .multi_scalar_mul_constraints = { 0 } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(msm_constraint); AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -530,13 +455,7 @@ TYPED_TEST(OpcodeGateCountTests, EcAdd) witness[8] = point1.y; witness[9] = fr(0); - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, - .public_inputs = {}, - .ec_add_constraints = { ec_add_constraint }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .ec_add_constraints = { 0 } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(ec_add_constraint); AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -568,14 +487,11 @@ TYPED_TEST(OpcodeGateCountTests, BlockRomRead) .calldata_id = CallDataType::None, }; - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, // The block constraint creates 2 opcodes, but the first one doesn't add any gate, so we - // set num_acir_opcodes to 1 to track only the contribution from the second one - .public_inputs = {}, - .block_constraints = { block_constraint }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .block_constraints = { { 0 } } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(block_constraint); + // The block constraint creates 2 opcodes (MemoryInit + MemoryOp), but MemoryInit doesn't add gates, so we + // adjust num_acir_opcodes to track only the MemoryOp gates + constraint_system.num_acir_opcodes = 1; + constraint_system.original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .block_constraints = { { 0 } } }; AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -607,19 +523,18 @@ TYPED_TEST(OpcodeGateCountTests, BlockRamRead) .calldata_id = CallDataType::None, }; - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, // The block constraint creates 2 opcodes, but the first one doesn't add any gate, so we - // set num_acir_opcodes to 1 to track only the contribution from the second one - .public_inputs = {}, - .block_constraints = { block_constraint }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .block_constraints = { { 0 } } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(block_constraint); + // The block constraint creates 2 opcodes (MemoryInit + MemoryOp), but MemoryInit doesn't add gates, so we + // adjust num_acir_opcodes to track only the MemoryOp gates + constraint_system.num_acir_opcodes = 1; + constraint_system.original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .block_constraints = { { 0 } } }; AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; auto builder = create_circuit(program, metadata); -}; + + EXPECT_EQ(program.constraints.gates_per_opcode, std::vector({ BLOCK_RAM_READ })); +} TYPED_TEST(OpcodeGateCountTests, BlockRamWrite) { @@ -644,14 +559,11 @@ TYPED_TEST(OpcodeGateCountTests, BlockRamWrite) .calldata_id = CallDataType::None, }; - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, // The block constraint creates 2 opcodes, but the first one doesn't add any gate, so we - // set num_acir_opcodes to 1 to track only the contribution from the second one - .public_inputs = {}, - .block_constraints = { block_constraint }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .block_constraints = { { 0 } } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(block_constraint); + // The block constraint creates 2 opcodes (MemoryInit + MemoryOp), but MemoryInit doesn't add gates, so we + // adjust num_acir_opcodes to track only the MemoryOp gates + constraint_system.num_acir_opcodes = 1; + constraint_system.original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .block_constraints = { { 0 } } }; AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -689,14 +601,11 @@ TYPED_TEST(OpcodeGateCountTests, BlockCallData) .calldata_id = CallDataType::Primary, }; - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, // The block constraint creates 2 opcodes, but the first one doesn't add any gate, so - // we set num_acir_opcodes to 1 to track only the contribution from the second one - .public_inputs = {}, - .block_constraints = { block_constraint }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .block_constraints = { { 0 } } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(block_constraint); + // The block constraint creates 2 opcodes (MemoryInit + MemoryOp), but MemoryInit doesn't add gates, so we + // adjust num_acir_opcodes to track only the MemoryOp gates + constraint_system.num_acir_opcodes = 1; + constraint_system.original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .block_constraints = { { 0 } } }; AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -714,14 +623,11 @@ TYPED_TEST(OpcodeGateCountTests, BlockCallData) .calldata_id = CallDataType::Secondary, }; - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, // The block constraint creates 2 opcodes, but the first one doesn't add any gate, so - // we set num_acir_opcodes to 1 to track only the contribution from the second one - .public_inputs = {}, - .block_constraints = { block_constraint }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .block_constraints = { { 0 } } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(block_constraint); + // The block constraint creates 2 opcodes (MemoryInit + MemoryOp), but MemoryInit doesn't add gates, so we + // adjust num_acir_opcodes to track only the MemoryOp gates + constraint_system.num_acir_opcodes = 1; + constraint_system.original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .block_constraints = { { 0 } } }; AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ .collect_gates_per_opcode = true }; @@ -751,14 +657,11 @@ TYPED_TEST(OpcodeGateCountTests, BlockReturnData) .calldata_id = CallDataType::None, }; - AcirFormat constraint_system{ - .max_witness_index = static_cast(witness.size()) - 1, - .num_acir_opcodes = 1, // The block constraint creates 2 opcodes, but the first one doesn't add any gate, so we - // set num_acir_opcodes to 1 to track only the contribution from the second one - .public_inputs = {}, - .block_constraints = { block_constraint }, - .original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .block_constraints = { { 0 } } }, - }; + AcirFormat constraint_system = constraint_to_acir_format(block_constraint); + // The block constraint creates 2 opcodes (MemoryInit + MemoryOp), but MemoryInit doesn't add gates, so we + // adjust num_acir_opcodes to track only the MemoryOp gates + constraint_system.num_acir_opcodes = 1; + constraint_system.original_opcode_indices = AcirFormatOriginalOpcodeIndices{ .block_constraints = { { 0 } } }; AcirProgram program{ constraint_system, witness }; const ProgramMetadata metadata{ diff --git a/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa.test.cpp b/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa.test.cpp index 04377546f8ff..f6745cea0b15 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa.test.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa.test.cpp @@ -69,26 +69,20 @@ template class EcdsaTests : public ::testing::Test { return { account, signature }; } - std::string tampering(std::string message_string, - ecdsa_key_pair& account, - ecdsa_signature& signature, - TamperingMode mode) + void tampering(std::string message_string, + ecdsa_key_pair& account, + ecdsa_signature& signature, + TamperingMode mode) { - std::string failure_msg; - switch (mode) { case TamperingMode::XCoordinateOverflow: { // Invalidate the circuit by passing a public key with x >= q // Do nothing here, tampering happens in circuit - failure_msg = "ECDSA input validation: coordinate(s) of the public key bigger than the base field modulus. " - "(x coordinate): hi limb."; break; } case TamperingMode::YCoordinateOverflow: { // Invalidate the circuit by passing a public key with y >= q // Do nothing here, tampering happens in circuit - failure_msg = "ECDSA input validation: coordinate(s) of the public key bigger than the base field modulus. " - "(y coordinate): hi limb."; break; } case TamperingMode::InvalidR: { @@ -113,23 +107,16 @@ template class EcdsaTests : public ::testing::Test { s = -s; FrNative::serialize_to_buffer(s, &signature.s[0]); - failure_msg = - "ECDSA input validation: the s component of the signature is bigger than (Fr::modulus + 1)/2.: " - "hi limb."; // The second part of the message is added by the range constraint break; } case TamperingMode::ZeroR: { // Invalidate signature by setting r to 0 signature.r = std::array{}; - - failure_msg = "ECDSA input validation: the r component of the signature is zero."; break; } case TamperingMode::ZeroS: { // Invalidate signature by setting s to 0 signature.s = std::array{}; - - failure_msg = "ECDSA input validation: the s component of the signature is zero."; break; } case TamperingMode::InfinityScalarMul: { @@ -150,24 +137,18 @@ template class EcdsaTests : public ::testing::Test { // Verify that the result is the point at infinity auto P = G1Native::one * fr_hash + account.public_key * r; BB_ASSERT_EQ(P.is_point_at_infinity(), true); - - failure_msg = "ECDSA validation: the result of the batch multiplication is the point at infinity."; break; } case TamperingMode::InvalidPubKey: { // Invalidate the circuit by passing a public key which is not on the curve account.public_key.x = account.public_key.y; BB_ASSERT_EQ(account.public_key.on_curve(), false); - - failure_msg = "ECDSA input validation: the public key is not a point on the elliptic curve."; break; } case TamperingMode::InfinityPubKey: { // Invalidate the circuit by passing a public key which is not on the curve account.public_key.self_set_infinity(); BB_ASSERT_EQ(account.public_key.is_point_at_infinity(), true); - - failure_msg = "ECDSA input validation: the public key is the point at infinity."; break; } case TamperingMode::None: @@ -193,8 +174,6 @@ template class EcdsaTests : public ::testing::Test { expected, "Signature verification returned a different result from the expected one. If the signature was " "randomly generated, there is a (very) small chance this is not a bug."); - - return failure_msg; } std::pair> create_stdlib_ecdsa_data( @@ -203,29 +182,33 @@ template class EcdsaTests : public ::testing::Test { const ecdsa_signature& signature, const TamperingMode mode) { - std::vector rr(signature.r.begin(), signature.r.end()); - std::vector ss(signature.s.begin(), signature.s.end()); - - stdlib::ecdsa_signature sig{ stdlib::byte_array(&builder, rr), - stdlib::byte_array(&builder, ss) }; - // We construct the point via its x,y-coordinates with an explicit infinity flag. // This avoids: // 1. The on-curve check of G1::from_witness (so we can test invalid points) // 2. The expensive infinity auto-detection of the 2-arg constructor - Fq x = Fq::from_witness(&builder, account.public_key.x); - Fq y = Fq::from_witness(&builder, account.public_key.y); - if (mode == TamperingMode::XCoordinateOverflow || mode == TamperingMode::YCoordinateOverflow) { - // To test the case in which one of the two coordinates is above the modulus of the base field, we need - // to override the limbs of the coordinates - uint256_t max_uint = (static_cast(1) << 256) - 1; - for (size_t idx = 0; idx < 4; idx++) { - builder.set_variable(mode == TamperingMode::XCoordinateOverflow - ? x.binary_basis_limbs[idx].element.get_witness_index() - : y.binary_basis_limbs[idx].element.get_witness_index(), - bb::fr(max_uint.slice(64 * idx, 64 * (idx + 1)))); + Fq x; + Fq y; + if (mode == TamperingMode::XCoordinateOverflow) { + stdlib::byte_array x_bytes = stdlib::byte_array::constant_padding(&builder, /*length*/ 0); + for (size_t idx = 0; idx < 32; idx++) { + stdlib::byte_array to_write(stdlib::field_t(0xff), /*length*/ 1); + x_bytes.write(to_write); } + x = Fq(x_bytes); + y = Fq::from_witness(&builder, account.public_key.y); + } else if (mode == TamperingMode::YCoordinateOverflow) { + stdlib::byte_array y_bytes = stdlib::byte_array::constant_padding(&builder, /*length*/ 0); + for (size_t idx = 0; idx < 32; idx++) { + stdlib::byte_array to_write(stdlib::field_t(0xff), /*length*/ 1); + y_bytes.write(to_write); + } + x = Fq::from_witness(&builder, account.public_key.x); + y = Fq(y_bytes); + } else { + x = Fq::from_witness(&builder, account.public_key.x); + y = Fq::from_witness(&builder, account.public_key.y); } + if (mode == TamperingMode::InfinityPubKey) { // Override coordinates to (0, 0) for infinity x = Fq::from_witness(&builder, FqNative::zero()); @@ -242,18 +225,24 @@ template class EcdsaTests : public ::testing::Test { create_element_with_explicit_infinity( x, y, is_infinity, /*assert_on_curve=*/false); pub_key.set_free_witness_tag(); + BB_ASSERT_EQ(pub_key.is_point_at_infinity().get_value(), account.public_key.is_point_at_infinity()); + + std::vector rr(signature.r.begin(), signature.r.end()); + std::vector ss(signature.s.begin(), signature.s.end()); + + stdlib::ecdsa_signature sig{ stdlib::byte_array(&builder, rr), + stdlib::byte_array(&builder, ss) }; return { pub_key, sig }; } - size_t ecdsa_verification_circuit(Builder& builder, - const stdlib::byte_array& hashed_message, - const ecdsa_key_pair& account, - const ecdsa_signature& signature, - const bool signature_verification_result, - const bool circuit_checker_result, - const std::string failure_msg, - const TamperingMode mode) + void ecdsa_verification_circuit(Builder& builder, + const stdlib::byte_array& hashed_message, + const ecdsa_key_pair& account, + const ecdsa_signature& signature, + const bool signature_verification_result, + const bool expected_circuit_result, + const TamperingMode mode) { auto [public_key, sig] = create_stdlib_ecdsa_data(builder, account, signature, mode); @@ -268,28 +257,15 @@ template class EcdsaTests : public ::testing::Test { // Check native values EXPECT_EQ(signature_result.get_value(), signature_verification_result); - // Log data - size_t finalized_num_gates = builder.get_num_finalized_gates_inefficient(); - info("num gates = ", finalized_num_gates); - benchmark_info(Builder::NAME_STRING, "ECDSA", "Signature Verification Test", "Gate Count", finalized_num_gates); - // Circuit checker bool is_circuit_satisfied = CircuitChecker::check(builder); - EXPECT_EQ(is_circuit_satisfied, circuit_checker_result); - - // Check the error - EXPECT_EQ(builder.err(), failure_msg); - - return finalized_num_gates; + EXPECT_EQ(is_circuit_satisfied, expected_circuit_result); } - size_t test_verify_signature(bool random_signature, TamperingMode mode) + void test_verify_signature(bool random_signature, TamperingMode mode) { // Map tampering mode to signature verification result - bool signature_verification_result = (mode == TamperingMode::None) || (mode == TamperingMode::HighS); - // Map tampering mode to circuit checker result - bool circuit_checker_result = - (mode == TamperingMode::None) || (mode == TamperingMode::InvalidR) || (mode == TamperingMode::InvalidS); + bool signature_verification_result = (mode == TamperingMode::None); std::string message_string = "Goblin"; std::vector message_bytes(message_string.begin(), message_string.end()); @@ -303,21 +279,88 @@ template class EcdsaTests : public ::testing::Test { auto [account, signature] = generate_dummy_ecdsa_data(message_string, /*random_signature=*/random_signature); // Tamper with the signature - std::string failure_msg = tampering(message_string, account, signature, mode); + tampering(message_string, account, signature, mode); // Create ECDSA verification circuit Builder builder; stdlib::byte_array hashed_message(&builder, hashed_message_bytes); // ECDSA verification - return ecdsa_verification_circuit(builder, - hashed_message, - account, - signature, - signature_verification_result, - circuit_checker_result, - failure_msg, - mode); + ecdsa_verification_circuit( + builder, hashed_message, account, signature, signature_verification_result, true, mode); + } + + // Test deterministic failure when P = ±G for secp256r1 + void test_signature_generator(bool is_minus_generator, bool signature_result, bool expected_circuit_result) + { + std::string message_string = "Goblin"; + std::vector message_bytes(message_string.begin(), message_string.end()); + std::array hashed_message_bytes_ = Sha256Hasher::hash(message_bytes); + std::vector hashed_message_bytes; + hashed_message_bytes.reserve(32); + for (auto byte : hashed_message_bytes_) { + hashed_message_bytes.emplace_back(byte); + } + + // Generate a signature with P = ±G + ecdsa_key_pair account; + + account.private_key = is_minus_generator ? FrNative(-1) : FrNative(1); + account.public_key = G1Native::one * account.private_key; + + ecdsa_signature signature = + ecdsa_construct_signature(message_string, account); + + // Create ECDSA verification circuit + Builder builder; + stdlib::byte_array hashed_message(&builder, hashed_message_bytes); + + // ECDSA verification + ecdsa_verification_circuit(builder, + hashed_message, + account, + signature, + signature_result, + expected_circuit_result, + TamperingMode::None); + } + + // This test covers a specific behaviour of the implementation. If either the x or y coordinate of the public key + // are bigger than the base field modulus, the ECDSA verification algorithm substitutes the public key with 2G to + // avoid circuit failures. This means that an attacker could craft a valid signature for 2G and submit such + // signature together with a public key with x coordinate overflowing. The ECDSA equation would then be satisfied + // because of the internal substitution. The attack should not be succesful because the result of the ECDSA + // verification algorithm takes into account whether either the x or y coordinate overflowed. + void test_signature_double_generator() + { + std::string message_string = "Goblin"; + std::vector message_bytes(message_string.begin(), message_string.end()); + std::array hashed_message_bytes_ = Sha256Hasher::hash(message_bytes); + std::vector hashed_message_bytes; + hashed_message_bytes.reserve(32); + for (auto byte : hashed_message_bytes_) { + hashed_message_bytes.emplace_back(byte); + } + + // Generate a signature with P = 2G + ecdsa_key_pair account; + + account.private_key = FrNative(2); + account.public_key = G1Native::one * account.private_key; + + ecdsa_signature signature = + ecdsa_construct_signature(message_string, account); + + // Tamper with the signature by making the x coordinate overflow + tampering(message_string, account, signature, TamperingMode::XCoordinateOverflow); + + // Create ECDSA verification circuit + Builder builder; + stdlib::byte_array hashed_message(&builder, hashed_message_bytes); + + // ECDSA verification + ecdsa_verification_circuit( + builder, hashed_message, account, signature, false, true, TamperingMode::XCoordinateOverflow); } /** @@ -359,7 +402,6 @@ template class EcdsaTests : public ::testing::Test { { r, s, v }, test.is_valid_signature, test.is_circuit_satisfied, - test.failure_msg, TamperingMode::None); } } @@ -379,15 +421,7 @@ TYPED_TEST(EcdsaTests, VerifyRandomSignature) TYPED_TEST(EcdsaTests, VerifySignature) { - using Curve = TypeParam; - - size_t finalized_num_gates = - TestFixture::test_verify_signature(/*random_signature=*/false, TestFixture::TamperingMode::None); - static constexpr size_t NUM_GATES_SECP256K1 = 42284; - static constexpr size_t NUM_GATES_SECP256R1 = IsMegaBuilder ? 72057 : 72055; - BB_ASSERT_EQ(finalized_num_gates, - Curve::type == bb::CurveType::SECP256K1 ? NUM_GATES_SECP256K1 : NUM_GATES_SECP256R1, - "There has been a change in the number of gates for ECDSA verification"); + TestFixture::test_verify_signature(/*random_signature=*/false, TestFixture::TamperingMode::None); } TYPED_TEST(EcdsaTests, XCoordinateOverflow) @@ -461,6 +495,21 @@ TYPED_TEST(EcdsaTests, Wycherproof) } } +TYPED_TEST(EcdsaTests, SignatureDoubleGenerator) +{ + TestFixture::test_signature_double_generator(); +} + +TYPED_TEST(EcdsaTests, SignatureGenerator) +{ + // Circuit works for Secp256k1, fails for Secp256r1 + bool signature_result = (TypeParam::type == bb::CurveType::SECP256K1); + bool expected_circuit_result = (TypeParam::type == bb::CurveType::SECP256K1); + + TestFixture::test_signature_generator(false, signature_result, expected_circuit_result); + TestFixture::test_signature_generator(true, signature_result, expected_circuit_result); +} + TEST(EcdsaTests, Secp256k1PointAtInfinityRegression) { // Disable asserts because native ecdsa verification raises an error if the result of the scalar multiplication is @@ -531,9 +580,8 @@ TEST(EcdsaTests, Secp256k1PointAtInfinityRegression) const bool circuit_valid = CircuitChecker::check(builder); - // Circuit should fail because the result of the scalar multiplication is the point at infinity - ASSERT_FALSE(circuit_valid); - EXPECT_EQ(builder.err(), "ECDSA validation: the result of the batch multiplication is the point at infinity."); + // Circuit should be satisfied but signature should be rejected + ASSERT_TRUE(circuit_valid); } // Both native and stdlib should reject this invalid signature diff --git a/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa_impl.hpp b/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa_impl.hpp index bc987d1a5bf3..bd21cbe0d112 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa_impl.hpp @@ -50,16 +50,8 @@ auto& engine = numeric::get_debug_randomness(); * an in-circuit boolean value which bears witness to whether the signature verification was successfull or not. The * boolean is NOT constrained to be equal to bool_t(true). * - * @note The circuit introduces constraints for the following assertions: - * 1. \f$P = (x,y)\f$, then x < q, y < q - * 2. \f$P\f$ is on the curve - * 3. \f$P\f$ is not the point at infinity - * 4. \f$0 < r < n\f$ - * 5. \f$0 < s < (n+1)/2\f$ - * 6. \f$Q := H(m) s^{-1} G + r s^{-1} P\f$ is not the point at infinity - * Therefore, if the witnesses passed to this function do not satisfy these constraints, the resulting circuit - * will be unsatisfied. If a user wants to use the verification inside a in-circuit branch, then they need to supply - * valid data for \f$P, r, s\f$, even though \f$(r,s)\f$ doesn't need to be a valid signature. + * @note There is one case in which the circuit will be unsatisfiable even if the witness values are correct: for the + * curve Secp256R1 when the public key is plus or minus the generator point. This is due to our usage of lookup tables. * * @tparam Builder * @tparam Curve @@ -76,6 +68,8 @@ bool_t ecdsa_verify_signature(const stdlib::byte_array& hashed const G1& public_key, const ecdsa_signature& sig) { + using bool_ct = stdlib::bool_t; + BB_ASSERT_EQ(Fr::modulus.get_msb() + 1, 256UL, "The implementation assumes that the bit-length of Fr is 256 bits."); // Fetch the context @@ -90,54 +84,63 @@ bool_t ecdsa_verify_signature(const stdlib::byte_array& hashed Fr z(hashed_message); // Step 1. - public_key.assert_coordinates_in_field( - "ECDSA input validation: coordinate(s) of the public key bigger than the base field modulus."); // x < q, y < q + bool_ct is_x_less_than_modulus = public_key.x().is_less_than( + Fq::modulus, "ECDSA input validation: x coordinate of the public key bigger than the base field modulus."); + bool_ct is_y_less_than_modulus = public_key.y().is_less_than( + Fq::modulus, "ECDSA input validation: y coordinate of the public key bigger than the base field modulus."); // Step 2. - public_key.validate_on_curve("ECDSA input validation: the public key is not a point on the elliptic curve."); + bool_ct is_point_at_infinity = public_key.is_point_at_infinity(); // Step 3. - public_key.is_point_at_infinity().assert_equal(bool_t(false), - "ECDSA input validation: the public key is the point at infinity."); + // We conditionally select a public key whose x and y coordinates are smaller than the base field modulus. We need + // to do this to avoid circuit failures in the function validate_on_curve. Note that this doesn't allow any attack + // as the result of the verification takes into account whether the original point coordinates were valid or not. + typename Curve::AffineElementNative native_double_generator(Curve::g1::one + Curve::g1::one); + G1 double_generator(Fq(native_double_generator.x), Fq(native_double_generator.y), /*assert_on_curve=*/false); + G1 corrected_public_key = G1::conditional_assign( + is_point_at_infinity || !is_x_less_than_modulus || !is_y_less_than_modulus, double_generator, public_key); + bool_t is_point_on_curve = + corrected_public_key.validate_on_curve( + "ECDSA input validation: the public key is not a point on the elliptic curve.", false) == Fq::zero(); // Step 4. Fr r(sig.r); - r.assert_is_in_field("ECDSA input validation: the r component of the signature is bigger than the order of the " - "elliptic curve."); // r < n - r.assert_is_not_equal(Fr::zero(), "ECDSA input validation: the r component of the signature is zero."); // 0 < r + bool_ct is_r_in_range = r.is_less_than( + Fr::modulus, "ECDSA input validation: the r component of the signature is bigger than Fr::modulus."); + bool_ct is_r_zero = r == Fr::zero(); // Step 5. Fr s(sig.s); - s.assert_less_than( - (Fr::modulus + 1) / 2, - "ECDSA input validation: the s component of the signature is bigger than (Fr::modulus + 1)/2."); // s < (n+1)/2 - s.assert_is_not_equal(Fr::zero(), "ECDSA input validation: the s component of the signature is zero."); // 0 < s + bool_ct is_s_in_range = + s.is_less_than((Fr::modulus + 1) / 2, + "ECDSA input validation: the s component of the signature is bigger than (Fr::modulus + 1)/2."); + bool_ct is_s_zero = s == Fr::zero(); // Step 6. - Fr u1 = z.div_without_denominator_check(s); - Fr u2 = r.div_without_denominator_check(s); + // We conditionally select a non-zero scalar to perform the verification to avoid circuit failures when s = 0. + Fr corrected_s = Fr::conditional_assign(is_s_zero, Fr::one(), s); + + Fr u1 = z.div_without_denominator_check(corrected_s); + Fr u2 = r.div_without_denominator_check(corrected_s); G1 result; if constexpr (Curve::type == bb::CurveType::SECP256K1) { - result = G1::secp256k1_ecdsa_mul(public_key, u1, u2); + result = G1::secp256k1_ecdsa_mul(corrected_public_key, u1, u2); } else { // This error comes from the lookup tables used in batch_mul. We could get rid of it by setting with_edgecase = // true. However, this would increase the gate count, and it would handle a case that should not appear in // general: someone using plus or minus the generator as a public key. - if ((public_key.get_value().x == Curve::g1::affine_one.x) && (!builder->failed())) { + if ((corrected_public_key.get_value().x == Curve::g1::affine_one.x) && (!builder->failed())) { builder->failure("ECDSA input validation: the public key is equal to plus or minus the generator point."); } - result = G1::batch_mul({ G1::one(builder), public_key }, { u1, u2 }); + result = G1::batch_mul({ G1::one(builder), corrected_public_key }, { u1, u2 }); } // Step 7. - auto result_is_infinity = result.is_point_at_infinity(); - result_is_infinity.assert_equal( - bool_t(false), "ECDSA validation: the result of the batch multiplication is the point at infinity."); + bool_ct result_is_infinity = result.is_point_at_infinity(); // Step 8. - // We reduce result.x() to 2^s, where s is the smallest s.t. 2^s > q. It is cheap in terms of constraints, and - // avoids possible edge cases result.x().reduce_mod_target_modulus(); // Transfer Fq value result.x() to Fr (this is just moving from a C++ class to another) @@ -150,10 +153,11 @@ bool_t ecdsa_verify_signature(const stdlib::byte_array& hashed result_x_mod_r.binary_basis_limbs[idx].maximum_value = result.x().binary_basis_limbs[idx].maximum_value; } - // Check result.x() = r mod n AND result is not point at infinity - bool_t x_matches = result_x_mod_r == r; - bool_t is_not_infinity = !result_is_infinity; - bool_t is_signature_valid = x_matches && is_not_infinity; + // Check result.x() = r mod n AND that no other check failed + bool_ct x_matches = result_x_mod_r == r; + bool_ct is_signature_valid = x_matches && !is_point_at_infinity && !result_is_infinity && is_r_in_range && + !is_r_zero && is_s_in_range && !is_s_zero && is_point_on_curve && + is_x_less_than_modulus && is_y_less_than_modulus; // Logging if (is_signature_valid.get_value()) { diff --git a/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa_tests_data.hpp b/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa_tests_data.hpp index 94390ad23fc4..44d443fa6c32 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa_tests_data.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa_tests_data.hpp @@ -45,11 +45,9 @@ const std::vector secp256k1_tests{ .message = { 0x31, 0x32, 0x33, 0x34, 0x30, 0x30 }, .r = WycherproofSecp256k1::Fr("0x0000000000000000000000000000000000000000000000000000000000000101"), .s = WycherproofSecp256k1::Fr("0xc58b162c58b162c58b162c58b162c58a1b242973853e16db75c8a1a71da4d39d"), - .is_valid_signature = true, - .is_circuit_satisfied = false, + .is_valid_signature = false, + .is_circuit_satisfied = true, .comment = "Arithmetic error, s is larger than (n+1)/2", - .failure_msg = - "ECDSA input validation: the s component of the signature is bigger than (Fr::modulus + 1)/2.: hi limb.", }, WycherproofSecp256k1{ .x = WycherproofSecp256k1::Fq("0xd6ef20be66c893f741a9bf90d9b74675d1c2a31296397acb3ef174fd0b300c65"), @@ -60,7 +58,6 @@ const std::vector secp256k1_tests{ .is_valid_signature = true, .is_circuit_satisfied = true, .comment = "Arithmetic error, r component is small", - .failure_msg = "", }, // Point duplication tests WycherproofSecp256k1{ @@ -72,7 +69,6 @@ const std::vector secp256k1_tests{ .is_valid_signature = false, .is_circuit_satisfied = true, .comment = "Point duplication, public key shares x-coordinates with generator", - .failure_msg = "", }, // Edge case public key tests WycherproofSecp256k1{ @@ -84,7 +80,6 @@ const std::vector secp256k1_tests{ .is_valid_signature = true, .is_circuit_satisfied = true, .comment = "Edge case public key, y coordinate is small", - .failure_msg = "", }, }; @@ -103,7 +98,6 @@ const std::vector secp256r1_tests{ .is_valid_signature = true, .is_circuit_satisfied = true, .comment = "Arithmetic error", - .failure_msg = "", }, // Point duplication test WycherproofSecp256r1{ @@ -116,7 +110,6 @@ const std::vector secp256r1_tests{ .is_circuit_satisfied = false, // When the public key is equal to ±G, the circuit fails because of the generation of lookup tables .comment = "Point duplication, public key shares x-coordinates with generator", - .failure_msg = "ECDSA input validation: the public key is equal to plus or minus the generator point.", }, // Edge case public key test WycherproofSecp256r1{ @@ -128,7 +121,6 @@ const std::vector secp256r1_tests{ .is_valid_signature = true, .is_circuit_satisfied = true, .comment = "Edge case public key, x-coordinate has many trailing zeros", - .failure_msg = "", }, }; } // namespace bb::stdlib diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield.hpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield.hpp index 4a29ac3e648c..b71b4a6a24ca 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield.hpp @@ -580,6 +580,7 @@ template class bigfield { void assert_equal(const bigfield& other, std::string const& msg = "bigfield::assert_equal") const; void assert_is_not_equal(const bigfield& other, std::string const& msg = "bigfield: prime limb diff is zero, but expected non-zero") const; + bool_t is_less_than(const uint256_t& upper_limit, std::string const& msg = "bigfield::is_less_than") const; /** * @brief Reduce the bigfield element modulo the target modulus. diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield.test.cpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield.test.cpp index fc1be38969c2..f1affe2b5a0c 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield.test.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield.test.cpp @@ -2591,3 +2591,37 @@ TYPED_TEST(stdlib_bigfield, assert_zero_if_computed_zero) { TestFixture::test_assert_zero_if_computed_zero(); } + +TYPED_TEST(stdlib_bigfield, less_than_works) +{ + using Builder = TypeParam::field_ct::Builder; + using fq_ct = TypeParam; + using byte_array_ct = stdlib::byte_array; + + Builder builder; + + auto [c_native, c_ct] = TestFixture::get_random_witness(&builder); + BB_ASSERT_LT(static_cast(c_native), fq_ct::modulus); + + // c_ct < modulus + auto is_ok = c_ct.is_less_than(fq_ct::modulus); + is_ok.assert_equal(stdlib::bool_t(true)); + + // c_ct not smaller than itself + auto is_not_ok = c_ct.is_less_than(c_native); + is_not_ok.assert_equal(stdlib::bool_t(false)); + + // c_ct < c_native + 1 (edge case) + auto is_ok_edge_case = c_ct.is_less_than(c_native + 1); + is_ok_edge_case.assert_equal(stdlib::bool_t(true)); + + // c_ct > modulus fails comparison but doesn't make the circuit fail + std::vector c_bytes(32, 0xff); + byte_array_ct c_byte_array = byte_array_ct(&builder, c_bytes); + fq_ct reconstructed_from_bytes(c_byte_array); + auto is_not_ok_larger_than_modulus = reconstructed_from_bytes.is_less_than(fq_ct::modulus); + is_not_ok_larger_than_modulus.assert_equal(stdlib::bool_t(false)); + + EXPECT_TRUE(CircuitChecker::check(builder)); + EXPECT_FALSE(builder.failed()); +} diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield_impl.hpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield_impl.hpp index 031eed92f7c1..14395471c625 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield_impl.hpp @@ -1862,6 +1862,56 @@ void bigfield::assert_less_than(const uint256_t& upper_limit, std::s unsafe_assert_less_than(upper_limit, is_default_msg ? "bigfield::unsafe_assert_less_than" : msg); } +// Return (a < b) as bool circuit type. +template +bool_t bigfield::is_less_than(const uint256_t& upper_limit, std::string const& msg) const +{ + bool is_default_msg = msg == "bigfield::is_less_than"; + + Builder* ctx = get_context(); + + // Range constraint the limbs, this is required by the ranged_less_than function + ctx->range_constrain_two_limbs(binary_basis_limbs[0].element.get_witness_index(), + binary_basis_limbs[1].element.get_witness_index(), + static_cast(NUM_LIMB_BITS), + static_cast(NUM_LIMB_BITS), + is_default_msg ? "bigfield::is_less_than: limb 0 or 1 too large" : msg); + + ctx->range_constrain_two_limbs(binary_basis_limbs[2].element.get_witness_index(), + binary_basis_limbs[3].element.get_witness_index(), + static_cast(NUM_LIMB_BITS), + static_cast(NUM_LIMB_BITS), + is_default_msg ? "bigfield::is_less_than: limb 2 or 3 too large" : msg); + + const uint256_t upper_limit_value_0 = upper_limit.slice(0, NUM_LIMB_BITS); + const uint256_t upper_limit_value_1 = upper_limit.slice(NUM_LIMB_BITS, NUM_LIMB_BITS * 2); + const uint256_t upper_limit_value_2 = upper_limit.slice(NUM_LIMB_BITS * 2, NUM_LIMB_BITS * 3); + const uint256_t upper_limit_value_3 = upper_limit.slice(NUM_LIMB_BITS * 3, NUM_LIMB_BITS * 4); + + bool_t third_limb_is_smaller = + binary_basis_limbs[3].element.template ranged_less_than(field_t(upper_limit_value_3)); + bool_t third_limb_is_equal = binary_basis_limbs[3].element == field_t(upper_limit_value_3); + + bool_t second_limb_is_smaller = + binary_basis_limbs[2].element.template ranged_less_than(field_t(upper_limit_value_2)); + bool_t second_limb_is_equal = binary_basis_limbs[2].element == field_t(upper_limit_value_2); + + bool_t first_limb_is_smaller = + binary_basis_limbs[1].element.template ranged_less_than(field_t(upper_limit_value_1)); + bool_t first_limb_is_equal = binary_basis_limbs[1].element == field_t(upper_limit_value_1); + + bool_t zeroth_limb_is_smaller = + binary_basis_limbs[0].element.template ranged_less_than(field_t(upper_limit_value_0)); + + // Limb comparison: we start from the most-significant limb and proceed to the least-significant limb + bool_t result = + third_limb_is_smaller || (third_limb_is_equal && second_limb_is_smaller) || + (third_limb_is_equal && second_limb_is_equal && first_limb_is_smaller) || + (third_limb_is_equal && second_limb_is_equal && first_limb_is_equal && zeroth_limb_is_smaller); + + return result; +} + // Reduces the element mod p. This is a strict reduction mod p, so the output is guaranteed to be < p. template void bigfield::reduce_mod_target_modulus() const { diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp index 0caccd17affe..06fa137ba701 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp @@ -99,12 +99,12 @@ template class element { /** * @brief Check that the point is on the curve */ - void validate_on_curve(std::string const& msg = "biggroup::validate_on_curve") const + Fq validate_on_curve(std::string const& msg = "biggroup::validate_on_curve", bool assert_is_on_curve = true) const { // Return early for constant inputs if (this->is_constant()) { BB_ASSERT(this->get_value().on_curve(), "biggroup::validate_on_curve: constant point not on curve"); - return; + return typename Fq::native(this->get_value().on_curve() ? 0 : 1); } // If this is a point at infinity, it must have (x, y) = (0, 0) @@ -112,23 +112,27 @@ template class element { _y.assert_zero_if(is_point_at_infinity(), "biggroup::validate_on_curve: infinity point must have y = 0"); bool has_circuit_failed = get_context()->failed(); + Fq result; Fq b(get_context(), uint256_t(NativeGroup::curve_b)); Fq adjusted_b = Fq::conditional_assign(is_point_at_infinity(), Fq::zero(), b); + // If `assert_is_on_curve` is true, we validate y^2 = x^3 + b by setting "fix_remainder_zero = true" when + // calling mult_madd. Otherwise, we return the difference x^3 + ax + b - y^2 if constexpr (!NativeGroup::has_a) { - // we validate y^2 = x^3 + b by setting "fix_remainder_zero = true" when calling mult_madd - Fq::mult_madd({ _x.sqr(), _y }, { _x, -_y }, { adjusted_b }, true); + result = Fq::mult_madd({ _x.sqr(), _y }, { _x, -_y }, { adjusted_b }, assert_is_on_curve); + ; } else { Fq a(get_context(), uint256_t(NativeGroup::curve_a)); Fq adjusted_a = Fq::conditional_assign(is_point_at_infinity(), Fq::zero(), a); - // we validate y^2 = x^3 + ax + b by setting "fix_remainder_zero = true" when calling mult_madd - Fq::mult_madd({ _x.sqr(), _x, _y }, { _x, adjusted_a, -_y }, { adjusted_b }, true); + result = Fq::mult_madd({ _x.sqr(), _x, _y }, { _x, adjusted_a, -_y }, { adjusted_b }, assert_is_on_curve); } if ((!has_circuit_failed) && (get_context()->failed())) { vinfo("Original bigfield error generated by biggroup::validate_on_curve: ", get_context()->err()); get_context()->failure(msg); } + + return result; } [[nodiscard]] bool is_constant() const @@ -252,6 +256,11 @@ template class element { return result; } + static element conditional_assign(const bool_t& predicate, const element& lhs, const element& rhs) + { + return rhs.conditional_select(lhs, predicate); + } + /** * @brief Asserts that two group elements are equal (i.e., x, y coordinates and infinity flag are all equal). * diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.test.cpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.test.cpp index 653076e67c3a..3da6ac309f2f 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.test.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.test.cpp @@ -68,6 +68,7 @@ template class stdlib_biggroup : public testing::Test { static constexpr auto EXPECT_CIRCUIT_CORRECTNESS = [](Builder& builder, bool expected_result = true) { info("num gates = ", builder.get_num_finalized_gates_inefficient()); EXPECT_EQ(CircuitChecker::check(builder), expected_result); + EXPECT_EQ(builder.failed(), !expected_result); }; // Helper to check the infinity status of a circuit element. @@ -227,11 +228,9 @@ template class stdlib_biggroup : public testing::Test { affine_element input_death(element::random_element()); auto x_death = element_ct::BaseField::from_witness(&builder, input_death.x); auto y_normal = element_ct::BaseField::from_witness(&builder, input_death.y); - auto pif_normal = bool_ct(witness_ct(&builder, false)); x_death.set_origin_tag(instant_death_tag); y_normal.set_origin_tag(constant_tag); - pif_normal.set_origin_tag(constant_tag); - element_ct death_point(x_death, y_normal, pif_normal, /*assert_on_curve=*/false); + element_ct death_point(x_death, y_normal, /*assert_on_curve=*/false); EXPECT_THROW(death_point + death_point, std::runtime_error); // AUDITTODO: incomplete_assert_equal has inconsistent instant_death behavior between builders. (this was simply @@ -2396,6 +2395,49 @@ using TestTypes = testing::Types) { + using Builder = TestFixture::Builder; + using element_ct = TestFixture::element_ct; + using Fq = TestFixture::Curve::BaseField; + using FqNative = TestFixture::Curve::BaseFieldNative; + using GroupNative = TestFixture::Curve::GroupNative; + + Builder builder; + auto [native_point, witness_point] = TestFixture::get_random_witness_point(&builder); + + // Valid point + Fq expected_zero = witness_point.validate_on_curve("biggroup::validate_on_curve", false); + expected_zero.assert_equal(Fq::zero()); + EXPECT_EQ(expected_zero.get_value(), static_cast(FqNative::zero())); + + // Invalid point + Fq random_x = Fq::from_witness(&builder, FqNative::random_element()); + Fq random_y = Fq::from_witness(&builder, FqNative::random_element()); + element_ct invalid_point(random_x, random_y, /*assert_on_curve*/ false); + Fq expected_non_zero = invalid_point.validate_on_curve("biggroup::validate_on_curve", false); + Fq expected_value = -random_y.sqr() + random_x.pow(3) + Fq(uint256_t(GroupNative::curve_b)); + if constexpr (GroupNative::has_a) { + expected_value += random_x * Fq(uint256_t(GroupNative::curve_a)); + } + expected_non_zero.assert_equal(expected_value); + + // Reduce the value to remove constants + expected_non_zero.self_reduce(); + expected_value.self_reduce(); + EXPECT_EQ(expected_non_zero.get_value(), expected_value.get_value()); + + TestFixture::EXPECT_CIRCUIT_CORRECTNESS(builder); + + // Check that the circuit fails if validate_on_curve is called with default parameters + [[maybe_unused]] Fq _ = invalid_point.validate_on_curve(); + TestFixture::EXPECT_CIRCUIT_CORRECTNESS(builder, false); + } +} + TYPED_TEST(stdlib_biggroup, basic_tag_logic) { TestFixture::test_basic_tag_logic(); diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_impl.hpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_impl.hpp index 636d7ecd3bfe..44adae1a84ac 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_impl.hpp @@ -57,7 +57,7 @@ element::element(const Fq& x_in, const Fq& y_in, const bool assert // Validate on-curve if requested if (assert_on_curve) { - validate_on_curve(); + [[maybe_unused]] Fq _ = validate_on_curve(); } } diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.test.cpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.test.cpp index 4f3334866241..87099a20e456 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.test.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.test.cpp @@ -95,10 +95,9 @@ TYPED_TEST(CycleGroupTest, TestBasicTagLogic) x_death.set_origin_tag(instant_death_tag); // Set constant tags on the other elements so they can be merged with instant_death_tag y_normal.set_origin_tag(constant_tag); - is_infinity_normal.set_origin_tag(constant_tag); // Use assert_on_curve=false to avoid triggering instant_death during validate_on_curve() - cycle_group_ct b(x_death, y_normal, is_infinity_normal, /*assert_on_curve=*/false); + cycle_group_ct b(x_death, y_normal, /*assert_on_curve=*/false); // Even requesting the tag of the whole structure can cause instant death EXPECT_THROW(b.get_origin_tag(), std::runtime_error); #endif