Extract Merkle math from tlog_tiles into a new tlog_core crate#231
Extract Merkle math from tlog_tiles into a new tlog_core crate#231lukevalenta wants to merge 1 commit intomainfrom
Conversation
|
/bonk review |
Slice 1 of #230: the c2sp.org/tlog-tiles spec covers an HTTP wire format for tile-encoded transparency logs. Until now `tlog_tiles` also carried the underlying RFC 6962 Merkle math (`Hash`, `HashReader`, the proof builders/verifiers, the `Subtree` type + the draft-ietf-plants-merkle-tree-certs subtree variants). That math isn't part of `tlog-tiles` itself — it's the algorithm-only core that every transparency-log spec in this workspace builds on. This commit moves that nucleus into a new `tlog_core` crate and migrates every internal consumer to import from it directly. New crate `crates/tlog_core`: - Carries `Hash`, `HASH_SIZE`, `EMPTY_HASH`, `HashReader`, `Proof`, `Subtree`, `LeafIndex`, `record_hash`, `node_hash`, `stored_hash_index` + variants, `tree_hash` + variants, `subtree_hash` + variants, `inclusion_proof` + verifiers, `consistency_proof` + verifiers, the subtree variants of all proofs, `evaluate_subtree_inclusion_proof`, and `TlogError` (now scoped to math errors only). - Dependencies are `base64`, `serde`, `sha2`, `thiserror` only — no `ed25519-dalek`, `signed_note`, `length_prefixed`, `url`, `rand` dragged along. Cleanups in `tlog_tiles`: - New `CheckpointError` type local to `checkpoint.rs` covers `open_checkpoint` and `TreeWithTimestamp::sign` failure modes (`MissingVerifierSignature`, `InvalidTimestamp`, `OriginMismatch`, `Note(NoteError)`, `MalformedCheckpoint`, `Tlog(TlogError)`). Previously these were variants on `tlog_tiles::TlogError` even though they had nothing to do with the math layer. - `Ed25519CheckpointSigner::new` is now infallible (`-> Self` instead of `-> Result<Self, TlogError>`); it never actually errored. - `TlogTilesPendingLogEntry::ParseError` is now `std::io::Error` directly — it parses big-endian uint16 length-prefixed blobs and the IO error was the only failure path. The `test_tree` integration test (which exercises the math against tile-encoded storage) moved to `tlog_tiles/src/tile.rs` together with the `TestTilesStorage` helper, since it depends on `Tile`, `TileReader`, `TileHashReader`. Pure-math tests stayed in `tlog_core`. Migration: `tlog_witness`, `tlog_cosignature`, `static_ct_api`, `bootstrap_mtc_api`, `bootstrap_mtc_worker`, `ct_worker`, `generic_log_worker`, `witness_worker`, `tlog_tiles_wasm`, and `integration_tests` all import moved symbols from `tlog_core` directly. `tlog_witness` no longer depends on `tlog_tiles` at all. The `fuzz` crate is unchanged: it already used deep-path imports (`tlog_tiles::tile::Tile`, `tlog_tiles::checkpoint::CheckpointText`) that are still valid. Slices 2 (move `checkpoint.rs` to a new `tlog_checkpoint` crate) and 3 (move `entries.rs` into `generic_log_worker`) are deferred to follow-up PRs per the issue's staging plan.
099a81b to
ae9e983
Compare
|
/bonk review |
There was a problem hiding this comment.
Reviewed locally on lvalenta/tlog-crate-split (HEAD ae9e983).
Pre-push checks
All four pass cleanly on this branch:
cargo clippy --workspace --all-targets -- -Dwarnings -Dclippy::pedantic— cleancargo test— 29 result blocks, all ok; the 6 pure-math tests are present intlog_core::testsand the relocatedtest_treeruns intlog_tiles::tile::testscargo fmt --all --check— cleancargo machete— clean
What I verified
The split lines up with the PR description's table. Spot-checked importers and the new crate boundary holds:
tlog_core/Cargo.tomlonly depends onbase64,serde,sha2,thiserror— nosigned_note, nolength_prefixed, noworker. That matches the "transport-agnostic, no HTTP, no tile encoding, no checkpoint signing" docstring on the crate root and is exactly the boundaryAGENTS.mdcalls out for spec-level crates.tlog_witness/Cargo.tomlpreviously listedtlog_tiles.workspace = true; now it'stlog_core.workspace = trueandtlog_witness/src/add_checkpoint.rsimportstlog_core::{Hash, HASH_SIZE}directly. The crate-level rustdoc links totlog_core::verify_consistency_prooffor the consistency check the witness intentionally doesn't perform itself — and that link is now valid against the actual location of the function.- The math-only
TlogErrorvariants (InvalidProof,MalformedHash,InvalidTile,BadMath,RecordedTilesOnly,InconsistentTile,IndexesNotInTree,IndexesOutOfOrder,ConditionNotMet,InvalidBase64) stay ontlog_core::TlogError. The non-math variants (MissingVerifierSignature,InvalidTimestamp,OriginMismatch,Note,MalformedCheckpoint) move to a newtlog_tiles::CheckpointErrorwith#[from] TlogErrorfor the math-error case. This is a clean separation and the#[from]chain means?continues to work for theTreeWithTimestamp::sign/from_hash_readercall sites. Ed25519CheckpointSigner::newgoing fromResult<Self, TlogError>toSelf: the old body never returnedErr, so this is a purely API-shape cleanup. Confirmed againstgit show main:crates/tlog_tiles/src/checkpoint.rs.TlogTilesPendingLogEntry::ParseErrorgoing fromTlogErrortostd::io::Error: previously the only failure path wasread_length_prefixedreturning io error, wrapped through the now-removedTlogError::IOvariant. Directstd::io::Erroris correct and removes a layer.- The
tree_size == 0behavioral change forverify_inclusion_proof(nowConditionNotMetrather thanInvalidProof) is documented in-place atcrates/tlog_core/src/lib.rs:493-498and pinned bytest_verify_inclusion_proof_rejects_zero_tree_size. Grepped all in-tree callers — none match on the variant, and the onlytree_size: 0site is the test itself, so this is a safe strict-subset change. - The
test_treeintegration test correctly belongs intlog_tiles/src/tile.rsbecause it exercisesTile,TileReader, andTileHashReaderagainst the math layer; the standaloneTestHashStoragenewtype duplication intlog_core/src/lib.rs::testsandtlog_tiles/src/tile.rs::testsis unavoidable given orphan-rule constraints and is acceptable for test-only code. - The
fuzzcrate's deep-path imports (tlog_tiles::tile::Tile,tlog_tiles::checkpoint::CheckpointText) are unaffected — confirmedfuzzwas not touched and still builds clean under clippy.
Documentation / minor
crates/tlog_core/README.mdline 14 references[\tlog_checkpoint`](../tlog_tiles)— the link target is intentional (it lives intlog_tilestoday; the parenthetical "Currently still insidetlog_tiles`" makes that explicit). Worth re-checking when slice 2 lands so the link target updates.- The
tlog_corerustdoc mentions "MTC-related crates for the IETF Merkle Tree Certificates work" which is forward-looking —bootstrap_mtc_apiis the current consumer; fine as-is.
Stacking note
The PR description's claim that #228 / #229 are based on main and not on this branch matches what I'd expect — those are the witness PRs and don't depend on the import-path changes here. They will need a rebase after this lands so they pick up tlog_core::Hash (etc.) instead of tlog_tiles::Hash.
LGTM. The slice is mechanical for ~80% of the diff, the boundary cleanups (CheckpointError, Ed25519CheckpointSigner::new, TlogTilesPendingLogEntry::ParseError) are individually well-scoped, and the future-slice plan (#230) is concretely tracked in the README.
|
Review posted on PR #231. Summary of findings:
Verdict: LGTM. |
First slice of #230. Extract the RFC 6962 Merkle math from
tlog_tilesinto a newtlog_corecrate sotlog_tilesis closer to the c2sp.org/tlog-tiles HTTP wire format only.Closes neither slice 2 (
checkpoint.rs→tlog_checkpoint) nor slice 3 (entries.rs→generic_log_worker); those land in follow-ups.What moves
Hash,HASH_SIZE,EMPTY_HASH,Proof,HashReader,Subtreetlog_tiles::tlogtlog_coreLeafIndextlog_tiles::entriestlog_corerecord_hash,node_hash,stored_hash_*tlog_tiles::tlogtlog_coretree_hash+_indexes,subtree_hash+_indexestlog_tiles::tlogtlog_coreinclusion_proof+_indexes,subtree_inclusion_proof+_indexes,evaluate_subtree_inclusion_prooftlog_tiles::tlogtlog_coreconsistency_proof+_indexes,subtree_consistency_proof+_indexestlog_tiles::tlogtlog_coreverify_inclusion_proof,verify_subtree_inclusion_proof,verify_consistency_proof,verify_subtree_consistency_prooftlog_tiles::tlogtlog_coreTlogError(math-only variants)tlog_tiles::tlogtlog_coreWhat stays in
tlog_tilesfor nowtile.rs— the c2sp.org/tlog-tiles wire format itself.checkpoint.rs— c2sp.org/tlog-checkpoint (slice 2 will move it).entries.rs—LogEntry/PendingLogEntrytraits +UnixTimestamp/LookupKeyaliases (slice 3 will move worker abstractions).Cleanups in
tlog_tilesCheckpointErrortype foropen_checkpointandTreeWithTimestamp::sign. Variants:MissingVerifierSignature,InvalidTimestamp,OriginMismatch,Note(NoteError),MalformedCheckpoint,Tlog(TlogError). These were previously variants ontlog_tiles::TlogErroreven though they're not math errors.Ed25519CheckpointSigner::newis now infallible (-> Selfinstead of-> Result<Self, TlogError>); it never actually errored.TlogTilesPendingLogEntry::ParseErrorisstd::io::Errordirectly (wasTlogError); the io error was the only failure path.Crate-name choice
tlogis taken on crates.io by an unmaintained logging crate.tlog_corefollows the local workspace convention (snake_case) and conveys the role accurately ("core primitives below the spec crates"). Other candidates considered:merkle_tlog,cf_tlog,tlog_proofs.Migration
Every internal consumer (
tlog_witness,tlog_cosignature,static_ct_api,bootstrap_mtc_api,bootstrap_mtc_worker,ct_worker,generic_log_worker,witness_worker,tlog_tiles_wasm,integration_tests) imports moved symbols fromtlog_coredirectly.tlog_witnessno longer depends ontlog_tilesat all.The
fuzzcrate is unchanged — it already used deep-path imports (tlog_tiles::tile::Tile,tlog_tiles::checkpoint::CheckpointText).Test relocations
test_treeintegration test (math + tile encoding) moved fromtlog_tiles/src/tlog.rstotlog_tiles/src/tile.rstogether with theTestTilesStoragehelper, since it depends onTile/TileReader/TileHashReader.test_split_stored_hash_index,test_new_subtree,test_evaluate_subtree_inclusion_proof_rejects_out_of_range,test_verify_inclusion_proof_rejects_zero_tree_size,test_empty_tree,test_subtrees_split_interval) stayed intlog_core.Stats
42 files changed, +641 / −359. The bulk:
tlog_corecrate scaffolding (Cargo.toml,LICENSE,README).tlog.rs→tlog_core/src/lib.rs(file rename, ~80% similarity preserved pergit mv).test_treetest (~286 lines now intlog_tiles/src/tile.rs).LeafIndexrelocation + theCheckpointErrorrefactor.Pre-push checks
All four pass:
cargo clippy --workspace --all-targets -- -Dwarnings -Dclippy::pedanticcargo test(29 test result blocks ok, including all 6 pure-math tests intlog_coreand the relocatedtest_tree)cargo fmt --all --checkcargo macheteWASM build also clean (
cargo check -p witness_worker --target wasm32-unknown-unknown).Stacking
Independent of the in-flight witness PRs (#228, #229), based directly on
main. Those branches will need rebasing onto this one's eventual merge to pick up the import path changes.Refs #230.