Skip to content

Conversation

@iakovenkos
Copy link
Contributor

@iakovenkos iakovenkos commented Jan 8, 2026

Summary

This PR addresses findings from the external audit of the Transcript and Poseidon2 components. The changes ensure consistent behavior between native and in-circuit verification, particularly around field element serialization/deserialization.

1. Field Element Serialization/Deserialization (Issues 5, 6, 9, 10, 11)

Problem: Inconsistent handling of field elements during serialization, particularly around edge cases involving the field modulus and aliased values.

  • validate_split_in_field_unsafe now properly rejects hi||lo == field_modulus (Issue 5)
  • Replaced hardcoded $254$ with generically calculated max_bits (Issue 6)
  • Added proper borrow boolean check even when lo is constant (Issue 10)
  • Unified deserialization behavior: both FrCodec (native) and StdlibCodec (in-circuit) now use assert_is_in_field() to reject unreduced values (Issues 9 & 11)
  • For Mega arithmetization, validation is deferred to Translator+ECCVM; for Ultra, strict validation happens immediately via bigfield

2. Transcript Round Tracking (Issues 3, 4)

  • Removed redundant round_number field, now using only round_index (Issue 4)
  • Consecutive challenges with no data between them now stay in the same round
  • prover_init_empty and verifier_init_empty are now properly marked as testing methods (Issue 3)

3. Poseidon2 Cleanup (Issues 1, 2, 7, 8)

Strategy:

  • Not marking the first element of the sponge state as used.
  • Added documentation for $-1$ offset in internal_matrix_diagonal (Issue 2)
  • Removed unnecessary normalization assert in matrix_multiplication_external (Issue 7)
  • Fixed transcript initialization in circuit failure tests (Issue 8)

Commits:

  • Issue 1 (68011cc): First element of of FieldSponge::hash_internal is not marked as used
  • Issue 2 (2dd4197): Missing annotation about $-1$ offset in poseidon2 internal_matrix_diagonal
  • Issue 3 (1dddf54): prover_init_empty and verifier_init_empty are testing methods
  • Issue 4 (ced1a88): round_number is redundant with round_index
  • Issues 5 & 6 (e4fa3c5): validate_split_in_field_unsafe accepts hi||lo == modulus + hardcoded $254$
  • Issue 7 (dd1f644): Unnecessary assert for matrix_multiplication_external output normalization
  • Issue 8 (d064598): Verification in poseidon2.circuit.failure.test.cpp fails on valid input
  • Issues 9 & 11 (7f1cb65): deserialize_from_fields rejects unreduced fqs + (fq_modulus, fq_modulus) handling
  • Issue 10 (98178c5): validate_split_in_field_unsafe skips borrow check when lo is constant

Documentation

Added barretenberg/cpp/src/barretenberg/stdlib/primitives/field/CODEC_README.md documenting:

  • Codec architecture (FrCodec, StdlibCodec, U256Codec)
  • Field element encoding (2-limb representation for fq)
  • Canonical representation requirements and point at infinity handling
  • Ultra vs Mega arithmetization differences
  • Threat model: why native/recursive consistency matters
  • Mega/Goblin deferred validation flow and ECCVM↔Translator translation check

New Tests

field_utils.test.cpp (new file):

  • ValidateSplitRejectsModulus — Issue 5: split rejects hi||lo == modulus
  • ValidateSplitAcceptsModulusMinusOne — edge case: modulus - 1 is valid
  • SplitUniqueRejectsModulus — Issue 5: unique split rejects modulus
  • SplitUniqueMaxValue — edge case handling
  • ValidateSplitConstantLoWitnessHiRejectsModulus — Issue 10: constant lo with witness hi
  • ValidateSplitWitnessLoConstantHiRejectsModulus — Issue 10: witness lo with constant hi

field_conversion.test.cpp:

  • BigfieldDeserializationFailsOnLimbOverflow — limb bounds validation
  • BothCodecsRejectPointAtInfinityAlias — Issue 11: aliased infinity rejected
  • BothCodecsAcceptCanonicalRejectAlias — Issues 9, 11: native/circuit consistency
  • AcceptCanonicalPointAtInfinity — canonical infinity accepted
  • RejectPointNotOnCurve — off-curve points rejected

poseidon2.test.cpp:

  • PointCoordinatesVsAliasProduceDifferentHashes — aliased coords produce different hashes

poseidon2.circuit.failure.test.cpp:

  • ValidCircuitVerifies — Issue 8: baseline validity check

@iakovenkos iakovenkos self-assigned this Jan 8, 2026
@iakovenkos iakovenkos requested a review from notnotraju January 8, 2026 16:32
@iakovenkos iakovenkos marked this pull request as ready for review January 8, 2026 16:33
EXPECT_TRUE(CircuitChecker::check(builder));
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

super!

* the resulting hash must be different from the canonical representation.
* Also verifies consistency between stdlib and native Poseidon2 implementations.
*/
TYPED_TEST(StdlibPoseidon2, PointCoordinatesVsAliasProduceDifferentHashes)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice test. Nit suggestion (optional): create a helper function (fixture) like other tests and call that in the test.

Copy link
Contributor

@suyash67 suyash67 left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for adding the codec readme, its very useful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants