Extract checkpoint signed-note format into a new tlog_checkpoint crate#233
Extract checkpoint signed-note format into a new tlog_checkpoint crate#233lukevalenta wants to merge 1 commit intolvalenta/tlog-crate-splitfrom
Conversation
Slice 2 of #230. The c2sp.org/tlog-checkpoint signed-note format is a sibling spec to c2sp.org/tlog-tiles, not a part of it: the tlog-tiles spec defers checkpoint formatting to tlog-checkpoint. Until now the implementation lived inside the tlog_tiles crate alongside the tile-encoding wire format. This commit moves it into its own tlog_checkpoint crate. New crate `crates/tlog_checkpoint`: - `CheckpointText` + `MalformedCheckpointTextError` — text parser/ serializer for the c2sp.org/tlog-checkpoint body. - `CheckpointSigner` trait + `Ed25519CheckpointSigner` — basic Ed25519 signed-note algorithm (0x01). - `TreeWithTimestamp` — a (size, hash, timestamp) tuple ready to be signed. - `open_checkpoint` — caller-side parse-and-fully-verify entry point. - `CheckpointError` — error variants for the entry points above. - `UnixTimestampMillis` — type alias for the timestamp embedded in a checkpoint cosignature (see rename below). Dependencies: tlog_core, signed_note, ed25519-dalek, base64, sha2, rand, thiserror. Renamed `UnixTimestamp` to `UnixTimestampMillis`: The previous name didn't say what unit it was, even though the workspace uses it exclusively for milliseconds (every callsite reads `now_millis()` and the parameter names were `timestamp_unix_millis`). Putting the unit in the type name makes the contract explicit at the trait boundary and lets parameter names drop the redundant suffix: -fn sign(timestamp_unix_millis: UnixTimestamp, ...) -> ... +fn sign(timestamp: UnixTimestampMillis, ...) -> ... The cosignature_v1 / subtree_v1 cosigners that need POSIX seconds keep their `/ 1000` conversion at the spec boundary, with the local `timestamp_unix_secs` variable name preserved for the local domain where the unit changes. Cleanups in `tlog_tiles`: - `tlog_tiles` now contains only the tile-encoding wire format (tile.rs) and the LogEntry/PendingLogEntry abstractions (entries.rs). Slice 3 will move entries.rs into generic_log_worker. - Dropped now-unused deps: ed25519-dalek, rand, signed_note, thiserror. url, length_prefixed, base64, sha2, serde, tlog_core, tlog_checkpoint remain. - Description in Cargo.toml shortened to mention only c2sp.org/tlog-tiles. Migration: every internal consumer that used `tlog_tiles::CheckpointSigner`, `tlog_tiles::CheckpointText`, `tlog_tiles::open_checkpoint`, `tlog_tiles::Ed25519CheckpointSigner`, `tlog_tiles::TreeWithTimestamp`, `tlog_tiles::UnixTimestamp(/Millis)`, `tlog_tiles::CheckpointError`, or `tlog_tiles::MalformedCheckpointTextError` now imports from `tlog_checkpoint` directly. Touched: `tlog_witness`, `tlog_cosignature`, `tlog_tiles_wasm`, `static_ct_api`, `bootstrap_mtc_api`, `bootstrap_mtc_worker`, `ct_worker`, `generic_log_worker`, `witness_worker`, `integration_tests`, `x509_util`, and the `fuzz_parse_checkpoint` target. Several consumers (`tlog_witness`, `tlog_cosignature`, `tlog_tiles_wasm`, `witness_worker`) no longer depend on `tlog_tiles` at all after the migration. Slice 3 (move entries.rs into generic_log_worker) is deferred to a follow-up.
|
/bonk review |
There was a problem hiding this comment.
Review
Mechanical extraction looks clean. All four pre-push checks pass locally on this branch:
cargo clippy --workspace --all-targets -- -Dwarnings -Dclippy::pedanticcargo test(with default-members;integration_testsandfuzzexcluded as documented)cargo fmt --all --checkcargo machete
Plus cargo check -p witness_worker --target wasm32-unknown-unknown is clean.
Confirmed
- The new
tlog_checkpointcrate'sCargo.tomlonly pulls deps it actually uses; the corresponding deps drop intlog_tilesare real (no stale workspace pulls). tlog_tilesno longer depends oned25519-dalek,rand,signed_note, orthiserror— its only consumers ofHash/checkpoint types now go throughtlog_coreandtlog_checkpoint.tlog_tiles_wasm(Cargo.toml) really has notlog_tilesdep anymore — onlytlog_core+tlog_checkpoint. Thecrates/tlog_tiles_wasmdirectory name is now misleading, but the PR description acknowledges this is intentional pending the slice-3 / WASM rebinding plan in #230.UnixTimestamp→UnixTimestampMillisrename is workspace-wide:rg '\bUnixTimestamp\b'returns no matches.- The seconds boundary in
cosignature_v1/subtree_v1cosigners still does/ 1000and uses localtimestamp_unix_secsnames, exactly as the PR description claims (crates/tlog_cosignature/src/cosignature_v1.rs:44,subtree_v1.rs:333area). - The
git mvofcheckpoint.rs→tlog_checkpoint/src/lib.rspreserves the existing unit tests (test_parse_checkpoint,test_sign_verify); the +44/-17 in that file is the new module doc-comment header (lines 14-55), the newUnixTimestampMillisdoc, and thetlog_core::record_hashimport path in tests. CheckpointErrorcarries forward from slice 1 with the same variants; theMalformedCheckpoint(#[from] MalformedCheckpointTextError)andTlog(#[from] TlogError)conversions are preserved.
Nits (non-blocking)
Inconsistent parameter name in one trait impl. The PR description explicitly justifies dropping the _unix_millis suffix from CheckpointSigner::sign parameter names ("Putting the unit in the type name makes the contract explicit and lets parameter names drop the redundant _unix_millis suffix"). Every sign impl in the workspace uses timestamp (or _timestamp when unused) except crates/bootstrap_mtc_api/src/cosigner.rs:96, which still has _timestamp_unix_millis: UnixTimestampMillis. Cosmetic only — the parameter is unused and prefixed with _, so no caller sees it — but worth aligning before pushing for full consistency:
- _timestamp_unix_millis: UnixTimestampMillis,
+ _timestamp: UnixTimestampMillis,README typo on MalformedCheckpointError. crates/tlog_checkpoint/src/lib.rs:236 references [MalformedCheckpointError] in the # Errors doc for CheckpointText::new, but the actual type is MalformedCheckpointTextError. Pre-existing in slice 1 (the rustdoc link silently no-ops since it doesn't resolve), but worth fixing while the file is being touched:
- /// Returns a [`MalformedCheckpointError`] if the arguments do not comply with
+ /// Returns a [`MalformedCheckpointTextError`] if the arguments do not comply withStats sanity
PR body says "+355 / −141, 43 files changed" — git diff ae9e983..HEAD --stat confirms exactly 355 insertions / 141 deletions across 43 files. Test count claim ("31 test result blocks ok, 2 more than slice 1 from tlog_checkpoint's relocated unit tests") matches: the tlog_checkpoint crate's two tests (test_parse_checkpoint, test_sign_verify) now appear as their own running 2 tests block in the test runner output.
Boundary check
The crate split keeps Worker-specific concerns out of tlog_checkpoint: no worker / KV / Durable Object types appear, only signed_note, tlog_core, ed25519-dalek, rand, sha2, base64, thiserror. Aligns with the AGENTS.md boundary guidance for spec-level published crates.
LGTM modulo the two nits above.
|
Review posted to PR #233: #233 Summary of findings: All four pre-push checks pass locally (clippy pedantic, cargo test, fmt, machete) plus the WASM build. The crate extraction is mechanically clean and matches the PR description exactly:
Two non-blocking nits flagged:
|
Second slice of #230. Move the c2sp.org/tlog-checkpoint signed-note format out of
tlog_tilesinto its owntlog_checkpointcrate, so each crate now matches the C2SP spec slug it implements.Stacked on PR #231 (slice 1:
tlog_coreextraction). Diff against #231 is the new content here; once #231 lands this PR's base will be retargeted tomain.What moves
CheckpointText,MalformedCheckpointTextErrortlog_tiles::checkpointtlog_checkpointCheckpointSignertrait,Ed25519CheckpointSignertlog_tiles::checkpointtlog_checkpointTreeWithTimestamptlog_tiles::checkpointtlog_checkpointopen_checkpointtlog_tiles::checkpointtlog_checkpointCheckpointError(introduced in slice 1)tlog_tiles::checkpointtlog_checkpointUnixTimestamptlog_tiles::entriestlog_checkpointWhat stays in
tlog_tilestile.rs(the c2sp.org/tlog-tiles HTTP wire format itself) andentries.rs(theLogEntry/PendingLogEntryabstractions). Slice 3 of #230 will moveentries.rsintogeneric_log_worker.Bonus: rename
UnixTimestamp→UnixTimestampMillisEvery callsite in the workspace uses milliseconds (constructed via
now_millis(), namedtimestamp_unix_millis: UnixTimestampto clarify the unit). Thesubtree_v1/cosignature_v1cosigners that produce POSIX-seconds in the wire format already do/ 1000at the spec boundary.Putting the unit in the type name makes the contract explicit and lets parameter names drop the redundant
_unix_millissuffix:timestamp_unix_secslocal variable names are preserved at the cosigner spec boundaries since those are in seconds.Cleanups in
tlog_tilesed25519-dalek,rand,signed_note,thiserror.Cargo.tomlshortened to mention onlyc2sp.org/tlog-tiles.Consumers no longer depending on
tlog_tilesat allAfter the migration:
tlog_witnesstlog_cosignaturetlog_tiles_wasm(despite the name; it now wrapstlog_core+tlog_checkpointexclusively, since none of the tile-encoding types are exposed to JS; see Split tlog_tiles crate so it implements only c2sp.org/tlog-tiles #230 for the WASM rebinding plan)witness_workerEach had been transitively pulling in tile-encoding types and dependencies (
length_prefixed,url, etc.) only to get atHashorCheckpointSigner.Stats
43 files changed, +355 / −141 (vs slice-1 head). The
checkpoint.rsmove isgit mvwith 92% similarity. The bulk of the +355 istlog_checkpoint'sCargo.toml/LICENSE/READMEplus the mechanical rename across the workspace.Pre-push checks
All four pass:
cargo clippy --workspace --all-targets -- -Dwarnings -Dclippy::pedanticcargo test(31 test result blocks ok, 2 more than slice 1 fromtlog_checkpoint's relocated unit tests)cargo fmt --all --checkcargo macheteWASM build also clean (
cargo check -p witness_worker --target wasm32-unknown-unknown).Refs #230.