docs(audit): record six verified-rejected findings; correct the #693 invariant - #838
Conversation
…invariant Closes #691, #700, #702, #703, #682, #721 — all triaged as not reachable, by-design, or hygiene, and closed on GitHub with the same rationales. Adds two sections: - Trust-boundary findings (#682 lazy-loaded Merk metadata, #700 delete_checkpoint) — corrupt local storage and caller-chosen paths are inside the trust boundary; integrity auditing is delegated to Merk::verify / verify_grovedb on purpose. The #682 entry also records the argument against "fixing" it: validating hash_for_link on every link load adds hash_node_calls to the hot read path, and cost changes are replay-critical. - Version-gating and build hygiene (#702 GroveVersion::default, #703 missing version gates, #721 grovedbg build download). #691 joins the existing proof/encoding section: a dense root-only proof does verify in isolation, but every in-repo verifier either binds it to a query or discards entries and forces the root to the parent-committed hash. Also corrects a factual error that several entries leaned on. The overflow section claimed element counts are "hash-verified before the arithmetic runs". They are not: the non-Merk lower-layer verifiers are dispatched with an Element decoded from the proof's own value_bytes, and combine_hash binds it only afterwards. #693 stays closed on impact — the release build wraps and the chain check rejects the result — but the stated reason was wrong, and a build with overflow-checks enabled panics rather than wrapping. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 7 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
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. Comment |
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Found three documentation-correctness issues. Most importantly, the corrected #693 rationale still identifies the wrong verifier invariant: the validated bulk-tree height bounds completed_chunks below the multiplication overflow threshold, so the asserted release wrap and overflow-check panic cannot occur. The transaction and optional-dependency statements also need narrower wording. Focused bulk-append, dense-tree, and version tests passed (367 tests), and git diff --check passed.
| decoded from the proof's own `value_bytes`, and only at `verify.rs:2018` | ||
| does `combine_hash(value_hash(value_bytes), &lower_hash)` check it against | ||
| the parent-committed hash. A forged count therefore *does* reach the math; | ||
| what makes that safe is that the release build wraps and the chain check |
There was a problem hiding this comment.
[P2] Use the actual verifier bound for #693
verify_and_compute_root rejects height outside 1..=16 before this calculation. Therefore chunk_item_count = 2^height is at least 2, so for every u64 total_count, completed_chunks <= floor(u64::MAX / 2) = 2^63 - 1. Consequently 2 * completed_chunks - popcount(completed_chunks) cannot overflow. Neither release wrapping nor an overflow-checks = true panic is reachable on this verifier path. Please document this bound and remove the wrap/panic assertions; as written, the audit guide records behavior that cannot occur and the unchanged “Becomes real if” rule immediately matches the newly described unauthenticated input.
| | Issue | Claim | Why not real | | ||
| |---|---|---| | ||
| | [#702](https://github.com/dashpay/grovedb/issues/702) (closed) | `GroveVersion::default` creates protocol version 0, satisfying v0 gates | Inert on two independent grounds. (1) `protocol_version` is **never read by any logic in the workspace** — grep finds it only in the four version-constant definitions and in test assertions; every gate reads a *feature* slot such as `grove_version.grovedb_versions.operations.insert.*`. (2) `GroveVersion::default()` is behaviorally identical to `GROVE_V1`: every `FeatureVersion` in V1 is `0`, and the only non-zero values in `v1.rs` are `protocol_version: 1` (never read) and `max_aggregate_sum_query_elements_scanned: 1024`, which is not a version slot and whose hand-written `impl Default` returns the same 1024. So the worst outcome is "the caller got V1 behavior", which is already legal via an explicit `GROVE_V1`. | | ||
| | [#703](https://github.com/dashpay/grovedb/issues/703) (closed) | Public versioned APIs miss explicit version gates and could execute writes instead of returning `VersionError` | Three of the seven cited refs are stale line numbers resolving to `root_key`, `root_hash` and `verify_grovedb` — two accessors and a verification helper, none version-dependent, none a write path; gating them would break callers on any unrelated slot bump. The four typed non-Merk append entry points (`mmr_tree_append`, `bulk_append`, `dense_tree_insert`, `commitment_tree_insert`) do lack a *top-level* gate but already fail closed deeper and **before any mutation**, via their cost dispatchers (`grovedb-merkle-mountain-range/src/cost/mod.rs:88-98`, `grovedb-bulk-append-tree/src/cost/mod.rs:182-201`, `grovedb-dense-fixed-sized-merkle-tree/src/tree/root_maintenance/mod.rs:56-71`, `grovedb-commitment-tree/src/commitment_tree/cost/mod.rs:43,73`). Writes also land in a `StorageBatch` committed only on success. The genuine residual is narrower and different: those four types have no `element_creation` gate of the kind `PrivateDocumentStore` uses (`grovedb_versions.rs:180-186`, enabled only in `v4.rs:407`) — a design-consistency question about when they become *creatable*, worth filing separately if wanted. | |
There was a problem hiding this comment.
[P2] Qualify the StorageBatch atomicity claim
The direct typed append operations commit their child data_batch into the transaction before updating the parent Merk; the source comments explicitly say a later parent-update failure leaves orphaned subtree data in the transaction and that a caller-supplied transaction must be rolled back. For example, mmr_tree_append can pass the MMR cost gate, stage and commit its child writes, and then fail a later parent insert_subtree version gate. The early lower-layer dispatcher supports the narrow argument, but “committed only on success” is not generally true. Please qualify this to locally owned transactions/the caller rollback contract, or remove the sentence and narrow the “Becomes real if” condition.
| |---|---|---| | ||
| | [#702](https://github.com/dashpay/grovedb/issues/702) (closed) | `GroveVersion::default` creates protocol version 0, satisfying v0 gates | Inert on two independent grounds. (1) `protocol_version` is **never read by any logic in the workspace** — grep finds it only in the four version-constant definitions and in test assertions; every gate reads a *feature* slot such as `grove_version.grovedb_versions.operations.insert.*`. (2) `GroveVersion::default()` is behaviorally identical to `GROVE_V1`: every `FeatureVersion` in V1 is `0`, and the only non-zero values in `v1.rs` are `protocol_version: 1` (never read) and `max_aggregate_sum_query_elements_scanned: 1024`, which is not a version slot and whose hand-written `impl Default` returns the same 1024. So the worst outcome is "the caller got V1 behavior", which is already legal via an explicit `GROVE_V1`. | | ||
| | [#703](https://github.com/dashpay/grovedb/issues/703) (closed) | Public versioned APIs miss explicit version gates and could execute writes instead of returning `VersionError` | Three of the seven cited refs are stale line numbers resolving to `root_key`, `root_hash` and `verify_grovedb` — two accessors and a verification helper, none version-dependent, none a write path; gating them would break callers on any unrelated slot bump. The four typed non-Merk append entry points (`mmr_tree_append`, `bulk_append`, `dense_tree_insert`, `commitment_tree_insert`) do lack a *top-level* gate but already fail closed deeper and **before any mutation**, via their cost dispatchers (`grovedb-merkle-mountain-range/src/cost/mod.rs:88-98`, `grovedb-bulk-append-tree/src/cost/mod.rs:182-201`, `grovedb-dense-fixed-sized-merkle-tree/src/tree/root_maintenance/mod.rs:56-71`, `grovedb-commitment-tree/src/commitment_tree/cost/mod.rs:43,73`). Writes also land in a `StorageBatch` committed only on success. The genuine residual is narrower and different: those four types have no `element_creation` gate of the kind `PrivateDocumentStore` uses (`grovedb_versions.rs:180-186`, enabled only in `v4.rs:407`) — a design-consistency question about when they become *creatable*, worth filing separately if wanted. | | ||
| | [#721](https://github.com/dashpay/grovedb/issues/721) (closed) | `grovedbg` build script downloads a release artifact at build time | Acceptable posture. Gated off by default (`default = ["full", "estimated_costs"]`; with `grovedbg` off, `build.rs` compiles to a literal no-op and `reqwest`/`sha2` are not even resolved), and integrity is enforced by a pinned version tag plus a pinned `GROVEDBG_SHA256` asserted at `build.rs:33`. A substituted, tampered, or 404 artifact fails the build — fail-closed. Real but minor DX defect not named in the issue: the `if !grovedbg_zip_path.exists()` guard at `:15` caches a bad download, so every later rebuild fails the SHA assert without re-fetching until `target/` is cleared. | |
There was a problem hiding this comment.
[P3] Say optional dependencies are not activated, not unresolved
Cargo still resolves optional dependencies while producing the workspace lockfile even when their features are disabled. A feature-off focused test run here generated a Cargo.lock containing both reqwest and sha2; they are absent from the active feature-off build graph, so the accurate claim is that they are not activated, built, or executed. “Not even resolved” overstates the default-feature isolation.
1 participant
Records the rejected findings #691, #700, #702, #703, #682, and #721, with their caller and trust-boundary assumptions, so later audits can distinguish duplicate claims from different failure modes.
Corrects the #693 rationale: non-Merk verification can consume proof-supplied metadata before parent authentication, but the bulk/commitment completed-chunk conversion is bounded independently. The validated height is in
1..=16, socompleted_chunks = total_count / 2^height <= 2^63 - 1for everyu64 total_count. This conversion cannot overflow; its safety does not depend on release wrapping.Clarifies two other contracts:
grovedbgleaves its build script inactive and its optional build dependencies unactivated through that feature. Those dependencies still participate in workspace lockfile resolution.Rejections are scoped to the documented claim, caller, input trust boundary, and applicable version. Independent MMR arithmetic, pagination, and resource use before authentication require separate analysis.
Documentation only. Validation: checked the corrected statements against the PR-head source and Cargo resolver documentation; verified the patch with
git diff --check. No new runtime test run was needed for this prose-only update.