Repository navigation
fix(dpp)!: bound schema $ref walks that hang proposers and check_tx (PV14) - #5317
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughDocument-schema parsing now detects reference cycles. A versioned depth validator handles schema traversal. Create and update check transactions run depth validation for supported platform versions. ChangesDocument Schema Validation
Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Transition as Create or update transition
participant DepthCheck as validate_document_schemas_depth_for_check_tx
participant Definitions as Contract schema definitions
participant Validator as validate_max_depth_v1
Transition->>DepthCheck: validate document schemas
DepthCheck->>Definitions: enrich each document schema
DepthCheck->>Validator: validate enriched schema
Validator-->>DepthCheck: return depth validation result
DepthCheck-->>Transition: return validation result
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is established at the reviewed head. The schema-parsing and transaction-order safeguards appear to cover the reported hangs. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
✅ Final review complete — no blockers (commit 5a02c54) · triage: critical |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Static verification at a67319d found no in-scope blocking defect: the new depth-walk generation is selected at PV14, and both create and update mempool paths run the prefilter before schema expansion. Three test-quality suggestions remain, covering the activation boundary, update-path wiring, and safe regression failure. No builds or tests were run; the supplied CI snapshot still had test-workflow prerequisites queued and did not establish Rust validation results.
🟡 3 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate changes to validate_max_depth/v1/mod.rs, recursive schema parsing, and check_tx filtering alter consensus-sensitive schema acceptance and fee-related size calculation at PV14 while addressing exponential and cyclic traversal. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/data_contract/document_type/schema/validate_max_depth/v1/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/schema/validate_max_depth/v1/mod.rs:215-221: Exercise the historical/PV14 boundary through the public dispatcher
The comparison invokes the private v0 and v1 implementations with the same PV14 snapshot, so it verifies implementation parity but not activation routing. The new ABCI DAG test also does not distinguish the generations because its schema has no scalar references. Add a test through public validate_max_depth using PlatformVersion::get(13) and PlatformVersion::latest(), with a small scalar-reset fixture such as exponential_chain_schema(1). Assert the historical verdict and depth/size, then the new InvalidJsonSchemaRefError; also assert identical valid depth/size for an ordinary schema across the boundary. This pins the routing that controls consensus-visible rejection and size-based fees without exercising an expensive historical walk.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/contract_structure_error_tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/contract_structure_error_tests.rs:176-187: Keep the in-process DAG regression test safe when the guard fails
If the prefilter is removed or bypassed, this fixture asks the synchronous non-validating parser to materialize up to 2^40 leaf properties. The elapsed-time assertion runs only after check_tx returns, so it cannot interrupt expansion; the test process can hang or abort on allocation failure instead of reporting the expected-error assertion. The one-second threshold also measures the entire check_tx pipeline rather than just the walk. Use a smaller DAG, such as 12 levels, and retain the consensus-error assertions so a missing filter fails with bounded resource use. If the 40-level performance case is retained, isolate it in a subprocess with an externally enforced deadline rather than relying on a post-return timer.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs:471-484: New update-path depth pre-filter has no exponential-DAG test
The update path now runs the depth prefilter before constructing its action, but the new exponential-DAG regression exercises only contract creation. Existing update tests cover other structural errors and do not pin this new call: removing the update prefilter would leave those tests passing while restoring exponential expansion for updates. Add an update regression using a small branching $defs DAG, editing the serialized contract with resign_with_contract. Assert mempool rejection and the block's paid error with the expected contract-nonce bump, which differs from the create path.
Out-of-scope follow-up suggestions (3)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Bound schema expansion during proof-derived contract decoding — decode_proof_data_contract reconstructs contracts with full_validation=false, and Drive::verify_contract performs that reconstruction before proof.rs authenticates the computed root with verify_tenderdash_proof. A compact acyclic branching schema can therefore exhaust client resources before an attacker-controlled root is rejected; the bincode budget bounds deserialization, not subsequent property expansion. The decoding order and unbounded acyclic expansion already exist in the base, and this PR explicitly targets proposer/check_tx behavior without changing proof-derived contract loading.
- Follow-up: Track a separate security fix to bound proof-derived schema expansion or authenticate the proof root before constructing the contract, preserving historical contract compatibility.
- Preserve the depth-overflow consensus error and partial size accounting — The map-depth overflow branch returns a validation error without MaxDepthValidationResult data. validate_schema_depth_and_account_for_size then calls into_data(), producing CorruptedCodeExecution before returning the original consensus error or emitting its size operation. This mismatch is present in the base v0 implementation and unchanged accounting helper, so its block-execution and fee behavior needs separate versioned treatment.
- Follow-up: Track a separate versioned correction with an end-to-end depth-overflow rejection test and preserved historical behavior.
- Enforce bounded reference expansion below the ABCI-only caller — Out of scope as a blocker. The base already reconstructs proof-derived contracts with full_validation=false before root authentication, and its parser already expands acyclic branching references without a budget. This PR does not introduce that client entry point or newly permit the expansion; its stated scope explicitly leaves contract loads untouched. The concrete security issue is retained as a separate follow-up, not a request to redesign this PR.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
eede76a to
7fbb2b2
Compare
… (PV14) Two denial-of-service bugs in data contract schema parsing, each reachable with one funded identity and no fee paid. The schema depth validator kept a visited set of `$ref` targets but pushed scalar targets and cleared the set when one popped, so a `$defs` chain with `$ref`s to `#/$schema` was walked exponentially in block execution (28 s at 22 levels, doubling per level) and every proposer hung. Generation 1 of `validate_max_depth`, selected at protocol version 14, skips scalar targets and never clears the set, so every target is expanded at most once; the verdict and size feed fees, hence the version gate. Generation 0 is kept for replay. The non-validating parse used by check_tx resolved `$ref` with no cycle guard, so a self-referencing definition never returned and allocated until the node died. `insert_values` and `insert_values_nested` now refuse a target that repeats on the current ancestry path, which never fires on a schema the validating path accepts. An acyclic graph reaching one definition along many paths still expands exponentially there, so check_tx and recheck_tx run the (bounded) depth walk over every document type before parsing a create or update; it is mempool-only and refuses nothing a block accepts. Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
…est names Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
…refilter Exercise `validate_max_depth` through its dispatcher at protocol versions 13 and 14: a `$defs` level whose two refs to one definition are split by refs to a scalar is accepted with depth 5 / size 18 at 13 and refused as a cycle at 14, and an ordinary schema measures the same on both sides. Cover the update path's check_tx depth prefilter with the same branching `$defs` DAG the create test uses, now shared through the harness. Both DAG tests use 12 levels instead of 40, so a missing prefilter fails the check_tx assertion in about a second instead of hanging the test process on a 2^40 expansion; the post-return timer this made pointless is gone. The DAG contract also carries a type the non-validating parse refuses and the depth check accepts, so the tests fail if the prefilter ever runs after the parse rather than before it. Drop `resign_with_schemas`, which no test calls any more. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of the complete diff at 7fbb2b2 found no in-scope blockers: the depth-walk change is activated only at PV14, and both mempool prefilters run before contract parsing. All three prior findings are fixed; two non-blocking test-quality suggestions remain, while the proof-decoding exposure is a concrete pre-existing issue retained only as a separate follow-up. No builds or tests were run, and the supplied exact-head CI snapshot contains no completed build/test results.
🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate schema traversal and parser changes alter consensus-visible validation verdicts and fee-relevant sizes in validate_max_depth/v1/mod.rs, with protocol-14 dispatch and coordinated check_tx prefilters for contract creation and updates. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_common/contract_structure_test_harness.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_common/contract_structure_test_harness.rs:408-413: Require present balances before asserting the rejection was charged
Both balances are Option<u64>, and fetch_identity_balance returns None when the balance entry is absent. Because None compares less than Some, this assertion passes if rejection processing removes the owner's balance entry instead of deducting a fee. The new create and update DAG regressions would therefore accept missing balance state as successful fee charging. Require both balances to be present before comparing their numeric values.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs:6400-6412: Enforce the cycle regression deadline outside the parser process
The one-second assertion runs only after the synchronous parser returns. If the ancestry guard is removed or bypassed, these true-cycle fixtures repeatedly expand the same object in insert_values, growing property paths and traversal allocations without reaching the assertion. Unlike the reduced acyclic DAG fixtures, a smaller true cycle still has unbounded expansion. Run the end-to-end cycle cases in a child process with a parent-enforced deadline that kills and reaps it on expiry; bounded direct tests of enter_ref can supplement that coverage. A post-return stopwatch or an in-process timeout thread does not contain allocation failure.
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Bound proof-derived contract expansion before root authentication — The existing proof-decoding path reconstructs a contract before authenticating the computed root: Drive::verify_contract calls decode_proof_data_contract, which invokes DataContract::try_from_platform_versioned with full_validation=false, and only afterward does FromProof call verify_tenderdash_proof. A compact acyclic branching $ref graph can therefore expand outside the wire-size and bincode allocation budgets when supplied by a malicious DAPI peer. The same unbounded expansion and authentication ordering exist at base 937bf00; this PR neither introduces that caller nor broadens its accepted acyclic graphs, and its stated goal targets proposer and mempool validation.
- Follow-up: Track a separate security fix for bounded untrusted reconstruction or authentication before schema materialization, preserving trusted historical reconstruction and verify-only builds.
- Bound untrusted schema expansion below the mempool adapter — Out of scope as a blocker. The proof decoder and authenticate-after-reconstruction sequence are unchanged from the PR base, whose parser already expands acyclic branching graphs without a resource bound. The new ancestry guard does not create or enable this exposure, and fixing SDK proof reconstruction is not required for the stated proposer/check_tx protections to work. The concrete security concern is retained once as a separate follow-up.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
…ion is paid `None` orders below `Some`, so comparing the optional balances would also pass if the rejection removed the owner's balance entry instead of charging it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
7fbb2b2 to
d7c0caa
Compare
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At d7c0caa, the complete diff correctly isolates the consensus-visible depth change to PV14 and places the create/update mempool checks before schema expansion; no in-scope correctness blocker was verified. One replay-safety documentation suggestion remains, four prior test findings are fixed, and the subprocess-deadline suggestion is intentionally deferred. This was static verification only; the supplied exact-head CI snapshot contains pending or queued checks and does not establish completed validation.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate schema traversal and parsing changes alter consensus-visible validation verdicts and fee-relevant size calculations in validate_max_depth/v1/mod.rs, activated through the PV14 contract version table. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs:178-180: Document replay safety for the in-place parser and transformer edits
This ancestry guard and its nested counterpart are shared by shipped try_from_schema generations 0–2, selected across PV1–13. The PR also edits the create/update transform_into_action_v0 implementations. These edits have a replay-neutral justification: full validation rejects true cycles before property parsing, accepted stored contracts cannot contain those cycles, and the new ABCI prefilter returns without work in Validator mode. However, the guard comments describe termination rather than historical replay safety, and the supplied PR description lacks the "In-place changes to shipped generations" section required by book/src/contributing/coding-conventions.md. Add the replay-safety explanation beside the shared guards and document the affected generations and selecting protocol versions in that section, including why historical rejection, fee accounting, and stored-contract reconstruction remain unchanged. This records the exception without introducing unnecessary consensus generations.
Out-of-scope follow-up suggestions (3)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Client reconstruction still permits exponential acyclic reference expansion — A concrete pre-existing client denial-of-service path remains: Drive's decode_proof_data_contract reconstructs contracts with full_validation=false, and the proof verifier calls Drive::verify_contract before authenticating the returned root with verify_tenderdash_proof. A malicious endpoint can therefore supply a compact branching reference DAG whose property expansion occurs outside the bincode decode budget. The unproved latest-contract-versions response also reconstructs contracts without full validation. These callers and the exponential DAG expansion already exist at the PR base; this change targets proposer and mempool handling and does not introduce the client exposure.
- Follow-up: Track separate client-decoding hardening that bounds post-deserialization schema expansion before trusting endpoint-supplied contracts, with proof-derived and unproved-response coverage and compatibility for historical contracts.
- Enforce untrusted reconstruction bounds in DPP, not only ABCI — Out of scope as a blocking request. The unproved response's full_validation=false reconstruction and the unbounded acyclic property expansion are present at base bc41f1b, and this PR neither introduces that caller nor makes acyclic DAGs newly accepted. The stated repair targets block depth validation and check_tx/recheck_tx; extending the client reconstruction policy is a separate, concrete security follow-up retained in out_of_scope_findings.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
- Enforce untrusted reference-expansion safety in DPP — Out of scope as a blocking request. Base and head use the same bounded_decode.rs reconstruction with full_validation=false and the same proof.rs ordering of Drive::verify_contract before verify_tenderdash_proof. The ancestry guard preserves the base parser's acyclic DAG expansion rather than introducing this exposure. The verified client issue is consolidated into the separate security follow-up, not treated as a prerequisite for the proposer/mempool repair.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Independently reviewed the complete diff at 5a02c54 and found no in-scope findings requiring changes. The consensus-visible depth change is selected only at PV14, the shared ancestry guard preserves historical validated parsing, and the create/update prefilters are mempool-only; five prior findings are fixed and the external cycle-test deadline is intentionally deferred. This assessment is static: no builds or tests were run, and build/test prerequisites remained queued or pending in the supplied exact-head CI snapshot.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate changes to validate_max_depth/v1/mod.rs and its PV14 dispatch alter consensus-visible schema acceptance and fee-relevant size calculations while coordinating parser cycle guards with create/update check_tx prefilters. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Bound client-side schema expansion before proof authentication — The SDK proof-decoding path has a concrete, pre-existing resource-exhaustion boundary: decode_proof_data_contract bounds bincode decoding but then constructs the contract with full_validation=false. Drive::verify_contract materializes document properties before rs-drive-proof-verifier authenticates the computed root with verify_tenderdash_proof, so a compact branching-reference DAG can cause exponential expansion before an invalid signature is rejected. The same non-validating construction is available through WASM transition getters. These callers and their expansion behavior already exist at the PR base; this PR neither introduces nor worsens that exposure and specifically targets proposer and mempool admission.
- Follow-up: Track separate client hardening to bound semantic construction before root authentication, or authenticate the proof-derived bytes before materialization. Preserve trusted historical reconstruction rather than applying current registration rules indiscriminately.
- Preserve the payload needed for maximum-depth rejection accounting — The historical maximum-depth branch returns new_with_error without depth/size data, and generation 1 retains that behavior. The unchanged validate_schema_depth_and_account_for_size then calls into_data() on the invalid result, producing CorruptedCodeExecution before recording the schema-size operation instead of returning the intended paid depth error. This mismatch is present at the PR base and is distinct from the reference-expansion fixes.
- Follow-up: Track a separate versioned correction for maximum-depth rejection payloads and fee accounting, with end-to-end coverage and no change to shipped generation 0.
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
Issue being fixed or feature implemented
Two denial-of-service bugs in data contract schema parsing, both reachable with one funded identity and no fee paid:
Exponential
$refwalk in the schema depth validator (block execution).validate_max_depthv0 keeps avisitedset of resolved$reftargets to stop cycles, but it pushes a$reftarget even when it is a scalar and clearsvisitedwhenever a scalar pops. A$refto a scalar (for example#/$schema, which schema enrichment inserts) is therefore a user-controlled reset of the cycle guard. A$defschain where each level references the next one twice, with a scalar$refbetween the two, is walked twice per level: a 2.5 KB contract with 22 levels took 28 s and the time quadruples every two levels. check_tx does not run this check, so the transition reaches every mempool, and every proposer then hangs in PrepareProposal.Cyclic
$refon the non-validating parse (check_tx).insert_valuesandinsert_values_nestedresolve$refwith no cycle guard. Withfull_validation = false, which check_tx, recheck and contract loads use, a$defs.awhose properties both$refanever finishes and allocates until the node is killed.What was done?
validate_max_depthgets a generation 1 that does not walk a scalar$reftarget and never clearsvisited, so every ref target is expanded at most once and the walk is linear in schema size. The verdict and the returnedsizefeed fees, so the change is selected only at protocol version 14 (validate_max_depth: 1inCONTRACT_VERSIONS_V6); generation 0 is left as is for replay. For schemas without a scalar$reftarget the result is identical to generation 0. Listed as PV14 change 86 inv14.rs.insert_valuesandinsert_values_nestedcarry the$reftargets resolved on the current ancestry path and refuse a target that repeats on it. Only a true cycle trips the guard, so it never fires on a schema the validating path accepts; sibling references to one definition still parse.$defsgraph that reaches one definition along many paths is not a cycle but still expands exponentially on the non-validating parse. The validating walk already refuses that shape (any repeated ref target), socheck_txandrecheck_txnow run the same depth walk over every document type before parsing a contract create or update (validate_document_schemas_depth_for_check_tx). It is mempool-only, refuses nothing a block accepts, and runs from generation 1 of the walk only, since generation 0 is itself exponential. Contracts loaded from state are untouched.a -> b -> cchain still parse; a branching$defsDAG is refused bycheck_txand by the block with the same paid error, for both a contract create and a contract update. The DAG tests use 12 levels and carry a type the non-validating parse refuses, so a missing prefilter, or one moved after the parse, fails them in about a second instead of hanging the test process.How Has This Been Tested?
cargo test -p dpp --lib(new and existing tests),RUST_MIN_STACK=4194304 cargo test -p drive-abci --libfor the data contract create/update tests (226 pass; the harness needs CI's 4 MiB test stack),cargo clippy -p dpp -p drive-abci -p platform-version --tests -- -D warnings,cargo fmt --all -- --check.check_txreporting the parser's error rather than the cycle.In-place changes to shipped generations
insert_values/insert_values_nested(try_from_schema/mod.rs), shared by parser generations 0–3 (try_from_schema0 at PV1–8, 1 at PV9–11, 2 at PV12–13, 3 at PV14). They now carry the$reftargets on the current path and refuse one that repeats on it. Generations 0 and the sharedcommoncore only pass the new empty path. Why consensus cannot change:validate_max_depthbefore any property is parsed. Generation 0 of the walk refuses a true cycle too, either as a cycle or by exceeding the max depth, since each re-entry adds depth. So no schema that reaches the parser under full validation has a target repeating on its path, and the guard never fires there.full_validation = falsewere accepted under full validation, so they contain no such cycle and reconstruct exactly as before. Sibling references to one definition, which are not on each other's path, still parse.DataContractCreateTransition/DataContractUpdateTransitiontransform_into_action_v0(drive-abci), selected at every protocol version. They callvalidate_document_schemas_depth_for_check_txfirst. It returns immediately unless the validation mode isCheckTxorRecheckTxandvalidate_max_depthis at least 1, which means PV14 only. Block execution, inValidatormode, is unchanged at every version. The check pushes no validation operation, so check_tx fees are unchanged too; it only refuses earlier what the block refuses anyway.validate_max_depthv0: doc comment only.Breaking Changes
Consensus change at protocol version 14 only: a schema containing a
$refto a scalar is judged by the linear walk from PV14. Nothing changes for earlier protocol versions.Checklist:
🤖 Generated with Claude Code
PR Hygiene ·
5a02c54/self-revieweddpp(packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/common/mod.rs,packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs,packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rsand 3 more) — approved by QuantumExplorerrs-drive-abci(packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_common/check_tx_schema_depth.rs,packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_common/contract_structure_test_harness.rs,packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_common/mod.rsand 4 more) — approved by QuantumExplorerpackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v6.rs,packages/rs-platform-version/src/version/v14.rs) — approved by QuantumExplorerWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.Summary by CodeRabbit