Repository navigation
perf: hash arena identities without SipHash in content tables - #370
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 13 minutes. View limit details
 |
| Layer / File(s) | Summary |
|---|---|
Seeded collection aliases Cargo.toml, src/collections.rs, src/collections/tests.rs |
The standard Map and Set aliases use SeededState. structured-asn1 is updated and enabled through the std feature. Tests check hash consistency, distribution, and variation between default states. |
Collection initialization updates src/key_manager.rs, src/operation.rs, src/parser_xmloxide/validation/dtd.rs, src/sxd_document/lazy_hash_map.rs, src/sxd_xpath/..., src/xml.rs, src/xml/dom/..., src/xmldsig/verify.rs, src/xmldsig/verify/tests.rs, src/xmldsig/xpath/storage.rs, scripts/patches/xmloxide-0.5.0.patch |
Map and set initialization sites use Default or the project’s Map and Set aliases. The corresponding DTD validation patch changes its local set initialization to Default. |
Transform dependency handling src/xmldsig/transforms.rs, src/xmldsig/sign.rs, src/xmldsig/sign/single_execution_tests.rs |
Node-set transforms can run while dependencies are tracked, allowing later transforms to remove tracked nodes before dependency status is settled. Signing dependency sets use the shared Set type. A test checks signing and recomputed digest values. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Merge Risk: 
437f4
-
The change moves content-keyed tables to seeded hashing and fixes the false dependency-cycle error when signing with XPath, Enveloped and canonicalization transforms. One existing gap remains in this area. An XPath that reads a DigestValue and is followed by an Enveloped transform can still sign with a mismatched digest. It is not made worse by this PR, so it is worth fixing soon but is a bounded concern.
Pre-merge checks: 
-
Check name Status Explanation Resolution Out of Scope Changes check The hashing aliases, their constructors, and related call-site updates support #368. However,src/xmldsig/transforms.rsalso changes signing dependency analysis so node-set transforms continue after…Remove the signing dependency-analysis change and its dedicated test from this PR, or link the change to an active issue with its own coding requirements. Then reassess the PR scope. Docstring Coverage Docstring coverage is 38.46% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main performance change: using a non-SipHash strategy for hash-based content tables. Linked Issues check #368is the only active directly linked issue. Thestdaliases insrc/collections.rsnow usestructured_asn1::hash::RandomState, with a new seed for each table. The non-stdaliases remain `BTr…Full details: Linked Issues check
-
Explanation
#368is the only active directly linked issue. Thestdaliases insrc/collections.rsnow usestructured_asn1::hash::RandomState, with a new seed for each table. The non-stdaliases remainBTreeMapandBTreeSet. The collection tests cover equal hashes, strided integer-key spread, control-byte coverage, and independent table seeds. The changed constructors preserve the existing table behavior. The reported workspace and compatibility tests provide no contrary evidence for document acceptance, outputs, or typed errors. The requested before/after CPU measurement is a non-coding task and does not affect this coding assessment.
-
Full details: Out of Scope Changes check
-
Explanation
The hashing aliases, their constructors, and related call-site updates support
#368. However,src/xmldsig/transforms.rsalso changes signing dependency analysis so node-set transforms continue after dependency detection. The added signing test covers this behavior. This is a substantive signing behavior change, not a hashing implementation, supporting test, or required refactor for#368. The current description reports the change, but it does not link an active issue that requires it.Resolution
Remove the signing dependency-analysis change and its dedicated test from this PR, or link the change to an active issue with its own coding requirements. Then reassess the PR scope.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 38.46% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 13 functions across 18 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b00592acb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
aef391a to
714f52d
Compare
3b00592 to
4ec184d
Compare
714f52d to
50f52d6
Compare
4ec184d to
dc989d4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Content-keyed tables hashed every key with the randomly keyed SipHash, including the arena identities (document identity and slot) that XPath node-set deduplication stores for every selected node. - The table alias now uses a seeded hasher: integer words are mixed by a folded multiply under a random odd seed per table, byte strings and every word after them go through the randomly keyed SipHash in order, so neither form lets a document predict bucket positions - Tables are built with Default instead of the RandomState-only new - Without std the tables remain ordered trees Closes #368
- Content-keyed tables use `structured_asn1::hash::RandomState`, the shared seeded hasher (integer mix under a per-table seed, keyed SipHash for strings and every word after one); this crate keeps only the table aliases. structured-asn1 0.20.2 becomes a required dependency, since tables exist without signature support too - Dependency analysis stops a chain early only before a step that reads content: a node-set step such as Enveloped still runs, so it can drop the Signature's own values instead of a known dependency turning them into a false cycle (regression test with a value-reading XPath, then Enveloped and canonicalization)
dc989d4 to
437f493
Compare
The product now requires structured-asn1 0.20.2, so the competitor harness lockfile moves with it.
A value-reading XPath recorded as dependencies only the tracked values it excluded; a value it read and kept stayed a plain visible node, so a later Enveloped step removed it and the dependency with it. The XPath had selected nodes by the empty placeholder, a verifier selects them by the filled value, and the signed digest did not verify. Every tracked value such an XPath can read now stays a dependency, visible or not, so a Reference whose selection reads its own DigestValue is refused as a cycle. The regression test covers a predicate on the DigestValue length and a string(.) test, which reads it through every ancestor's string value; it replaces a test that expected the latter to sign.
Summary
structured_asn1::hash::RandomState; strings keep the randomly keyed SipHash, and a document still cannot predict bucket positions.Changes
Map/Setaliases for content-keyed tables usestructured_asn1::hash::RandomState(each table independently seeded; strings and every word after one go through the keyed byte hasher in order). This crate keeps only the aliases; tables are built withDefault.structured-asn10.20.2 is a required dependency, since content tables exist without signature support; the sharedstructured-*crates resolve to 0.20.2 together.stdthe tables remain ordered trees.Testing
Workspace tests with all features (including the xmlsec1 and libxslt oracles), the PKCS#11 suite, Clippy, feature-profile lints, the no-std targets, the fuzz build, the shared-crate graph check and the compatibility ledgers pass on macOS; Linux and Windows run in CI.
Closes #368