Skip to content

Poseidon-arrays#2134

Closed
richardpringle wants to merge 42 commits intodevelopfrom
rp/poseidon-arrays
Closed

Poseidon-arrays#2134
richardpringle wants to merge 42 commits intodevelopfrom
rp/poseidon-arrays

Conversation

@richardpringle
Copy link
Copy Markdown
Collaborator

This is a draft. Will edit soon

@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 2, 2026

OCaml Reference Validation Results

Repository: https://github.com/MinaProtocol/mina.git
Branch: compatible
Status: ❌ Validation failed

Click to see full validation output
Checking OCaml references against https://github.com/MinaProtocol/mina.git (branch: compatible)
Fetching current commit from compatible...
Current OCaml commit: 96d469b19dc4bbc3f8d2a53e8c50f2cc99980412

Validating references...
========================
[OK] VALID: crates/ledger/src/account/account.rs:1266 -> src/lib/mina_base/account.ml L:201-224
  [WARN] STALE COMMIT: fc6be4c58091c761f827c858229c2edf9519e941 (current: 96d469b19dc4bbc3f8d2a53e8c50f2cc99980412)
[OK] VALID: crates/ledger/src/scan_state/transaction_logic.rs:117 -> src/lib/mina_base/transaction_status.ml L:9-51
  [WARN] STALE COMMIT: 5da42ccd72e791f164d4d200cf1ce300262873b3 (current: 96d469b19dc4bbc3f8d2a53e8c50f2cc99980412)
[OK] VALID: crates/ledger/src/scan_state/transaction_logic.rs:233 -> src/lib/mina_base/transaction_status.ml L:452-454
  [WARN] STALE COMMIT: 5da42ccd72e791f164d4d200cf1ce300262873b3 (current: 96d469b19dc4bbc3f8d2a53e8c50f2cc99980412)
[OK] VALID: crates/ledger/src/scan_state/transaction_logic.rs:249 -> src/lib/mina_base/with_status.ml L:6-10
  [WARN] STALE COMMIT: 5da42ccd72e791f164d4d200cf1ce300262873b3 (current: 96d469b19dc4bbc3f8d2a53e8c50f2cc99980412)
[OK] VALID: crates/ledger/src/scan_state/transaction_logic.rs:319 -> src/lib/mina_base/fee_transfer.ml L:76-80
  [WARN] STALE COMMIT: 5da42ccd72e791f164d4d200cf1ce300262873b3 (current: 96d469b19dc4bbc3f8d2a53e8c50f2cc99980412)
[OK] VALID: crates/ledger/src/scan_state/transaction_logic.rs:344 -> src/lib/mina_base/fee_transfer.ml L:68-69
  [WARN] STALE COMMIT: 5da42ccd72e791f164d4d200cf1ce300262873b3 (current: 96d469b19dc4bbc3f8d2a53e8c50f2cc99980412)
[OK] VALID: crates/ledger/src/scan_state/transaction_logic.rs:419 -> src/lib/mina_base/coinbase.ml L:17-21
  [WARN] STALE COMMIT: 5da42ccd72e791f164d4d200cf1ce300262873b3 (current: 96d469b19dc4bbc3f8d2a53e8c50f2cc99980412)
[OK] VALID: crates/ledger/src/scan_state/transaction_logic.rs:1039 -> src/lib/transaction/transaction.ml L:8-11
  [WARN] STALE COMMIT: 5da42ccd72e791f164d4d200cf1ce300262873b3 (current: 96d469b19dc4bbc3f8d2a53e8c50f2cc99980412)
[ERROR] INVALID: crates/ledger/src/scan_state/transaction_logic/for_tests.rs:63
   Code at L:2285-2285 differs between commit 5da42ccd72e791f164d4d200cf1ce300262873b3 and current branch
   Referenced: https://github.com/MinaProtocol/mina/blob/5da42ccd72e791f164d4d200cf1ce300262873b3/src/lib/transaction_logic/mina_transaction_logic.ml#L2285-L2285
   Current:    https://github.com/MinaProtocol/mina/blob/compatible/src/lib/transaction_logic/mina_transaction_logic.ml#L2285-L2285
[ERROR] INVALID: crates/ledger/src/scan_state/transaction_logic/for_tests.rs:67
   Code at L:2351-2356 differs between commit 5da42ccd72e791f164d4d200cf1ce300262873b3 and current branch
   Referenced: https://github.com/MinaProtocol/mina/blob/5da42ccd72e791f164d4d200cf1ce300262873b3/src/lib/transaction_logic/mina_transaction_logic.ml#L2351-L2356
   Current:    https://github.com/MinaProtocol/mina/blob/compatible/src/lib/transaction_logic/mina_transaction_logic.ml#L2351-L2356
