Skip to content

Conversation

@kaimast
Copy link
Contributor

@kaimast kaimast commented Nov 10, 2025

This PR aims to address the test failures seen in [this run](https://app.circleci.com/pipelines/github/ProvableHQ/snarkOS/2998/workflows/68fb3599-4c04-49b3-810f-2f35b013ff61/jobs/58981.

I believe this error occurs because in the current implementation any string that starts with a ? is considered invalid/unknown, whereas with this change there is exactly one sequence that corresponds to unknown.
So it might encode "?\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0", but decoding it will return unknown.

Changes

The PR changes the SHA to always be encoded as a 40-byte array. There was code that converted it from a string, without checking the length, which was error-prone.
Additionally, it relied on a hardcoded "unknown" string, which is now replaced by a constant UNKNOWN_SHA.

An alternative fix would have been to change the protest to not generate a string containing ?, but, in the long-term, it is safer to also ensure we always send exactly 40 bytes.

Reproducibility

I was able to reproduce this on staging by running PROPTEST_SEED=e472a2f4ccb7abe5a6f77dc67ab294386b82e205ea3a889c7707fa15328545c3 cargo test --package snarkos-node-bft-events helpers::codec::tests::event_roundtrip which generates the error below:

thread 'helpers::codec::tests::event_roundtrip' panicked at node/bft/events/src/helpers/codec.rs:108:5:
Test failed: assertion `left == right` failed
  left: [7, 0, 0, 0, 0, 0, 0, 0, 125, 34, 130, 115, 120, 14, 165, 160, 121, 106, 108, 183, 190, 119, 186, 201, 106, 81, 181, 42, 179, 55, 3, 133, 233, 50, 204, 189, 142, 70, 0, 5, 0, 0, 0, 0, 0, 0, 0, 0, 117, 110, 107, 110, 111, 119, 110]
 right: [7, 0, 0, 0, 0, 0, 0, 0, 125, 34, 130, 115, 120, 14, 165, 160, 121, 106, 108, 183, 190, 119, 186, 201, 106, 81, 181, 42, 179, 55, 3, 133, 233, 50, 204, 189, 142, 70, 0, 5, 0, 0, 0, 0, 0, 0, 0, 0, 63, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0].
minimal failing input: input = _EventRoundtripArgs {
    event: ChallengeRequest(
        ChallengeRequest {
            version: 0,
            listener_port: 0,
            address: aleo1053gyumcp6j6q7t2djmmuaa6e949rdf2kvms8p0fxtxtmrjxqqzslunqv7,
            nonce: 0,
            snarkos_sha: "?\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0",
        },
    ),
}

@kaimast kaimast force-pushed the fix/sha-length branch 2 times, most recently from 7e2881b to a159525 Compare November 10, 2025 22:17
@kaimast kaimast marked this pull request as ready for review November 10, 2025 22:55
@kaimast kaimast force-pushed the fix/sha-length branch 2 times, most recently from d5c83aa to 659274e Compare November 10, 2025 23:03
Copy link
Collaborator

@vicsn vicsn left a comment

Choose a reason for hiding this comment

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

LGTM! glad you found the issue. After merging this PR, could you duplicate my latest sha fix PR into canary onto staging?

@kaimast kaimast merged commit de5dd90 into staging Nov 12, 2025
5 checks passed
@kaimast kaimast deleted the fix/sha-length branch November 12, 2025 01:07
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.

3 participants