CSHLD-1208: Add retention flags when appending commitments in Merkle tree logic#319
Conversation
| import com.fasterxml.jackson.databind.JsonNode; | ||
| import com.fasterxml.jackson.databind.ObjectMapper; |
There was a problem hiding this comment.
where does json come from here? I though we were using protobuf for WASM communication?
There was a problem hiding this comment.
This isn't WASM communication - tree.save() returns the internal Rust PersistedShardTreeState serialized by the shardtree crate via serde_json. We're just parsing it in the test to assert retention flags on individual leaves.
Replaced JsonNode with Jackson-annotated classes
31b7b1b to
bbd8fba
Compare
OttoAllmendinger
left a comment
OttoAllmendinger
left a comment
There was a problem hiding this comment.
Overall solid, focused change with CI green. Left three comments: a silent-truncation risk in the owned vs commitments length handling, a breaking API/README-staleness note on the removed Java overloads, and a suggestion to move the retention-flag test coverage into Rust rather than asserting on the (supposedly opaque) persisted JSON from Java.
Generated by Claude Code
| marking: Marking::None, | ||
| marking: if is_owned { | ||
| Marking::Marked | ||
| } else { |
There was a problem hiding this comment.
No length validation between owned and commitments: owned.get(i).copied().unwrap_or(false) silently treats an out-of-range index as "not owned". That's fine for a shorter/absent owned vec (matches the proto doc), but a caller passing a longer or misaligned owned vec (e.g. an off-by-one upstream) fails silently instead of erroring, and could mis-mark ownership for a real block. Consider validating owned.len() == commitments.len() (or <=) up front and returning an Err otherwise.
Also: there's no Rust-level unit test for this logic (there are no #[cfg(test)] tests anywhere in tree.rs/this crate). The new retention-flag behavior is only exercised indirectly through Java, via JSON round-trip through the WASM boundary — see the review comment on ShieldedMerkleTreeTest.java for why that's the wrong layer to test this in.
Generated by Claude Code
| long blockHeight, List<ShieldedCommitment> commitments, ShieldedRoot expectedRoot) { | ||
| long blockHeight, List<ShieldedCommitment> commitments, List<Boolean> owned, | ||
| ShieldedRoot expectedRoot) { | ||
| requireU32(blockHeight, "blockHeight"); |
There was a problem hiding this comment.
Removing the 2-arg and 3-arg appendCommitments overloads is a breaking API change for any external caller. Worth calling out explicitly in the PR description (not just implied by the diff), and confirming downstream consumers are updated in lockstep.
Separately: packages/wasm-privacy-coin/README.md (lines ~163-164, 278, 280, 290, 302, 316-317) still documents and uses the old 2-arg/3-arg signatures — those examples no longer compile against this new signature and should be updated in this PR.
Generated by Claude Code
| * <ul> | ||
| * <li>CMX (i=0, owned, not last) → f=2 MARKED | ||
| * <li>CMX_2 (i=1, not owned, not last) → f=0 EPHEMERAL | ||
| * <li>CMX_3 (i=2, owned, last) → f=3 CHECKPOINT|MARKED |
There was a problem hiding this comment.
These new tests assert on the persisted tree JSON's internal shape (shard/cap/Leaf/Parent nodes, f/h fields) via a hand-rolled Jackson POJO hierarchy — but TreeState's own doc explicitly says "the internal encoding is an implementation detail; do not interpret the bytes directly." Testing it from Java means the assertion travels through JSON serialization, the WASM ABI, and protobuf/Jackson decoding — none of which is the thing actually being verified (retention-flag assignment in tree.rs), and any of which could mask or introduce unrelated failures. It also means a shardtree crate serde format change surfaces as a confusing Java test failure two layers removed from the actual cause.
Suggest moving this coverage into a Rust unit test in tree.rs (there currently are none in this crate) that calls append_commitments and asserts directly on extract_state()'s returned struct (or the in-memory tree) — no JSON/WASM/Jackson round-trip required. The Java-side tests could then stay black-box (leaf count, root correctness) rather than reaching into the opaque internal format.
Generated by Claude Code
Per-commitment ownership booleans are now threaded from the proto wire format into the Rust shardtree append path. Owned leaves are assigned Retention::Marked so the shardtree preserves their witness paths; non-owned leaves remain Retention::Ephemeral (or Checkpoint for the last commitment in a block). Ticket: CSHLD-1208
bbd8fba to
4905aac
Compare
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
| <dependency> | ||
| <groupId>com.fasterxml.jackson.core</groupId> | ||
| <artifactId>jackson-databind</artifactId> | ||
| <version>2.17.0</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
actually now you don't need this any longer
2 participants
Summary
Threads per-commitment ownership booleans from the proto wire format through the Rust shardtree append path, assigning
Retention::Markedto owned leaves so the shardtree preserves their witness paths for future inclusion-proof generation.Linear: CSHLD-1208
Changes
proto/privacy_coin.proto: addrepeated bool owned = 4toAppendCommitmentsRequestsrc/zcash/tree.rs: acceptowned: Vec<bool>; assignMarking::Markedfor owned leaves,Marking::Noneotherwisesrc/lib.rs: extractreq.ownedand pass totree.append_commitmentsShieldedMerkleTree.java: addList<Boolean> ownedparameter toappendCommitments; removed backward-compatible overloadsShieldedMerkleTreeTest.java: add f-flag assertions verifyingMARKED(f=2) andCHECKPOINT|MARKED(f=3) on owned leavespom.xml: addjackson-databindtest-scoped dependency for JSON tree state parsing in testsTest Plan
make jarrebuilds the WASM binary with the Rust changesmake test-javarunsShieldedMerkleTreeTestincluding the two new retention-flag testsCLOSES TICKET: CSHLD-1208