[ERROR] INVALID: crates/ledger/src/scan_state/transaction_logic/for_tests.rs:76
   Code at L:2407-2407 differs between commit 5da42ccd72e791f164d4d200cf1ce300262873b3 and current branch
   Referenced: https://github.com/MinaProtocol/mina/blob/5da42ccd72e791f164d4d200cf1ce300262873b3/src/lib/transaction_logic/mina_transaction_logic.ml#L2407-L2407
   Current:    https://github.com/MinaProtocol/mina/blob/compatible/src/lib/transaction_logic/mina_transaction_logic.ml#L2407-L2407
[OK] VALID: crates/ledger/src/scan_state/transaction_logic/signed_command.rs:17 -> src/lib/mina_base/signed_command_payload.ml L:34-48
  [WARN] STALE COMMIT: 5da42ccd72e791f164d4d200cf1ce300262873b3 (current: 96d469b19dc4bbc3f8d2a53e8c50f2cc99980412)
[OK] VALID: crates/ledger/src/scan_state/transaction_logic/signed_command.rs:43 -> src/lib/mina_base/stake_delegation.ml L:11-13
  [WARN] STALE COMMIT: 5da42ccd72e791f164d4d200cf1ce300262873b3 (current: 96d469b19dc4bbc3f8d2a53e8c50f2cc99980412)
[OK] VALID: crates/ledger/src/scan_state/transaction_logic/signed_command.rs:70 -> src/lib/mina_base/signed_command_payload.ml L:179-181
  [WARN] STALE COMMIT: 5da42ccd72e791f164d4d200cf1ce300262873b3 (current: 96d469b19dc4bbc3f8d2a53e8c50f2cc99980412)
[OK] VALID: crates/ledger/src/scan_state/transaction_logic/signed_command.rs:81 -> src/lib/mina_base/signed_command_payload.ml L:239-243
  [WARN] STALE COMMIT: 5da42ccd72e791f164d4d200cf1ce300262873b3 (current: 96d469b19dc4bbc3f8d2a53e8c50f2cc99980412)
[OK] VALID: crates/ledger/src/scan_state/transaction_logic/signed_command.rs:112 -> src/lib/mina_base/signed_command_payload.ml L:352-362
  [WARN] STALE COMMIT: 5da42ccd72e791f164d4d200cf1ce300262873b3 (current: 96d469b19dc4bbc3f8d2a53e8c50f2cc99980412)
[ERROR] INVALID: crates/p2p-messages/src/string.rs:14
   Code at L:140-140 differs between commit c0c9d702b8cba34a603a28001c293ca462b1dfec and current branch
   Referenced: https://github.com/MinaProtocol/mina/blob/c0c9d702b8cba34a603a28001c293ca462b1dfec/src/lib/mina_base/zkapp_account.ml#L140-L140
   Current:    https://github.com/MinaProtocol/mina/blob/compatible/src/lib/mina_base/zkapp_account.ml#L140-L140
[OK] VALID: crates/p2p-messages/src/string.rs:16 -> src/lib/mina_base/account.ml L:92-92
  [WARN] STALE COMMIT: c0c9d702b8cba34a603a28001c293ca462b1dfec (current: 96d469b19dc4bbc3f8d2a53e8c50f2cc99980412)

Summary
=======
Total references found: 18
Valid references: 14
Invalid references: 4
Stale commits: 14

[ERROR] Validation failed: 4 invalid reference(s) found

@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 2, 2026

✓ Code Reference Verification Passed

All code references in the documentation have been verified successfully!

Total references checked: 1
Valid references: 1

The documentation is in sync with the codebase on the develop branch.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🤔 this is already in the mina-poseidon crate...

Comment on lines +1772 to +1774
pub trait SpongeParamsForField<F: FieldWitness> {
fn get_params(is_legacy: bool) -> &'static SpongeParams<F>;
}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This could be a const


let expected_root_hash = LedgerHash::from_str(expected_root_hash).unwrap().0.clone();

assert_eq!(root_hash, expected_root_hash);
Copy link
Copy Markdown
Collaborator Author

@richardpringle richardpringle Feb 2, 2026

Choose a reason for hiding this comment

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

sus that this was deleted (the whole test that is)

//!
//! // Initialize SNARK state with configuration
//! let config = SnarkConfig { /* ... */ };
//! let config: SnarkConfig = todo!();
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🤔

}

#[test]
// #[ignore = "Runs long, remember to re-enable me!"]
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Now remember to delete the comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There shouldn't be any changes in here, this whole file/crate should be deleted

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

delete me

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

delete me

}

fn block_cipher<F: FieldWitness>(
fn block_cipher<F: FieldWitness + SpongeParamsForField<F>>(
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

And here... but does it make sense that this even has to be re-implemented?

params: &SpongeParams<F>,
w: &mut Witness<F>,
) {
let round_constants = &<F as SpongeParamsForField<F>>::get_params(false).round_constants;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is odd. This is just a constant based on F. It should be a different generic argument, I would think, not a trait bound on F.

.zip(params.mds[i].iter())
.fold(F::zero(), |x, (s, &m)| m * s + x)
})
fn apply_mds_matrix<F: FieldWitness + SpongeParamsForField<F>>(state: &[F; 3]) -> [F; 3] {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This should already exist in mina_poseidon


for state_i in state.iter_mut() {
*state_i = sbox::<F>(*state_i);
*state_i = sbox(*state_i);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

sbox should already exist in mina-poseidon. How is this different?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It's because of the witness

let xi_chal: [u64; 2] = sponge.squeeze_limbs();
let r_chal: [u64; 2] = sponge.squeeze_limbs();
let xi_chal: [u64; 2] = sponge.squeeze_limbs(2).try_into().unwrap();
let r_chal: [u64; 2] = sponge.squeeze_limbs(2).try_into().unwrap();
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

no unwraps


fn to_fqs(fps: &[Fp]) -> Vec<Fq> {
fps.iter().map(|fp| Fq::from((*fp).into_bigint())).collect()
}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

...this is odd that we have to do this

Comment on lines +1588 to +1589
pub fields: Vec<F>,
pub bits: Vec<bool>,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

These probably shouldn't be pub

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is kind of crazy too. Can and should use bytemuck to do something more efficient

Comment on lines +1601 to +1602
fields: Vec::with_capacity(256),
bits: Vec::with_capacity(512),
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Comment on sizes here

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure where these hashes came with to begin with

Unattached { hashes, .. } => hashes,
Root { database, .. } => return dbg!(database.get_cached_hash(addr)),
Attached { hashes, .. } => dbg!(hashes),
Unattached { hashes, .. } => dbg!(hashes),
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

whoops

let of_coord =
|(x, y): &(BigInt, BigInt)| Ok(Vesta::of_coordinates(x.to_field()?, y.to_field()?));
let of_coord = |(x, y): &(BigInt, BigInt)| {
Vesta::of_coordinates(x.to_field::<Fq>(), y.to_field::<Fq>())
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fq can be inferred herer

BigInt: From<&'a <G as AffineRepr>::ScalarField>,
BigInt: From<&'a <G as AffineRepr>::BaseField>,
BigInt: From<<G as AffineRepr>::ScalarField>,
BigInt: From<<G as AffineRepr>::BaseField>,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

These are Copy types, we shouldn't be dealing with references here

fn block_cipher<F: FieldWitness>(
fn block_cipher<F: FieldWitness + SpongeParamsForField<F>>(
mut state: [F; M],
params: &SpongeParams<F>,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

did I need this for Legacy-params?

number: number.to_ne_bytes(),
}
.take(NBITS)
pub fn bits_iter<N: Pod, const NBITS: usize>(number: &N) -> impl Iterator<Item = bool> + '_ {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Need to double check this


pub fn to_bits<N: Into<u64>, const NBITS: usize>(number: N) -> [bool; NBITS] {
let mut iter = bits_iter::<N, NBITS>(number);
pub fn to_bits<N: Pod, const NBITS: usize>(number: N) -> [bool; NBITS] {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

And this


for key in keys {
match BaseLedger::location_of_account(&ledger, key) {
match BaseLedger::location_of_account(&ledger, dbg!(key)) {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Whoops

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Couple of dbg! statements that need to be deleted here

self.0.clone()
}
fn domain_string(_: ()) -> Option<String> {
Some("MinaProtoState".to_string())
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Should use HashParam if it's accessible

ROInput::new()
}
fn domain_string(_: ()) -> Option<String> {
Some("CoinbaseStack".to_string())
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

HashParam?

test_to_field!(to_field_u64_test: append_u64(u64::MIN, u64::MAX/2, u64::MAX) = "ffffffffffffffffffffffffffffff7f00000000000000000000000000000000");
test_to_field!(to_field_u48_test: append_u48(u48(u64::MIN), u48(u64::MAX/2), u48(u64::MAX)) = "ffffffffffffffffffffffff0000000000000000000000000000000000000000");
test_to_field!(to_field_bytes_test: append_bytes(&[0, 1, 255]) = "ff80000000000000000000000000000000000000000000000000000000000000");
test_to_field!(to_field_bytes_test: append_bytes_be(&[0, 1, 255]) = "ff80000000000000000000000000000000000000000000000000000000000000");
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🤔

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

PaddedSeq wraps an array, so we should just delegate to an array's Serialize and Deserialize implementations

where
S: serde::Serializer,
{
let mut serializer = serializer.serialize_tuple(N + 1)?;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No idea why the + 1 was here

inputs
}
fn domain_string(height: u32) -> Option<String> {
Some(format!("MinaMklTree{:03}", height))
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Should use HashParam

self.0.clone()
}
fn domain_string(_: Self::D) -> Option<String> {
Some("MinaVrfMessage".to_string())
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

HashParam

delegator_index: u64,
pub global_slot: u32,
pub epoch_seed: EpochSeed,
pub delegator_index: u64,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🤔 why are these pub?

inputs.append(x);
inputs.append(y);
fn domain_string(_: Self::D) -> Option<String> {
Some("MinaVrfOutput".to_string())
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

HashParam

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

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants