Extract LogEntry/PendingLogEntry traits into a new tlog_entry crate#234
Conversation
|
/bonk review |
There was a problem hiding this comment.
Reviewed slice 3 of #230 (the diff against #233's head, since this is stacked).
Summary
Mechanical extraction of LogEntry / PendingLogEntry and friends from tlog_tiles into a new internal tlog_entry crate. The entries.rs → tlog_entry/src/lib.rs move is a git mv with the only content changes being (a) a new module-level doc comment explaining why the crate exists, and (b) use crate::PathElem → use tlog_tiles::PathElem. Confirmed by diffing 9c1c99a:crates/tlog_tiles/src/entries.rs against 724bd29:crates/tlog_entry/src/lib.rs.
Architectural choice
The "why a new crate (and not generic_log_worker)" framing in the PR body is convincing. generic_log_worker already depends on the spec crates that implement these traits, so housing the trait definitions there would create a cycle; pushing them into any single spec crate would force every other spec crate to depend on it. A small publish = false interface crate is the cleanest break, and the boundary aligns with AGENTS.md's rule about keeping worker-specific concerns out of the spec-level crates — tlog_entry is explicitly the worker↔spec interface, not a published spec.
The end state where each remaining spec crate maps 1:1 to its C2SP slug (tlog_core → RFC 6962 + MTC subtree math, tlog_checkpoint → c2sp.org/tlog-checkpoint, tlog_tiles → c2sp.org/tlog-tiles HTTP wire format) is a noticeable improvement over the pre-#230 status quo.
What tlog_tiles becomes
After this slice, tlog_tiles is just tile.rs (HTTP wire format types). Production deps shrink to tlog_core only — length_prefixed, serde, sha2, tlog_checkpoint all dropped, and base64 / serde / serde_json / url retained only as dev-deps for the CT integration test in lib.rs. Spot-checked tile.rs for residual usage of serde / tlog_checkpoint: none. The dep diet is genuine.
Migration consistency
All 11 import-rewrite sites (bootstrap_mtc_api, bootstrap_mtc_worker ×3, ct_worker ×3, generic_log_worker ×4, integration_tests, static_ct_api) consistently switch to tlog_entry::{...}. Where a file needs both moved trait/type symbols and the still-in-tlog_tiles PathElem / tile-reader symbols, the imports are correctly split (e.g. bootstrap_mtc_api/src/lib.rs:29-30, static_ct_api/src/static_ct.rs:124-125, generic_log_worker/src/log_ops.rs:44-45, integration_tests/src/assertions.rs:19-20). No stragglers found.
bootstrap_mtc_worker/Cargo.toml and ct_worker/Cargo.toml correctly drop tlog_tiles entirely — confirmed by grep that neither worker references tlog_tiles:: anywhere after the rewrite.
Dep ordering nit
In bootstrap_mtc_api, bootstrap_mtc_worker, ct_worker, generic_log_worker, and static_ct_api the new line lands as tlog_core / tlog_entry / tlog_checkpoint / tlog_tiles, which isn't strict alphabetical but matches the pre-existing logical order (tlog_core before tlog_checkpoint was already there). integration_tests/Cargo.toml is the one place the slot is alphabetised (tlog_checkpoint / tlog_core / tlog_cosignature / tlog_entry / tlog_tiles / tlog_witness). Worth a follow-up to converge on one convention, but not a blocker for this PR.
Pre-push checks
Re-ran all four locally on 724bd29:
cargo clippy --workspace --all-targets -- -Dwarnings -Dclippy::pedantic— clean.cargo test— 33test result: ok.blocks, 0 failures (matches the PR body claim exactly; the +2 vs slice 2 istest_parse_tile_entryshowing up in the newtlog_entrycrate plus its doctest module).cargo fmt --all --check— clean.cargo machete— clean (no unused deps; thetlog_entryworkspace dep is actually used in every crate that declares it).
Verdict
LGTM. Mechanical, well-scoped, and motivated by a real cycle constraint. Once #231/#233 land and this gets rebased onto main, it should merge cleanly.
|
Posted the review on PR #234. Summary of findings:
|
9c1c99a to
062e089
Compare
Slice 3 of #230. The traits and helpers that lived in `tlog_tiles/src/entries.rs` aren't part of the c2sp.org/tlog-tiles HTTP wire format \u2014 they're a worker-side abstraction that `generic_log_worker` consumes and that the various per-application data types (CT entries, MTC certificates, etc.) implement. Putting them in any one of those crates creates a dependency cycle: `generic_log_worker` already depends on `static_ct_api` and `bootstrap_mtc_api`, so moving the trait definitions to `generic_log_worker` would force the spec-level data types to depend on a worker crate. Solution: a new internal crate `tlog_entry` (`publish = false`) that holds the traits + the c2sp.org/tlog-tiles default entry-bundle implementation, and that every spec crate plus `generic_log_worker` depends on. What moves to `tlog_entry`: - `PendingLogEntry`, `LogEntry` traits (the worker / spec interface). - `PendingLogEntryBlob` (DO storage shape). - `LookupKey`, `LOOKUP_KEY_LEN` (cache/dedup key). - `TileIterator` (helper for parsing entry-bundle tiles). - `TlogTilesPendingLogEntry`, `TlogTilesLogEntry` \u2014 the default c2sp.org/tlog-tiles entry-bundle implementation. Co-located with the trait it implements; no current worker uses this directly but `bootstrap_mtc_api` wraps `TlogTilesPendingLogEntry` as a field, and the upcoming `ietf_mtc_worker` will use it too. What stays in `tlog_tiles`: - Just `tile.rs`: the c2sp.org/tlog-tiles HTTP wire format (`Tile`, `TlogTile`, `PathElem`, `TileReader`, `TileHashReader`, `TlogTileRecorder`, `PreloadedTlogTileReader`). - The crate now matches its spec slug exactly. Production-deps shrunk to just `tlog_core`; `length_prefixed`, `serde`, `sha2`, `base64`, `url` are gone (`base64` / `serde` / `url` retained as dev-deps for the CT integration test in `lib.rs`). Migration: `bootstrap_mtc_api`, `bootstrap_mtc_worker`, `ct_worker`, `generic_log_worker`, `integration_tests`, `static_ct_api` now import `LogEntry`/`PendingLogEntry`/etc. from `tlog_entry` directly. `bootstrap_mtc_worker` and `ct_worker` no longer depend on `tlog_tiles` at all (they were only pulling it in for the trait definitions). This closes the migration plan in #230. Slices 1\u20133 together leave each spec crate aligned with its C2SP slug: - `tlog_core` \u2192 RFC 6962 Merkle math + draft-ietf-plants-merkle-tree-certs subtree proofs. - `tlog_checkpoint` \u2192 c2sp.org/tlog-checkpoint signed-note format. - `tlog_tiles` \u2192 c2sp.org/tlog-tiles HTTP wire format. - `tlog_entry` \u2192 internal worker/spec interface (`publish = false`).
724bd29 to
e376df2
Compare
Third slice of #230. Move
LogEntry/PendingLogEntrytraits + helpers (and the c2sp.org/tlog-tiles default entry-bundle implementation) out oftlog_tilesinto a new internaltlog_entrycrate. After this, each spec crate matches its C2SP slug exactly.Stacked on PR #233 (slice 2:
tlog_checkpoint). Diff against #233 is the new content here; once #231/#233 land this PR's base will be retargeted tomain.Why a new crate (and not
generic_log_worker)The natural framing of #230's slice 3 was "move worker abstractions to
generic_log_worker". On closer reading, the situation is more nuanced: the traits are a worker ↔ spec interface, with multiple consumers on each side:tlog_tiles(the default tlog-tiles entry-bundle),static_ct_api,bootstrap_mtc_api, soonietf_mtc_api.generic_log_worker.generic_log_workeralready depends onstatic_ct_api/bootstrap_mtc_api, so moving the trait definitions there would create a cycle. Putting them in any single spec crate forces every other spec crate to depend on it. A small interface crate (tlog_entry,publish = false) breaks the tangle cleanly.What moves to
tlog_entryPendingLogEntry,LogEntrytraitstlog_tiles::entriesPendingLogEntryBlobtlog_tiles::entriesLookupKey,LOOKUP_KEY_LENtlog_tiles::entriesTileIteratortlog_tiles::entriesTlogTilesPendingLogEntry,TlogTilesLogEntrytlog_tiles::entriesCrate has
publish = false. Deps:length_prefixed,serde,sha2,tlog_checkpoint,tlog_core,tlog_tiles.What stays in
tlog_tilesJust
tile.rs— the c2sp.org/tlog-tiles HTTP wire format itself (Tile,TlogTile,PathElem,TileReader,TileHashReader,TlogTileRecorder,PreloadedTlogTileReader).Production deps shrink to just
tlog_core.length_prefixed,serde,sha2dropped entirely;base64/urlretained only as dev-deps for the CT integration test inlib.rs.End state of #230
After slices 1–3 ship, each spec crate is aligned with the C2SP slug it implements:
tlog_core→ RFC 6962 Merkle math + draft-ietf-plants-merkle-tree-certs subtree proofs.tlog_checkpoint→ c2sp.org/tlog-checkpoint signed-note format.tlog_tiles→ c2sp.org/tlog-tiles HTTP wire format.tlog_entry→ internal worker/spec interface (publish = false).Migration
Every consumer of the moved symbols (
bootstrap_mtc_api,bootstrap_mtc_worker,ct_worker,generic_log_worker,integration_tests,static_ct_api) now imports fromtlog_entrydirectly.bootstrap_mtc_workerandct_workerno longer depend ontlog_tilesat all (they were only pulling it in for the trait definitions).Stats
25 files changed, +109 / −44 (vs slice-2 head). The
entries.rsmove isgit mvwith 87% similarity (the diff is the new module doc-comment header + thetlog_tiles::PathElemimport).Pre-push checks
All four pass:
cargo clippy --workspace --all-targets -- -Dwarnings -Dclippy::pedanticcargo test(33 test result blocks ok, 2 more than slice 2 fromtlog_entry's relocatedtest_parse_tile_entry)cargo fmt --all --checkcargo macheteCloses #230.