chore: deterministic backward compatibility tests#591
Conversation
dvdplm
left a comment
dvdplm
left a comment
There was a problem hiding this comment.
A few nitpicks but otherwise it LGTM. I don't love the ALLOW_UNCOVERED stuff or the workspace scanning, but I also don't know of a better way to do this.
| // BTreeMap gives a canonical iteration order so the serialized output is | ||
| // deterministic across processes. The previous HashMap field is preserved | ||
| // in KeyGenMetadataInnerV1 below for backward compatibility. |
There was a problem hiding this comment.
I think this comment is superfluous. Or could be shorter "BTreeMaps are deterministically serialized"
| ) -> Result<InternalCustodianSetupMessage, BackupError> { | ||
| let timestamp = SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs(); | ||
| self.generate_setup_message_with_timestamp(rng, custodian_name, timestamp) | ||
| } | ||
|
|
||
| // The timestamp is taken as an explicit argument so that callers needing deterministic | ||
| // output (e.g. backward-compatibility data generators) can pass a fixed value. | ||
| pub fn generate_setup_message_with_timestamp<R: Rng + CryptoRng>( | ||
| &self, | ||
| rng: &mut R, | ||
| custodian_name: String, | ||
| timestamp: u64, | ||
| ) -> Result<InternalCustodianSetupMessage, BackupError> { |
There was a problem hiding this comment.
Do we really need both of these? Is generate_setup_message_with_timestamp really used separately from generate_setup_message? Maybe we can unify them into generate_setup_message and save ourselves some pub-API surface?
There was a problem hiding this comment.
it's a bit tricky, generate_setup_message_with_timestampbecause I wanted a "new" function that's deterministic, for the backward compatibility tests. but generate_setup_message is used everywhere else
| //! `with_fixed_int_encoding()` on top of `Versionize::versionize()`), and | ||
| //! asserts every output is byte-identical. | ||
| //! | ||
| //! The N-repeats pattern is what catches HashMap regressions: each freshly |
There was a problem hiding this comment.
Hmm, what is it that we're actually testing here? It reads a bit like we're checking that HashMap is indeed non-deterministic and that BTreeMap is indeed deterministic? That'd be the concern of Rust's stdlib to do, i.e. that the documented invariants stay in place.
I don't think we need this. Am I wrong?
There was a problem hiding this comment.
it's more of a sanity check. on one hand it's not super useful for BTreeMap, but we also have functions like generate_setup_message_with_timestamp which is suppose to make de-randomize the initialization
kc1212
force-pushed
the
kc1212/chore/3023/deterministic-backward-compatibility-tests
branch
from
734a02f to
6994051
Compare
Consolidated Tests Results 2026-05-20 - 15:00:54Test Results
Details
test-reporter: Run #2282
🎉 All tests passed!TestsView All Tests
🍂 No flaky tests in this run. Github Test Reporter by CTRF 💚 🔄 This comment has been updated |
kc1212
force-pushed
the
kc1212/chore/3023/deterministic-backward-compatibility-tests
branch
2 times, most recently
from
1c4e1fc to
2d33cc5
Compare
kc1212
force-pushed
the
kc1212/chore/3023/deterministic-backward-compatibility-tests
branch
from
2d33cc5 to
cea9f4e
Compare
2 participants
Description of changes
This is not exactly a complete fix because on main we don't have deterministic datatypes, it only introduces in this PR. There needs to be a follow up PR that changes the revision in
kms/backward-compatibility/generate-v0.14.0/Cargo.tomlafter this PR is merged.To test it locally, one can use this commit cea9f4e, but I didn't want to add it in this PR because it's not a commit on main.
Issue ticket number and link
Closes zama-ai/kms-internal#3023
PR Checklist
I attest that all checked items are satisfied. Any deviation is clearly justified above.
chore: ...).TODO(#issue).unwrap/expect/paniconly in tests or for invariant bugs (documented if present).devopslabel + infra notified + infra-team reviewer assigned.!and affected teams notified.Zeroize+ZeroizeOnDropimplemented.unsafe; if unavoidable: minimal, justified, documented, and test/fuzz covered.Dependency Update Questionnaire (only if deps changed or added)
Answer in the
Cargo.tomlnext to the dependency (or here if updating):More details and explanations for the checklist and dependency updates can be found in CONTRIBUTING.md