test(dpp): guard doctype keyword names against stray keys of contracts admitted under meta-schema v0 - #4855
Conversation
|
Warning Review limit reachedNext included review available in 56 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: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
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 |
PR HygieneState: waiting-bots · commit
Self-review is an author attestation that you have read the diff: This check passes when the policy is satisfied; the repository decides whether merging requires it. |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-09-20T11:39:15.459Z |
|
🕓 Queued for automated review — 4th in line, estimated start in ~1.4 h (commit 9897e51)
|
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4855 +/- ##
============================================
- Coverage 83.92% 75.71% -8.22%
============================================
Files 3100 3105 +5
Lines 416777 454575 +37798
============================================
- Hits 349785 344178 -5607
- Misses 66992 110397 +43405
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
At dbc8fff, the changes are limited to tests and documentation; no blocking production or versioning defect was found. Two actionable suggestions remain: make the future-keyword guards detect omitted schema generations, and correct the description of the recorded keywords field. Verification included source inspection, schema inventory checks, and a successful diff whitespace check; Rust tests and the external network census were not independently rerun.
🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: platform-versioning); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The diff adds a substantial regression-test matrix and future-keyword coverage guards plus documentation, requiring review of versioned parsing assumptions but changing no runtime behavior or critical surface. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— platform-versioning (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(lane failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); 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/v3/meta_schema_v0_stray_keyword_tests.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v3/meta_schema_v0_stray_keyword_tests.rs:43-58: Make the keyword guards detect an omitted schema generation
Both future-keyword guards inspect only the manually maintained v1/v2/v3 list. If a later protocol activates document meta-schema v4 without extending this list, an unclassified keyword—or a recorded stray name such as `mutable`—introduced only in v4 escapes both guards. The existing maintenance comment does not make that omission fail, and the new book checklist promises that adding a keyword to a meta-schema triggers these checks. Add a completeness check against the document meta-schema versions selected by the platform-version tables, and include registration of new generations in the checklist.
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v3/meta_schema_v0_stray_keyword_tests.rs:21-22: Distinguish keywords absent from closed schemas from never-validated strays
The description says no meta-schema declares the recorded keys, but `document/v0/document-meta.json` declares `properties.keywords` as an array of bounded strings, with at most 20 entries. Consequently, the recorded testnet `keywords` field was subject to v0 validation, unlike the other stray names. It is absent from the closed v1/v2/v3 schemas, so retaining it in the collision guard is consistent with that guard's purpose. Qualify this description as keys that no closed meta-schema (v1 and later) declares, and make the same correction in the new book paragraph at lines 516–521.
| /// Every document meta-schema that closes the doctype level. A new | ||
| /// meta-schema version is added here when it is created. | ||
| const CLOSED_DOCUMENT_META_SCHEMAS: &[(&str, &str)] = &[ | ||
| ( | ||
| "v1", | ||
| include_str!("../../../../../../schema/meta_schemas/document/v1/document-meta.json"), | ||
| ), | ||
| ( | ||
| "v2", | ||
| include_str!("../../../../../../schema/meta_schemas/document/v2/document-meta.json"), | ||
| ), | ||
| ( | ||
| "v3", | ||
| include_str!("../../../../../../schema/meta_schemas/document/v3/document-meta.json"), | ||
| ), | ||
| ]; |
There was a problem hiding this comment.
🟡 Suggestion: Make the keyword guards detect an omitted schema generation
Both future-keyword guards inspect only the manually maintained v1/v2/v3 list. If a later protocol activates document meta-schema v4 without extending this list, an unclassified keyword—or a recorded stray name such as mutable—introduced only in v4 escapes both guards. The existing maintenance comment does not make that omission fail, and the new book checklist promises that adding a keyword to a meta-schema triggers these checks. Add a completeness check against the document meta-schema versions selected by the platform-version tables, and include registration of new generations in the checklist.
source: muse-spark-1.3-contributor (phase1-reviewer: general, architecture-layering, platform-versioning, rust-quality); gpt-6-astra (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality)
| /// Every doctype-level key that no meta-schema declares, carried by a contract | ||
| /// admitted under meta-schema v0. From a census of every contract create and |
There was a problem hiding this comment.
🟡 Suggestion: Distinguish keywords absent from closed schemas from never-validated strays
The description says no meta-schema declares the recorded keys, but document/v0/document-meta.json declares properties.keywords as an array of bounded strings, with at most 20 entries. Consequently, the recorded testnet keywords field was subject to v0 validation, unlike the other stray names. It is absent from the closed v1/v2/v3 schemas, so retaining it in the collision guard is consistent with that guard's purpose. Qualify this description as keys that no closed meta-schema (v1 and later) declares, and make the same correction in the new book paragraph at lines 516–521.
source: muse-spark-1.3-contributor (phase1-reviewer: general, architecture-layering, platform-versioning, rust-quality)
…s admitted under meta-schema v0 The document meta-schema v0 admitted every contract created at protocol versions 1 to 11 and does not refuse unknown doctype-level keys, so such a contract may carry a key named like a generation 3 keyword, of any shape, that no validator ever looked at. From protocol version 14 the stored path reads those keywords without validation. A census of every contract create and update transition on mainnet and testnet (2026-09-20) found no contract admitted under meta-schema v0 that carries any of them, and that set closed when protocol version 12 activated (mainnet height 398435, testnet height 362782). So the rule stays as it is: a keyword is read wherever it appears and its shape is enforced on both the validating and the stored path, with no stored-path leniency. - State the rule once on try_from_schema_generation_3 and point the inline actionFees comment at it. - Correct two comments that claimed a stored contract must have passed the meta-schema. - Record the stray doctype-level keys those contracts do carry (mainnet has `mutable`) and fail if any closed document meta-schema declares one as a keyword. That is the one hazard still open: a future keyword named after a stray key would change the meaning of a live contract. No behaviour change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
dbc8fff to
9897e51
Compare
QuantumExplorer
changed the title
…delete-by-type-03196d #4855 added its stray keyword guard test module at the same spot as the moderators' deletion tests: both kept. `canBeDeletedByModerators` is on neither network's list of stray doctype keys, and it is read on the stored path as on the validating one, as the rule #4855 states asks. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 participants
Issue being fixed or feature implemented
From protocol version 14, stored contracts are parsed by
try_from_schema_generation_3without full validation, and it reads doctype-level keywords (indexOnly,immutable,immutableAllowSetting,actionFees, the aggregate keywords, thekeeps*Historyflags) that not every stored contract was validated against. The document meta-schema v0 admitted every contract created at protocol versions 1 to 11 and does not refuse unknown doctype-level keys, so such a contract may carry a key of one of those names, of any shape. A malformed stray would make the contract impossible to load from the block that activates protocol version 14, and a well-formed strayindexOnly: truewould change the storage layout the parser assumes for documents already written.A reviewer of #4851 asked for one decision in the generation 3 driver instead of per-keyword leniency. #4851 has since merged with the leniency removed. This PR records the decision and the evidence behind it, and guards the one part of the hazard that is still open.
The hazard is not real on mainnet or testnet. A census of every contract create and update transition on both networks (2026-09-20), decoded from the raw bytes with dpp:
The set of contracts admitted under meta-schema v0 closed when protocol version 12 activated: every create and update since is validated by a meta-schema that refuses unknown doctype-level keys, and the property and index levels were closed from v0 on. So the result is final.
What is still open is the name of a future keyword. Strays do exist: a mainnet contract carries a doctype-level
mutable, and testnet contracts carrymutable,comment,position,tokenCosts,indexes,bls_public_keyandkeywords. That list is exhaustive, so a new doctype-level keyword is safe exactly when its name is not on it. A keyword named after one of them would silently change the meaning of a live contract.What was done?
No behaviour changes.
try_from_schema_generation_3: every doctype-level keyword of the generation is read wherever it appears, and its shape is enforced on both the validating and the stored path; no keyword gets stored-path leniency. The comment gives the reasons (the parser cannot tell which meta-schema admitted a contract,full_validation: falseis alsocheck_txand client parsing, refusing is the safe failure) and says that the safety rests on the census. The inline restatement feat(platform)!: document action fees paid to the contract owner and moderators, with a fee claim state transition #4851 added next toactionFeesbecomes a pointer to it.try_from_schema/common/mod.rs.parse_property_name_list_keywordclaimed a stored contract "can only ever have passed" the meta-schema, andparse_doctype_aggregate_keywordsclaimed meta-schema v0 allows the aggregate keywords as optional booleans. Neither is true for contracts admitted under meta-schema v0. Comments only; the file is shared with shipped generations and its code is untouched.should_not_name_a_doctype_keyword_after_a_stray_key_of_a_meta_schema_v0_contract, in a newtry_from_schema/v3/meta_schema_v0_stray_keyword_tests.rs. It records the stray keys and fails if any document meta-schema other than v0 declares one of them as a doctype-level keyword. It scans the meta-schema directory, so a future meta-schema version is covered without touching the test.The behaviour pinned in
immutable_tests.rs(rejects_a_non_array_value_on_both_modes,rejects_a_non_string_entry_on_both_modes) is kept, not reversed. Only its stated justification was wrong.How Has This Been Tested?
cargo test -p dpp --features all_features_without_client --lib -- try_from_schema action_fees: 237 passed.mutablekeyword to the v3 meta-schema makes the guard fail with "document meta-schema v3 declaresmutable, which contracts admitted under meta-schema v0 already carry as a stray key".cargo clippy -p dpp --features all_features_without_client --lib --tests -- -D warningsandcargo fmt.The census used the platform explorer's transaction list (
transaction_type0 and 4) for both networks and decoded every raw transition withStateTransition::deserialize_from_bytes_untrusted, then loaded each contract withDataContract::try_from_platform_versionedat protocol versions 11, 13 and 14, with and without validation. The explorer is a third-party indexer; its own contract count equals successful creates plus successful updates plus the system contracts on both networks. Running the same scan over a node state snapshot during the protocol version 14 upgrade rehearsal would remove that dependency.Breaking Changes
None. No consensus effect.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code