Skip to content

Add full type check for SHA3 portable#1157

Open
parrot7483 wants to merge 12 commits intomainfrom
Parrot7483/sha3-type-check
Open

Add full type check for SHA3 portable#1157
parrot7483 wants to merge 12 commits intomainfrom
Parrot7483/sha3-type-check

Conversation

@parrot7483
Copy link
Collaborator

@parrot7483 parrot7483 commented Sep 18, 2025

This PR adds full type checking for the SHA3 portable version for #395

Extract using hax.py extract --portable, prove using hax.py prove.

Besides prove code the following things where changed:

  • reorder functions in libcrux-sha3/src/generic_keccak/xof.rs
  • The absorb_full function in libcrux-sha3/src/generic_keccak/xof.rs previously did not always consume the internal buffer when it was full. This state could only be reached by misusing the API. The only bug I found.
  • split up the rho and pi function for verification purpose

This version contains a few things I would consider "hacks"

  • The extraction for the squeeze does not work. We add the expected F* code using hax_lib::fstar::replace
  • impl From<u32> for Algorithm does not work because of the panic.
  • In libcrux-sha3/src/generic_keccak/xof.rs the #[hax_lib::fstar::options("...")] has no effect. I use an hax_lib::fstar::before instead

@parrot7483 parrot7483 force-pushed the Parrot7483/sha3-type-check branch from 8f7a614 to 223bcaf Compare September 18, 2025 11:07
@parrot7483 parrot7483 marked this pull request as ready for review September 18, 2025 11:50
@parrot7483 parrot7483 requested review from a team as code owners September 18, 2025 11:50
Copy link
Member

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Sorry for being a little slow on this.
Here a couple first comments. Can you address them and update? Then we can do another round.

Copy link
Member

Choose a reason for hiding this comment

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

This requires the unconditional hax_lib dependency for debug builds. Is there a better way to do this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think I understand this correctly. Would putting a cfg(hax) be enough? It is only needed for proof.

@parrot7483
Copy link
Collaborator Author

@franziskuskiefer I applied your feedback and also cherry-picked one commit from my AVX/NEON branch adding the correct precondition on the traits and doing some light refactoring (mainly having consistent names for variables).

Copy link
Member

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Thanks, looks pretty good. A couple things and some cleanup, then this can go in.

Comment on lines 122 to 123
Copy link
Member

Choose a reason for hiding this comment

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

Are these changes needed for the proof? If yes, they'd need some explanation to why. If not, we should keep it as is.
Same for the similar change below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, if I inline everything F* can't figure out that the bounds are correct every time. Sometimes it works sometimes it doesn't.

I think inlining let bytes = get_ij(s, octets / 5, octets % 5).to_le_bytes() should be possible.

"
)]
pub(crate) trait Squeeze1<T: KeccakItem<1>> {
#[hax_lib::attributes]
pub(crate) trait Squeeze<T: KeccakItem<1>> {
Copy link
Member

Choose a reason for hiding this comment

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

Why did this lose the 1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I want to emphasize that having the Squeeze trait multiple times is because of HAX not supporting &[&mut [u8]; N] or other types with mutables inside. If that would work that would simplify a lot of things in this crate. I will write a comment explaining it.

@parrot7483
Copy link
Collaborator Author

@franziskuskiefer I applied your review. cherry-picked 1064d0d from SIMD branch. Merged main.

@parrot7483
Copy link
Collaborator Author

I have a tendency to pressing these big green buttons on github messing everything up. sorry.

@jschneider-bensch jschneider-bensch added the waiting-on-author Status: This is awaiting some action from the author. label Jan 20, 2026
@parrot7483 parrot7483 force-pushed the Parrot7483/sha3-type-check branch from 589ff18 to 14e49e7 Compare January 21, 2026 20:15
@parrot7483
Copy link
Collaborator Author

parrot7483 commented Jan 21, 2026

What do you want to do about these warnings?

warning: unused attribute
   --> crates/algorithms/sha3/src/lib.rs:181:1
    |
181 | #[inline(always)]
    | ^^^^^^^^^^^^^^^^^ help: remove this attribute
    |
note: attribute also specified here
   --> crates/algorithms/sha3/src/lib.rs:176:1
    |
176 | #[cfg_attr(not(eurydice), inline(always))]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!`

@parrot7483
Copy link
Collaborator Author

F* proofs failing with "Namespace 'Core' cannot be found" after core-models directory moved

Reproducer:

  • cd libcrux-ml-kem && ./hax.py extract && ./hax.py prove
  • cd crates/algorithms/sha3 && ./hax.py extract && ./hax.py prove

Both fail at prove step with same error. ML-KEM and SHA3 both have this issue.

Using the old name "Core_models" still works for single models.

@parrot7483 parrot7483 force-pushed the Parrot7483/sha3-type-check branch from 14e49e7 to 2c33cb9 Compare February 3, 2026 16:19
@parrot7483
Copy link
Collaborator Author

The auto-discovery in Makefile.generic doesn't find the Core F* library from the hax proof-libs, so I had to set HAX_HOME explicitly, which doesn't work in CI.

Fix F* Makefile include paths.

Work around hax from_fn extraction bug: Add helper function with fstar::replace to fix incorrect type parameter
extraction. See: cryspen/hax#1920.
Copy link
Member

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

lgtm with those nits addressed

interface, "
class t_Squeeze (v_Self: Type0) (v_T: Type0) = {
// TODO: This super variable is problematic and makes typecheck fail
// https://github.com/cryspen/hax/issues/1554
Copy link
Member

Choose a reason for hiding this comment

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

Looks like that issue is closed and if it works without the commented code, let's drop it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

still a problem. I reopened the issue

}

// TODO: See above
// [@@ FStar.Tactics.Typeclasses.tcinstance]
Copy link
Member

Choose a reason for hiding this comment

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

Same here, let's remove when it's not used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same

@franziskuskiefer
Copy link
Member

What do you want to do about these warnings?

that should be fixed in a separate PR

env=hax_env,
)

# Extract Core models
Copy link
Collaborator

Choose a reason for hiding this comment

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

From the output of ./hax.py extract --portable it looks like nothing is extracted here actually, and you also removed the core-models proof directory from FSTAR_INCLUDE_DIRS_EXTRA in the Makefile. So is this extraction step needed here, after all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure. I am confused about the difference between Core and Core_models

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to avoid this manual F* dependency by making the lemmas in proof_utils::lemmas into Rust functions (nops which are replaced as they are now) and calling these from Rust instead of invoking them in inline fstar! blocks?

@@ -1,8 +1,25 @@
use hax_lib::int::*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be behind a #[cfg(hax)], no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question about avoiding the manual F* open as in generic_keccak::portable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you move it to proof_utils and pass in xof.buf_len from here instead of passing in xof and accessing it in the invariant body?

@parrot7483
Copy link
Collaborator Author

@jschneider-bensch Does buf_len need to remain private, or is pub(crate) acceptable? I made it pub(crate) so portable.rs can access it when calling keccak_xof_state_inv in the trait implementations.

@parrot7483 parrot7483 requested review from jschneider-bensch and removed request for karthikbhargavan February 10, 2026 19:12
@parrot7483
Copy link
Collaborator Author

@jschneider-bensch friendly ping

@jschneider-bensch
Copy link
Collaborator

@jschneider-bensch Does buf_len need to remain private, or is pub(crate) acceptable? I made it pub(crate) so portable.rs can access it when calling keccak_xof_state_inv in the trait implementations.

Sorry for the late reply!
I think having it pub(crate) is okay.

@parrot7483
Copy link
Collaborator Author

parrot7483 commented Feb 17, 2026

Can we merge then?

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

Labels

keep-open waiting-on-author Status: This is awaiting some action from the author.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments