feat(dpp): add documents_countable to DocumentTypeV2 for O(1) total document counts#3457
feat(dpp): add documents_countable to DocumentTypeV2 for O(1) total document counts#3457QuantumExplorer wants to merge 1 commit intov3.1-devfrom
Conversation
📝 WalkthroughWalkthroughAdd DocumentTypeV2 and two optional top-level schema booleans ( Changes
Sequence Diagram(s)sequenceDiagram
participant Schema as Schema (JSON)
participant Parser as try_from_schema_v2
participant DocType as DocumentTypeV2
participant Drive as Drive (Insert/Update)
participant Storage as LowLevelTreeOps
Schema->>Parser: provide schema (may include documentsCountable, rangeCountable)
Parser->>Parser: read flags, compute documents_countable = documentsCountable || rangeCountable
Parser->>DocType: construct DocumentTypeV2 with flags
DocType->>Drive: Drive queries range_countable()/documents_countable()
alt range_countable == true
Drive->>Storage: for_known_path_key_empty_provable_count_tree (ProvableCountTree)
else documents_countable == true
Drive->>Storage: for_known_path_key_empty_count_tree (CountTree)
else
Drive->>Storage: batch_insert_empty_tree (NormalTree fallback)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
✅ Review complete (commit 51d1808) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3457 +/- ##
==========================================
Coverage 84.12% 84.13%
==========================================
Files 2601 2604 +3
Lines 271457 271856 +399
==========================================
+ Hits 228366 228717 +351
- Misses 43091 43139 +48
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs (1)
10-39: Random V2 generators never exercisedocuments_countable.Line 16 and Line 32 both build through
DocumentTypeV1, andFrom<DocumentTypeV1> for DocumentTypeV2initializesdocuments_countabletofalse. Any fuzz/property tests using these helpers therefore miss the new CountTree path entirely. Please thread the flag throughRandomDocumentTypeParametersor randomize it when the platform version supports the feature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs` around lines 10 - 39, The current helpers random_document_type and invalid_random_document_type call DocumentTypeV1 generators and convert to DocumentTypeV2, but the From<DocumentTypeV1> for DocumentTypeV2 sets documents_countable = false so the CountTree path is never exercised; update either RandomDocumentTypeParameters to include a documents_countable boolean and pass it through into DocumentTypeV1 -> DocumentTypeV2 construction, or (simpler) when platform_version supports CountTree, use the provided rng inside random_document_type and invalid_random_document_type to set DocumentTypeV2.documents_countable randomly before returning; locate the two functions (random_document_type, invalid_random_document_type) and the From<DocumentTypeV1> for DocumentTypeV2 conversion to implement threading or randomized assignment conditioned on platform_version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs`:
- Around line 301-307: The match in DocumentTypeRef::schema_owned (and similarly
in DocumentTypeMutRef::schema_owned) contains duplicated DocumentTypeRef::V2
pattern arms causing unreachable code; remove the redundant V2 arm so each
variant (V0, V1, V2) appears exactly once in the match, keeping the correct call
(v2.clone().schema_owned()) for the single V2 arm and run cargo clippy to verify
the warning is gone.
In
`@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs`:
- Around line 51-61: In the v1 parser in try_from_schema
(packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs),
add logic to detect the DOCUMENTS_COUNTABLE field during parsing and, when
full_validation is enabled and platform_version.protocol_version < 12 (i.e.,
v1/protocols 2–3), return the same
ProtocolError::ConsensusError(BasicError::UnsupportedFeatureError(...)) as the
v2 branch does; specifically, mirror the check that sets documents_countable and
returns UnsupportedFeatureError for "documentsCountable" so the v1 path no
longer silently ignores DOCUMENTS_COUNTABLE but fails explicitly under older
protocol versions.
---
Nitpick comments:
In `@packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs`:
- Around line 10-39: The current helpers random_document_type and
invalid_random_document_type call DocumentTypeV1 generators and convert to
DocumentTypeV2, but the From<DocumentTypeV1> for DocumentTypeV2 sets
documents_countable = false so the CountTree path is never exercised; update
either RandomDocumentTypeParameters to include a documents_countable boolean and
pass it through into DocumentTypeV1 -> DocumentTypeV2 construction, or (simpler)
when platform_version supports CountTree, use the provided rng inside
random_document_type and invalid_random_document_type to set
DocumentTypeV2.documents_countable randomly before returning; locate the two
functions (random_document_type, invalid_random_document_type) and the
From<DocumentTypeV1> for DocumentTypeV2 conversion to implement threading or
randomized assignment conditioned on platform_version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23a22067-a368-4eb1-b3a4-17e2a65e5136
📒 Files selected for processing (15)
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/accessors/mod.rspackages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-dpp/src/data_contract/document_type/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rspackages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/v12.rs
QuantumExplorer
force-pushed
the
feat/documents-countable
branch
2 times, most recently
from
d3b946a to
1e92143
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs (1)
8-8: Tighten inline comment scope to include both v2 flags.Line 8 and Line 36 comments mention only
documentsCountable, but the v2 parser path also coversblastSyncable. Updating the comment would prevent future confusion.✏️ Suggested comment tweak
-// Introduced in protocol version 12, adds documents_countable feature via try_from_schema v2 +// Introduced in protocol version 12, enables v2 schema parsing for documentsCountable/blastSyncable ... - try_from_schema: 2, // changed: supports documentsCountable + try_from_schema: 2, // changed: supports documentsCountable and blastSyncableAlso applies to: 36-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs` at line 8, Update the inline comment(s) that currently mention only `documentsCountable` to also include `blastSyncable`, clarifying that both feature flags are introduced in protocol version 12 and are set via the `try_from_schema v2` parser path; locate the comments near the `try_from_schema` v2 handling (references: `documentsCountable`, `blastSyncable`, `try_from_schema v2`) in v4.rs and adjust the text on both occurrences so the scope clearly covers both flags.packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs (2)
64-79: Consider using a named constant for the protocol version threshold.The literal
12is used here and inv1/mod.rs(line 325) for the same feature gate. WhilePROTOCOL_VERSION_12is defined inpackages/rs-platform-version/src/version/v12.rs, it's not used here. Using a constant would improve maintainability and clarity.💡 Example improvement
+use crate::version::PROTOCOL_VERSION_12; // or appropriate import path #[cfg(feature = "validation")] -if full_validation && needs_v2 && platform_version.protocol_version < 12 { +if full_validation && needs_v2 && platform_version.protocol_version < PROTOCOL_VERSION_12 {🤖 Prompt for AI Agents
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/v2/mod.rs` around lines 64 - 79, The code uses the literal 12 as a protocol threshold when gating features (in the if checking full_validation && needs_v2 && platform_version.protocol_version < 12); replace the magic number with the named constant PROTOCOL_VERSION_12 (import it from the platform-version crate/module where it is defined) and use that constant in the comparison (platform_version.protocol_version < PROTOCOL_VERSION_12); also make the same replacement in the corresponding v1 check to keep behavior consistent and add the necessary use/import for PROTOCOL_VERSION_12 at the top of the module.
96-100: Redundant assignment:documents_countable || blast_syncableis already enforced by the getter.At line 98,
documents_countableis set todocuments_countable || blast_syncable. However, theDocumentTypeV2Getters::documents_countable()getter (inv2/accessors.rslines 202-204) already returnsself.documents_countable || self.blast_syncable. This means the field value doesn't need to encode the implication—the getter handles it.Consider simplifying to just set the parsed values directly:
♻️ Proposed simplification
if needs_v2 { let mut v2: DocumentTypeV2 = v1.into(); - v2.documents_countable = documents_countable || blast_syncable; + v2.documents_countable = documents_countable; v2.blast_syncable = blast_syncable; Ok(v2.into())🤖 Prompt for AI Agents
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/v2/mod.rs` around lines 96 - 100, The code redundantly sets v2.documents_countable = documents_countable || blast_syncable inside the try_from path even though DocumentTypeV2Getters::documents_countable() already returns self.documents_countable || self.blast_syncable; change the assignment to store the parsed boolean directly (v2.documents_countable = documents_countable) and leave v2.blast_syncable = blast_syncable as-is so the getter enforces the implication, updating the conversion in the DocumentTypeV2 construction path that uses the try_from logic where DocumentTypeV2 is produced.packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs (1)
1-40: Indentation should use 4 spaces per coding guidelines.The file uses 2-space indentation, but Rust files in this repository should use 4-space indentation. Also,
random_document_typeis missing a doc comment whileinvalid_random_document_typehas one—consider adding a brief doc comment for consistency.As per coding guidelines: "Use 4-space indent for Rust files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs` around lines 1 - 40, The file uses 2-space indentation and is missing a doc comment for random_document_type; update the indentation to 4 spaces across the entire file and add a brief doc comment above impl DocumentTypeV2::random_document_type matching the style of invalid_random_document_type (e.g., "Creates a random valid document type" or similar). Ensure you adjust the indentation for the impl block and both methods (random_document_type and invalid_random_document_type) so they use 4-space indents and run rustfmt after changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs`:
- Around line 64-79: The code uses the literal 12 as a protocol threshold when
gating features (in the if checking full_validation && needs_v2 &&
platform_version.protocol_version < 12); replace the magic number with the named
constant PROTOCOL_VERSION_12 (import it from the platform-version crate/module
where it is defined) and use that constant in the comparison
(platform_version.protocol_version < PROTOCOL_VERSION_12); also make the same
replacement in the corresponding v1 check to keep behavior consistent and add
the necessary use/import for PROTOCOL_VERSION_12 at the top of the module.
- Around line 96-100: The code redundantly sets v2.documents_countable =
documents_countable || blast_syncable inside the try_from path even though
DocumentTypeV2Getters::documents_countable() already returns
self.documents_countable || self.blast_syncable; change the assignment to store
the parsed boolean directly (v2.documents_countable = documents_countable) and
leave v2.blast_syncable = blast_syncable as-is so the getter enforces the
implication, updating the conversion in the DocumentTypeV2 construction path
that uses the try_from logic where DocumentTypeV2 is produced.
In `@packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs`:
- Around line 1-40: The file uses 2-space indentation and is missing a doc
comment for random_document_type; update the indentation to 4 spaces across the
entire file and add a brief doc comment above impl
DocumentTypeV2::random_document_type matching the style of
invalid_random_document_type (e.g., "Creates a random valid document type" or
similar). Ensure you adjust the indentation for the impl block and both methods
(random_document_type and invalid_random_document_type) so they use 4-space
indents and run rustfmt after changes.
In
`@packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs`:
- Line 8: Update the inline comment(s) that currently mention only
`documentsCountable` to also include `blastSyncable`, clarifying that both
feature flags are introduced in protocol version 12 and are set via the
`try_from_schema v2` parser path; locate the comments near the `try_from_schema`
v2 handling (references: `documentsCountable`, `blastSyncable`, `try_from_schema
v2`) in v4.rs and adjust the text on both occurrences so the scope clearly
covers both flags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5570ee7c-2eae-487c-9586-dd234a5251f6
📒 Files selected for processing (15)
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/accessors/mod.rspackages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-dpp/src/data_contract/document_type/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rspackages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/v12.rs
✅ Files skipped from review due to trivial changes (3)
- packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rs
- packages/rs-platform-version/src/version/v12.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json
- packages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rs
- packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs
- packages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rs
- packages/rs-dpp/src/data_contract/document_type/v2/accessors.rs
- packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs
- packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs
QuantumExplorer
force-pushed
the
feat/documents-countable
branch
from
1e92143 to
be7f38d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs (1)
871-905: Add direct coverage for the new V2-specific accessor dispatch.There are no focused assertions here for
documents_countable()/blast_syncable()on the owned, ref, and mut-ref variants. A smallDocumentType::V2test would lock in these new match arms and the V0/V1 fallback behavior.Also applies to: 907-938
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs` around lines 871 - 905, Add focused unit tests that exercise the V2-specific match arms on DocumentType by constructing DocumentType::V2 (owned), a &DocumentType::V2 (ref) and a &mut DocumentType::V2 (mut-ref) and asserting the getters DocumentTypeV2Getters::documents_countable and ::blast_syncable return the expected values; also test setters DocumentTypeV2Setters::set_documents_countable and ::set_blast_syncable mutate the V2 state when called on the mut-ref and that calling the getters/setters on V0/V1 remains a no-op/returns false as currently implemented; target the DocumentType, DocumentTypeV2Getters, DocumentTypeV2Setters, documents_countable, blast_syncable, set_documents_countable and set_blast_syncable symbols in your new tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-dpp/src/data_contract/document_type/v2/accessors.rs`:
- Around line 211-220: The setter set_documents_countable currently allows
documents_countable to be set to false while blast_syncable may remain true;
update impl DocumentTypeV2Setters for DocumentTypeV2 so
set_documents_countable(&mut self, countable: bool) sets
self.documents_countable = countable and if countable == false also clear
self.blast_syncable to false to preserve the blast_syncable =>
documents_countable invariant (mirror the normalization done in
set_blast_syncable).
In `@packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs`:
- Around line 288-319: Existing code allows toggling documents_countable and
blast_syncable on contract updates which corrupts on-disk trees; modify
validate_config to detect any change to DocumentType::documents_countable or
DocumentType::blast_syncable for existing document types and reject the update.
Specifically, in validate_config compare the old and new document type maps (by
name/identifier) and if an existing type's documents_countable or blast_syncable
differs, return a validation error (match the style used for
documents_mutable/documents_keep_history checks). Ensure the same symbols are
referenced: validate_config, DocumentType (or document type map),
documents_countable, blast_syncable, and use the same error type/format used
elsewhere so callers (including update_contract logic) will fail fast.
---
Nitpick comments:
In `@packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs`:
- Around line 871-905: Add focused unit tests that exercise the V2-specific
match arms on DocumentType by constructing DocumentType::V2 (owned), a
&DocumentType::V2 (ref) and a &mut DocumentType::V2 (mut-ref) and asserting the
getters DocumentTypeV2Getters::documents_countable and ::blast_syncable return
the expected values; also test setters
DocumentTypeV2Setters::set_documents_countable and ::set_blast_syncable mutate
the V2 state when called on the mut-ref and that calling the getters/setters on
V0/V1 remains a no-op/returns false as currently implemented; target the
DocumentType, DocumentTypeV2Getters, DocumentTypeV2Setters, documents_countable,
blast_syncable, set_documents_countable and set_blast_syncable symbols in your
new tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc904f91-e9e2-4e3d-b430-2cf570fb26de
📒 Files selected for processing (15)
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/accessors/mod.rspackages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-dpp/src/data_contract/document_type/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rspackages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/v12.rs
✅ Files skipped from review due to trivial changes (5)
- packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs
- packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rs
- packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json
- packages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rs
- packages/rs-platform-version/src/version/v12.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs
- packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs
- packages/rs-dpp/src/data_contract/document_type/v2/mod.rs
QuantumExplorer
force-pushed
the
feat/documents-countable
branch
from
be7f38d to
1e21664
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-dpp/src/data_contract/document_type/v2/mod.rs`:
- Around line 71-75: The two capability flags documents_countable and
blast_syncable currently have crate-level mutable visibility which allows
bypassing DocumentTypeV2Setters and breaking the invariant that blast_syncable
implies documents_countable; make both fields private (remove pub(in
crate::data_contract)) and enforce the invariant in the setter methods on
DocumentTypeV2 (DocumentTypeV2Setters): when setting blast_syncable = true also
set documents_countable = true, and when setting documents_countable = false
prevent or clear blast_syncable (or return an error) to avoid inconsistency;
update any internal callers to use the provided getters/setters instead of
writing the fields directly.
In `@packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs`:
- Around line 288-319: The update path currently always creates a NormalTree for
newly added document types; replicate the same conditional tree selection as in
the insert path by checking document_type.blast_syncable() and
document_type.documents_countable() and creating the matching tree: if
blast_syncable() create a ProvableCountTree (use
Element::empty_provable_count_tree_with_flags /
Element::empty_provable_count_tree and
LowLevelDriveOperation::insert_for_known_path_key_element with path_items and
vec![0]), else if documents_countable() call
LowLevelDriveOperation::for_known_path_key_empty_count_tree with path_items,
vec![0], and storage_flags.as_ref(), otherwise fall back to calling
batch_insert_empty_tree with Key(vec![0]), passing storage_flags.as_ref(), &mut
batch_operations and platform_version.drive; mirror the storage_flags handling
used in the insert code so update and insert behavior are consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68ecc863-7609-4966-bab9-2ef4bf430a49
📒 Files selected for processing (16)
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/accessors/mod.rspackages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-dpp/src/data_contract/document_type/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rspackages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/v12.rs
✅ Files skipped from review due to trivial changes (3)
- packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rs
- packages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs
- packages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs
- packages/rs-platform-version/src/version/v12.rs
| /// When true, the primary key tree uses a CountTree enabling O(1) total document count queries | ||
| pub(in crate::data_contract) documents_countable: bool, | ||
| /// When true, the primary key tree uses a ProvableCountTree enabling BLAST sync. | ||
| /// Implies documents_countable = true. | ||
| pub(in crate::data_contract) blast_syncable: bool, |
There was a problem hiding this comment.
Restrict direct writes to capability flags to preserve invariants
blast_syncable is documented to imply documents_countable, but both fields are writable across crate::data_contract via pub(in crate::data_contract). That allows bypassing DocumentTypeV2Setters and creating inconsistent internal state.
🔧 Proposed fix
- pub(in crate::data_contract) documents_countable: bool,
+ documents_countable: bool,
...
- pub(in crate::data_contract) blast_syncable: bool,
+ blast_syncable: bool,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-dpp/src/data_contract/document_type/v2/mod.rs` around lines 71 -
75, The two capability flags documents_countable and blast_syncable currently
have crate-level mutable visibility which allows bypassing DocumentTypeV2Setters
and breaking the invariant that blast_syncable implies documents_countable; make
both fields private (remove pub(in crate::data_contract)) and enforce the
invariant in the setter methods on DocumentTypeV2 (DocumentTypeV2Setters): when
setting blast_syncable = true also set documents_countable = true, and when
setting documents_countable = false prevent or clear blast_syncable (or return
an error) to avoid inconsistency; update any internal callers to use the
provided getters/setters instead of writing the fields directly.
There was a problem hiding this comment.
I couldn't validate a concrete current-SHA bug here. Crate-internal visibility alone does not create an observable invariant break, and I did not find a call path in this PR that bypasses the existing constructor/setter normalization to persist an inconsistent state.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all direct field assignments to documents_countable and blast_syncable
rg -n 'documents_countable\s*=' --type rust -C 2
echo "---"
rg -n 'blast_syncable\s*=' --type rust -C 2
Length of output: 7843
@thepastaclaw, you're right — I've confirmed your analysis. The search results show two relevant facts:
-
The getter absorbs field-level inconsistency:
documents_countable()is defined asself.documents_countable || self.blast_syncable, so even a direct write ofblast_syncable = truewithout touchingdocuments_countablestill produces the correct observable result from the getter. -
All production callers use the getter: the drive
insert_contractcode callsdocument_type.documents_countable()/document_type.blast_syncable(), not the raw fields. The test at Line 243 even demonstrates this pattern intentionally.
The try_from_schema v2 direct assignment (v2.documents_countable = documents_countable || blast_syncable) also manually preserves the invariant correctly without going through the setter.
There's no concrete current-SHA bug here — the getter provides a second layer of invariant enforcement. I'll withdraw this concern.
✏️ Learnings added
Learnt from: thepastaclaw
Repo: dashpay/platform PR: 3457
File: packages/rs-dpp/src/data_contract/document_type/v2/mod.rs:71-75
Timestamp: 2026-04-08T12:17:18.923Z
Learning: In `packages/rs-dpp/src/data_contract/document_type/v2/`, the `documents_countable` and `blast_syncable` fields on `DocumentTypeV2` are `pub(in crate::data_contract)`. This is intentional. The getter `documents_countable()` returns `self.documents_countable || self.blast_syncable`, which absorbs any field-level inconsistency. All production callers (including drive's `insert_contract`) use the getter, not the raw field. Direct field writes are valid within `crate::data_contract` and are used in both `try_from_schema/v2` and tests; the getter ensures observable correctness even when the setter is bypassed. No tighter visibility restriction is required.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 3457
File: packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs:64-79
Timestamp: 2026-04-08T09:53:34.375Z
Learning: In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema`, the v1 parser intentionally ignores the `documentsCountable` field because it has no field to parse it into. The v2 parser handles `documentsCountable` explicitly. Parser-version selection (controlled by `try_from_schema` version in the platform version constant, e.g. `CONTRACT_VERSIONS_V4` for protocol v12+) is the gate that prevents older parsers from producing a `DocumentTypeV2` with `documents_countable: true`, so no explicit rejection in v1 is needed.
9e4cd7c to
4fa6850
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I validated the current SHA from the completed Codex review outputs plus direct code inspection. Two consensus-critical problems remain: contract updates still create newly added countable / BLAST document types with a plain NormalTree, and several stateless query / fee-estimation paths still hard-code TreeType::NormalTree even when execution now uses CountTree / ProvableCountTree. The other live CodeRabbit threads were either already fixed or already retracted in-thread on the current SHA.
I validated existing CodeRabbit comments separately in-thread instead of restating them here.
Reviewed commit: 4fa6850
🔴 1 blocking
1 additional finding
🔴 blocking: Stateless query and fee-estimation paths still underprice countable / BLAST document trees as `NormalTree`
packages/rs-drive/src/drive/document/insert/add_document_for_contract_operations/v0/mod.rs (lines 44-45)
Primary document storage now executes on CountTree / ProvableCountTree for countable or BLAST-syncable document types, but related stateless paths still hard-code TreeType::NormalTree. The affected code includes insert-time existence checks here, delete-time reads in packages/rs-drive/src/drive/document/delete/delete_document_for_contract_operations/v0/mod.rs, delete-cost estimation in packages/rs-drive/src/drive/document/delete/internal/add_estimation_costs_for_remove_document_to_primary_storage/v0/mod.rs, and add-cost estimation in packages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rs. Because CountTree/ProvableCountTree maintain additional count/provable metadata, pricing these operations as NormalTree creates a consensus-accepted undercharge for attacker-chosen document types that enable the new flags.
🤖 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/src/drive/document/insert/add_document_for_contract_operations/v0/mod.rs`:
- [BLOCKING] lines 44-45: Stateless query and fee-estimation paths still underprice countable / BLAST document trees as `NormalTree`
Primary document storage now executes on `CountTree` / `ProvableCountTree` for countable or BLAST-syncable document types, but related stateless paths still hard-code `TreeType::NormalTree`. The affected code includes insert-time existence checks here, delete-time reads in `packages/rs-drive/src/drive/document/delete/delete_document_for_contract_operations/v0/mod.rs`, delete-cost estimation in `packages/rs-drive/src/drive/document/delete/internal/add_estimation_costs_for_remove_document_to_primary_storage/v0/mod.rs`, and add-cost estimation in `packages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rs`. Because CountTree/ProvableCountTree maintain additional count/provable metadata, pricing these operations as NormalTree creates a consensus-accepted undercharge for attacker-chosen document types that enable the new flags.
4fa6850 to
0568dd4
Compare
| }, | ||
| "documentsCountable": { | ||
| "type": "boolean", | ||
| "description": "When true, the primary key tree uses a CountTree enabling O(1) total document count queries" |
There was a problem hiding this comment.
doesn't it allow more than that? Wouldn't it allow counts where some indexed property is something? What is the primary key if something has multiple indexes? Would it be better to allow selecting an individual index as countable?
There was a problem hiding this comment.
This PR is for primary key only, we already did another PR for indexes (which might need fixing).
| let primary_key_tree_type = if document_type.blast_syncable() { | ||
| TreeType::ProvableCountTree | ||
| } else if document_type.documents_countable() { | ||
| TreeType::CountTree |
There was a problem hiding this comment.
are these differences explained somewhere? I don't think I realized the counts for normal count trees aren't provable?
There was a problem hiding this comment.
The count is provable. But sub counts are not. Sub counts are only important in blast sync and basically don't give meaningful information except for BLAST sync.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs (1)
125-148:⚠️ Potential issue | 🟠 MajorKeep the per-document history subtree as
NormalTree.At Line 145 this branch is creating the
document_idsubtree underprimary_key_path, but it passesprimary_key_tree_typeas the type of the new subtree. For history-enabled types, the count/provable-count tree needs to stay onprimary_key_pathso it counts document IDs; the childdocument_idbucket should remainNormalTree. The estimator added inpackages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rsalready models that split, so the runtime path is diverging from the estimated layout.🔧 Suggested adjustment
let apply_type = if estimated_costs_only_with_layer_info.is_none() { BatchInsertTreeApplyType::StatefulBatchInsertTree } else { BatchInsertTreeApplyType::StatelessBatchInsertTree { - in_tree_type: TreeType::NormalTree, - tree_type: primary_key_tree_type, + in_tree_type: primary_key_tree_type, + tree_type: TreeType::NormalTree, flags_len: storage_flags .map(|s| s.serialized_size()) .unwrap_or_default(), } }; // we first insert an empty tree if the document is new self.batch_insert_empty_tree_if_not_exists( path_key_info, - primary_key_tree_type, + TreeType::NormalTree, storage_flags, apply_type,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs` around lines 125 - 148, The code is creating the per-document (document_id) subtree using primary_key_tree_type, which makes history-enabled document types get Count/ProvableCount trees under each document_id; instead, ensure the child document_id subtree is always created as TreeType::NormalTree. Update the call to batch_insert_empty_tree_if_not_exists (and any place where a new document_id subtree type is passed) to use TreeType::NormalTree for the document_id subtree while leaving primary_key_tree_type unchanged for the parent primary_key_path; keep the existing apply_type/flags logic intact so the estimator and runtime layout match.
🧹 Nitpick comments (1)
packages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rs (1)
73-79: Consider centralizing primary-key tree selection.This
blast_syncable()/documents_countable()→TreeTypemapping now exists in multiple insert/update/estimation paths. A small helper on the document-type or drive side would make the storage-layout rule single-source and reduce drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rs` around lines 73 - 79, The primary-key TreeType selection logic (the conditional using document_type.blast_syncable(), document_type.documents_countable() to set primary_key_tree_type to TreeType::ProvableCountTree / TreeType::CountTree / TreeType::NormalTree) is duplicated across insert/update/estimation code paths; create a single helper method (e.g., DocumentType::primary_key_tree_type() or Drive::primary_key_tree_type_for(document_type)) that encapsulates this mapping and replace the inline conditional in add_estimation_costs_for_add_document_to_primary_storage::v0::mod.rs (and other insert/update/estimation modules) to call that helper, keeping the unique symbols document_type, blast_syncable(), documents_countable(), primary_key_tree_type, and TreeType as the mapping contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs`:
- Around line 125-148: The code is creating the per-document (document_id)
subtree using primary_key_tree_type, which makes history-enabled document types
get Count/ProvableCount trees under each document_id; instead, ensure the child
document_id subtree is always created as TreeType::NormalTree. Update the call
to batch_insert_empty_tree_if_not_exists (and any place where a new document_id
subtree type is passed) to use TreeType::NormalTree for the document_id subtree
while leaving primary_key_tree_type unchanged for the parent primary_key_path;
keep the existing apply_type/flags logic intact so the estimator and runtime
layout match.
---
Nitpick comments:
In
`@packages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rs`:
- Around line 73-79: The primary-key TreeType selection logic (the conditional
using document_type.blast_syncable(), document_type.documents_countable() to set
primary_key_tree_type to TreeType::ProvableCountTree / TreeType::CountTree /
TreeType::NormalTree) is duplicated across insert/update/estimation code paths;
create a single helper method (e.g., DocumentType::primary_key_tree_type() or
Drive::primary_key_tree_type_for(document_type)) that encapsulates this mapping
and replace the inline conditional in
add_estimation_costs_for_add_document_to_primary_storage::v0::mod.rs (and other
insert/update/estimation modules) to call that helper, keeping the unique
symbols document_type, blast_syncable(), documents_countable(),
primary_key_tree_type, and TreeType as the mapping contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57e10670-6fbf-4eda-af45-176e7a525d52
📒 Files selected for processing (21)
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/accessors/mod.rspackages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-dpp/src/data_contract/document_type/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rspackages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-drive/src/drive/contract/update/update_contract/v0/mod.rspackages/rs-drive/src/drive/document/delete/internal/add_estimation_costs_for_remove_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/fees/op.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/v12.rs
✅ Files skipped from review due to trivial changes (8)
- packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs
- packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rs
- packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json
- packages/rs-platform-version/src/version/v12.rs
- packages/rs-dpp/src/data_contract/document_type/accessors/v2/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/accessors/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rs
- packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs
- packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs
|
@thepastaclaw are things good now? |
|
Not quite yet. I re-checked the current head (
So countable / BLAST document types are still being modeled as There is also still the history-layout mismatch in So my answer is: better, but not good yet. I’d want one more push that fixes those remaining paths before clearing this. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The earlier storage-layout bugs around contract update and the dedicated estimation helpers are fixed, but this SHA still has three merge-blocking gaps in the new tree-type rollout. Pre-v12 contracts can already carry the new schema keys even though older parsers ignore them, and the remaining insert paths still mis-handle CountTree/ProvableCountTree semantics in ways that keep stateless costing and stateful execution out of sync.
Reviewed commit: 0568dd4
🔴 3 blocking
1 additional finding
🔴 blocking: Stateless non-history inserts still price count/provable primary storage as `NormalTree`
packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs (lines 445-452)
The non-history batch_insert_if_not_exists path still hard-codes in_tree_type: TreeType::NormalTree even though this PR now creates the primary key tree as CountTree or ProvableCountTree for documents_countable / blast_syncable document types. That keeps fee estimation and dry-run validation on the cheaper tree variant while execution writes into the heavier count/provable tree, so flagged document types can still be accepted with underpriced insert costs. The dedicated estimation helper was fixed earlier in the patch, but this inline stateless insert path is still out of sync with the actual storage layout.
💡 Suggested change
let primary_key_tree_type = if document_type.blast_syncable() {
TreeType::ProvableCountTree
} else if document_type.documents_countable() {
TreeType::CountTree
} else {
TreeType::NormalTree
};
let apply_type = if estimated_costs_only_with_layer_info.is_none() {
BatchInsertApplyType::StatefulBatchInsert
} else {
BatchInsertApplyType::StatelessBatchInsert {
in_tree_type: primary_key_tree_type,
target: QueryTargetValue(document_type.estimated_size(platform_version)? as u32),
}
};
🤖 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/schema/meta_schemas/document/v0/document-meta.json`:
- [BLOCKING] lines 508-514: Pre-v12 contracts can smuggle in tree-type flags that activate after the v12 parser switch
These keys are now accepted by the shared document meta-schema for every protocol version, but pre-v12 parsing still dispatches to the older document-type parsers that validate against this schema and then ignore the fields. Because contracts serialize the raw document schemas and reparse them on deserialization with the current `platform_version`, a contract accepted before v12 can later be reloaded after upgrade with `documentsCountable` / `blastSyncable` suddenly taking effect even though its primary tree was originally created as a normal tree. That creates a storage-layout invariant break across protocol activation and can surface later as fee/proof divergence or block-processing failures for document types that were accepted too early.
In `packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs`:
- [BLOCKING] lines 136-147: Keep-history inserts swap the parent and child tree types
In the `documents_keep_history()` branch, the stateless apply type still treats the parent as `NormalTree` and the child as `primary_key_tree_type`, and the stateful insert creates the per-document history subtree with `primary_key_tree_type` as well. Those are backwards: the parent being inserted into is the primary key tree, which can now be `CountTree` / `ProvableCountTree`, while the per-document history subtree should remain a plain `NormalTree`. As written, stateful execution builds the wrong subtree type and stateless costing models both sides of the insert against the wrong tree variants, so history-enabled countable/BLAST document types no longer preserve a consistent storage-layout/cost invariant.
- [BLOCKING] lines 445-452: Stateless non-history inserts still price count/provable primary storage as `NormalTree`
The non-history `batch_insert_if_not_exists` path still hard-codes `in_tree_type: TreeType::NormalTree` even though this PR now creates the primary key tree as `CountTree` or `ProvableCountTree` for `documents_countable` / `blast_syncable` document types. That keeps fee estimation and dry-run validation on the cheaper tree variant while execution writes into the heavier count/provable tree, so flagged document types can still be accepted with underpriced insert costs. The dedicated estimation helper was fixed earlier in the patch, but this inline stateless insert path is still out of sync with the actual storage layout.
0568dd4 to
5adbe0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/rs-drive/src/drive/document/insert/add_document_for_contract_operations/v0/mod.rs (1)
40-49: Consider centralizing primary-key tree selection.This
blast_syncable/documents_countable/NormalTreemapping now exists in several insert/delete/estimation paths. One missed call site already caused drift in this PR, so a shared helper would make future consensus-layout changes harder to miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/document/insert/add_document_for_contract_operations/v0/mod.rs` around lines 40 - 49, Extract the repeated mapping logic (document_type.blast_syncable -> TreeType::ProvableCountTree, document_type.documents_countable -> TreeType::CountTree, else -> TreeType::NormalTree) into a single helper function (e.g., fn primary_key_tree_type_for(document_type: &DocumentType) -> TreeType) and replace the inline logic in add_document_for_contract_operations::v0 (the block that sets primary_key_tree_type) and all other insert/delete/estimation call sites with calls to that helper; ensure the helper lives in a shared module so all consumers (insert, delete, estimation paths) import and use it to avoid drift.packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs (1)
667-941: Add a V2-backed accessor test.These new
V2branches are not exercised by this module’s tests; the current fixture coverage is still V0-oriented, so regressions in token-cost delegation ordocuments_countable/blast_syncabledispatch would slip through. Please add at least one unit test that constructs aDocumentType::V2and checks owned/ref/mut-ref accessors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs` around lines 667 - 941, Add a unit test that constructs a DocumentType::V2 instance (with deterministic values for token costs and documents_countable/blast_syncable), then verify the V2-tail dispatch for owned, reference, and mutable-reference variants: call DocumentTypeV1Getters methods (document_creation_token_cost, document_replacement_token_cost, document_deletion_token_cost, document_transfer_token_cost, document_update_price_token_cost, document_purchase_token_cost, all_document_token_costs, all_external_token_costs_contract_tokens) and DocumentTypeV2Getters methods (documents_countable, blast_syncable) on DocumentType::V2, DocumentTypeRef::V2, and DocumentTypeMutRef::V2 and assert they return the expected values from the underlying V2 struct; place the test alongside existing tests for this module so V2 branches are exercised.packages/rs-dpp/src/data_contract/document_type/v2/mod.rs (2)
108-140: Consider collapsing the duplicated V0/V1→V2 conversions.These two
Fromimpls copy the same field list almost verbatim, which makes futureDocumentTypeV2additions easy to miss in one path. A small shared constructor/helper withtoken_costsas the only varying input would make this less brittle.Also applies to: 142-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/v2/mod.rs` around lines 108 - 140, The two From impls for DocumentTypeV2 (From<DocumentTypeV0> and From<DocumentTypeV1>) duplicate the same field mapping; extract a shared constructor/helper (e.g., DocumentTypeV2::from_common or a private fn build_from_fields) that accepts a source struct (or common fields) and a TokenCosts value, move the repeated field assignments (name, schema, indices, index_structure, flattened_properties, properties, identifier_paths, binary_paths, required_fields, transient_fields, documents_keep_history, documents_mutable, documents_can_be_deleted, documents_transferable, trade_mode, creation_restriction_mode, data_contract_id, requires_identity_encryption_bounded_key, requires_identity_decryption_bounded_key, security_level_requirement, #[cfg(feature="validation")] json_schema_validator, documents_countable, blast_syncable) into that helper, and then have both From<DocumentTypeV0> and From<DocumentTypeV1> call it passing the appropriate TokenCosts::V0(Default::default()) or TokenCosts::V1(value) so only the token_costs difference remains in each From impl.
185-195: Pin the test helper to a fixed platform version.Using
PlatformVersion::latest()here makes these unit tests vulnerable to unrelated protocol-profile changes. A concrete test version would keep the V0/V1→V2 conversion coverage stable over time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/v2/mod.rs` around lines 185 - 195, The test helper make_v0 currently calls IndexLevel::try_from_indices with PlatformVersion::latest(), which makes the unit test brittle; replace PlatformVersion::latest() with a pinned, concrete platform version used for tests (e.g., the specific PlatformVersion variant/constructor your test-suite uses) so DocumentTypeV0 -> V2 conversion remains stable; update the call in make_v0 (and any similar helpers) to pass that fixed PlatformVersion instead of PlatformVersion::latest(), using the existing test constant or explicit version constructor used elsewhere in the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs`:
- Around line 666-696: The new validation block in try_from_schema v1 (the if
full_validation branch that calls Value::inner_optional_bool_value on
property_names::DOCUMENTS_COUNTABLE and property_names::BLAST_SYNCABLE and
returns ProtocolError::ConsensusError/UnsupportedFeatureError) must be removed
or disabled so V1 stays tolerant: V1 should ignore V2-only flags instead of
failing; revert the change that inspects documentsCountable/blastSyncable and
the associated error path (ProtocolError::ConsensusError /
UnsupportedFeatureError) and ensure routing of protocol_version >= 12 schemas to
the V2 parser remains the mechanism that enforces those features, leaving
try_from_schema v1 to simply skip/map only fields present in the V1 output type
(do not rely on full_validation to enforce this).
---
Nitpick comments:
In `@packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs`:
- Around line 667-941: Add a unit test that constructs a DocumentType::V2
instance (with deterministic values for token costs and
documents_countable/blast_syncable), then verify the V2-tail dispatch for owned,
reference, and mutable-reference variants: call DocumentTypeV1Getters methods
(document_creation_token_cost, document_replacement_token_cost,
document_deletion_token_cost, document_transfer_token_cost,
document_update_price_token_cost, document_purchase_token_cost,
all_document_token_costs, all_external_token_costs_contract_tokens) and
DocumentTypeV2Getters methods (documents_countable, blast_syncable) on
DocumentType::V2, DocumentTypeRef::V2, and DocumentTypeMutRef::V2 and assert
they return the expected values from the underlying V2 struct; place the test
alongside existing tests for this module so V2 branches are exercised.
In `@packages/rs-dpp/src/data_contract/document_type/v2/mod.rs`:
- Around line 108-140: The two From impls for DocumentTypeV2
(From<DocumentTypeV0> and From<DocumentTypeV1>) duplicate the same field
mapping; extract a shared constructor/helper (e.g., DocumentTypeV2::from_common
or a private fn build_from_fields) that accepts a source struct (or common
fields) and a TokenCosts value, move the repeated field assignments (name,
schema, indices, index_structure, flattened_properties, properties,
identifier_paths, binary_paths, required_fields, transient_fields,
documents_keep_history, documents_mutable, documents_can_be_deleted,
documents_transferable, trade_mode, creation_restriction_mode, data_contract_id,
requires_identity_encryption_bounded_key,
requires_identity_decryption_bounded_key, security_level_requirement,
#[cfg(feature="validation")] json_schema_validator, documents_countable,
blast_syncable) into that helper, and then have both From<DocumentTypeV0> and
From<DocumentTypeV1> call it passing the appropriate
TokenCosts::V0(Default::default()) or TokenCosts::V1(value) so only the
token_costs difference remains in each From impl.
- Around line 185-195: The test helper make_v0 currently calls
IndexLevel::try_from_indices with PlatformVersion::latest(), which makes the
unit test brittle; replace PlatformVersion::latest() with a pinned, concrete
platform version used for tests (e.g., the specific PlatformVersion
variant/constructor your test-suite uses) so DocumentTypeV0 -> V2 conversion
remains stable; update the call in make_v0 (and any similar helpers) to pass
that fixed PlatformVersion instead of PlatformVersion::latest(), using the
existing test constant or explicit version constructor used elsewhere in the
codebase.
In
`@packages/rs-drive/src/drive/document/insert/add_document_for_contract_operations/v0/mod.rs`:
- Around line 40-49: Extract the repeated mapping logic
(document_type.blast_syncable -> TreeType::ProvableCountTree,
document_type.documents_countable -> TreeType::CountTree, else ->
TreeType::NormalTree) into a single helper function (e.g., fn
primary_key_tree_type_for(document_type: &DocumentType) -> TreeType) and replace
the inline logic in add_document_for_contract_operations::v0 (the block that
sets primary_key_tree_type) and all other insert/delete/estimation call sites
with calls to that helper; ensure the helper lives in a shared module so all
consumers (insert, delete, estimation paths) import and use it to avoid drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4801f72-3c6d-4969-ba1c-d018958e7213
📒 Files selected for processing (25)
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/accessors/mod.rspackages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-dpp/src/data_contract/document_type/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rspackages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-drive/src/drive/contract/update/update_contract/v0/mod.rspackages/rs-drive/src/drive/document/delete/delete_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/delete/internal/add_estimation_costs_for_remove_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_document_from_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/fees/op.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/v12.rs
✅ Files skipped from review due to trivial changes (3)
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rs
- packages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rs
- packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rs
- packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs
- packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs
- packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json
- packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs
- packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs
- packages/rs-drive/src/drive/contract/update/update_contract/v0/mod.rs
- packages/rs-drive/src/fees/op.rs
- packages/rs-platform-version/src/version/v12.rs
- packages/rs-dpp/src/data_contract/document_type/mod.rs
- packages/rs-dpp/src/data_contract/document_type/v2/accessors.rs
|
Re-checked the current head ( What changed on the paths I was worried about:
So from my side: yes, things look good now. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This force-push fixes the earlier CountTree/ProvableCountTree insertion/costing mismatches, but two merge-blocking contract-parsing bugs remain. First, protocol v12 routes document-type parsing through the new v2 parser, yet that parser forwards documentsCountable / blastSyncable straight into the v1 parser with full_validation intact, so full validation rejects the very fields this PR introduces and contract creation cannot use the feature. Second, the new pre-v12 rejection only runs during full validation; contracts already accepted into state before this fix can still be deserialized after the v12 parser switch with those flags taking effect even though their primary storage trees were originally created as normal trees. I also confirmed a smaller coverage gap: the new immutability checks in validate_update do not have matching unit tests like the neighboring config invariants do.
Reviewed commit: 5adbe0d
🔴 2 blocking | 🟡 1 suggestion(s)
🤖 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/v2/mod.rs`:
- [BLOCKING] lines 57-70: V2 document-type parsing forwards the new flags into the V1 rejection path
Protocol v12 switches `document_type.try_from_schema` to version 2, but `DocumentTypeV2::try_from_schema()` only reads `documentsCountable` / `blastSyncable` and then calls `DocumentTypeV1::try_from_schema()` with the original `schema` and the same `full_validation` flag. The new v1 guard at `try_from_schema/v1/mod.rs:670-695` rejects either field whenever full validation is enabled, so any contract-create or contract-update flow that parses a v12 schema with one of these flags set will raise `UnsupportedFeatureError` before the v2 wrapper can ever reattach the values. In other words, the feature is dead on arrival in the production validation path. Strip the two v2-only keys before delegating to v1 (or explicitly bypass that v1 guard from the v2 entry point) so v12 contracts can actually create count/provable document types.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs`:
- [BLOCKING] lines 670-695: The pre-v12 flag rejection still lets already-stored contracts mutate meaning after the v12 parser switch
The new `documentsCountable` / `blastSyncable` rejection only runs when `full_validation` is true. Contracts read back from Drive use `DataContract::versioned_deserialize(..., false, ...)`, which forwards `full_validation = false` through `DataContractV1::try_from_platform_versioned_v1()` into `create_document_types_from_document_schemas()` and then `DocumentType::try_from_schema()`. On a v12 node that deserialization path now uses the v2 parser, so any contract that was previously accepted into chain state with these raw schema keys present will be reloaded as a count/provable document type even though its primary key tree was originally created as a normal tree. The top-level document meta-schema on the base branch did not forbid unknown document-type properties, so this is not just hypothetical future input: old state can already contain the latent keys. The new guard prevents fresh pre-v12 submissions, but it does not protect existing chain data from changing storage semantics after upgrade. The fix needs to sanitize or reject those fields on deserialization as well, not only during full-validation submission paths.
In `packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs`:
- [SUGGESTION] lines 183-211: The new countable/blast immutability checks are missing the companion tests the neighboring invariants already have
`validate_config()` now rejects changes to `documents_countable()` and `blast_syncable()`, which is the right invariant because changing those flags after creation would change the primary storage tree shape. However, the test module below still only covers the older config immutability checks (transferable, can_be_deleted, keep_history, mutable, encryption/decryption key requirements, security level, etc.). A quick grep in this file shows no test exercising the new countable/blast branches. Given that these guards protect storage-layout stability, they deserve the same regression coverage as the surrounding checks.
5adbe0d to
b1064f8
Compare
|
Re-checked the current head ( But I still do not think this is fully good yet. The remaining problem is the pre-v12 deserialization path. So from my side the answer is: not quite yet — I still want that old-state migration / sanitize / rejection hole closed before I clear it. |
QuantumExplorer
force-pushed
the
feat/documents-countable
branch
from
b1064f8 to
cc855a8
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json (1)
508-515:⚠️ Potential issue | 🔴 CriticalBlocking: protocol-agnostic schema acceptance still allows pre-v12 flag smuggling.
Line 508-515 makes these keys valid in the shared document meta-schema for all versions. Without a hard pre-v12 sanitize/reject path during deserialization/reparse, legacy contracts can be interpreted post-upgrade with different primary-tree semantics than originally materialized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json` around lines 508 - 515, The schema currently exposes documentsCountable and blastSyncable as valid keys for all versions which allows pre-v12 contracts to smuggle new primary-tree flags; update the validation flow so those keys are only accepted when protocolVersion >= 12: remove or hide documentsCountable and blastSyncable from the shared, protocol-agnostic document meta acceptance and instead validate them in the v12+ document meta validator (or add an explicit check in the document meta deserialization/reparse path) that rejects or strips these fields for protocolVersion < 12; ensure the deserializer/reparser path that handles document meta enforces this reject/strip behavior so legacy contracts cannot be interpreted with v12 primary-tree semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json`:
- Around line 508-515: The schema currently exposes documentsCountable and
blastSyncable as valid keys for all versions which allows pre-v12 contracts to
smuggle new primary-tree flags; update the validation flow so those keys are
only accepted when protocolVersion >= 12: remove or hide documentsCountable and
blastSyncable from the shared, protocol-agnostic document meta acceptance and
instead validate them in the v12+ document meta validator (or add an explicit
check in the document meta deserialization/reparse path) that rejects or strips
these fields for protocolVersion < 12; ensure the deserializer/reparser path
that handles document meta enforces this reject/strip behavior so legacy
contracts cannot be interpreted with v12 primary-tree semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66bc0296-6d90-4e7e-856b-f361772932a2
📒 Files selected for processing (27)
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/accessors/mod.rspackages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-dpp/src/data_contract/document_type/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rspackages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-drive/src/drive/contract/update/update_contract/v0/mod.rspackages/rs-drive/src/drive/document/delete/delete_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/delete/internal/add_estimation_costs_for_remove_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_document_from_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/mod.rspackages/rs-drive/src/drive/document/primary_key_tree_type.rspackages/rs-drive/src/fees/op.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/v12.rs
✅ Files skipped from review due to trivial changes (5)
- packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rs
- packages/rs-drive/src/fees/op.rs
- packages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rs
- packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/rs-drive/src/drive/document/delete/internal/add_estimation_costs_for_remove_document_to_primary_storage/v0/mod.rs
- packages/rs-drive/src/drive/document/delete/delete_document_for_contract_operations/v0/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs
- packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs
- packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs
- packages/rs-drive/src/drive/contract/update/update_contract/v0/mod.rs
- packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs
|
Re-checked the current head ( The remaining blocker is the same old-state/deserialization hole, and it still reproduces on the current SHA:
So from my side the answer is still: not good yet — I still want that deserialization / old-state migration hole closed before I clear this. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This force-push fixes the previous dead-on-arrival parser bug: v12 contracts can now actually parse documentsCountable / blastSyncable, and the Drive insert/delete/cost paths consistently derive the matching tree type. But the replacement change removed the pre-v12 rejection entirely, which reopens a consensus/storage-layout problem across upgrades: protocol-11 contracts can now persist those keys in raw schema JSON, be created over a NormalTree, and then get reinterpreted after a v12 upgrade as CountTree / ProvableCountTree document types when the stored contract is deserialized with the current platform version. I also confirmed the new immutability guards still have no direct regression tests.
Reviewed commit: b1064f8
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 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/v1/mod.rs`:
- [BLOCKING] lines 666-670: Relaxing the V1 parser reopens pre-v12 schema smuggling for countable document types
These lines now intentionally ignore `documentsCountable` / `blastSyncable` in the v1 parser. That is unsafe because the shared document meta-schema already allows both keys (`packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json:508-515`), and pre-v12 platform versions still dispatch `DocumentType::try_from_schema()` to the v1 parser (`packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rs:35`, `v3.rs:37`). A protocol-11 contract can therefore submit those raw schema fields, pass validation, and get stored with a `DocumentTypeV1` that creates a `NormalTree` at contract insertion time (`packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs:287-307`). Later, Drive fetches stored contracts by deserializing them with the *current* platform version (`packages/rs-drive/src/drive/contract/get_fetch/fetch_contract/v0/mod.rs:81-85`), and that reconstruction path calls `create_document_types_from_document_schemas()` / `DocumentType::try_from_schema()` with the current version (`packages/rs-dpp/src/data_contract/v1/serialization/mod.rs:154-165`). After a network upgrade to v12, the same stored raw schema is reparsed through the v2 parser, which now reads those keys and makes document operations use `CountTree` / `ProvableCountTree` semantics (`packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs:71-77`) even though the underlying tree was created as `NormalTree`. That is a consensus-critical storage-layout mismatch across upgrades. The v1 path still needs to reject or normalize these keys for pre-v12 contracts instead of silently preserving them in serialized state.
In `packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs`:
- [SUGGESTION] lines 183-210: The new countable/blast immutability guards still have no direct regression tests
`validate_config()` now correctly rejects changes to `documents_countable` and `blast_syncable`, which is the guard that prevents an update from changing the primary tree type after creation. But the surrounding test module still only covers the older immutability rules (creation restriction, trade mode, transferable, deletable, keep-history, mutable, bounded-key requirements, security level, etc.). There is still no focused test that builds two v2 document types differing only in `documentsCountable` or `blastSyncable` and asserts `DocumentTypeUpdateError` is returned. Because these flags affect storage/proof layout, they deserve the same regression coverage as the neighboring config invariants.
| // Note: documentsCountable / blastSyncable schema keys are intentionally | ||
| // ignored here. The v1 parser produces DocumentTypeV1 which has no countable | ||
| // fields. When protocol v12+ is active, the v2 parser is used instead, which | ||
| // reads these keys and produces DocumentTypeV2. The v1 parser should never | ||
| // reject unknown keys — it simply doesn't map them to its output type. |
There was a problem hiding this comment.
🔴 Blocking: Relaxing the V1 parser reopens pre-v12 schema smuggling for countable document types
These lines now intentionally ignore documentsCountable / blastSyncable in the v1 parser. That is unsafe because the shared document meta-schema already allows both keys (packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json:508-515), and pre-v12 platform versions still dispatch DocumentType::try_from_schema() to the v1 parser (packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rs:35, v3.rs:37). A protocol-11 contract can therefore submit those raw schema fields, pass validation, and get stored with a DocumentTypeV1 that creates a NormalTree at contract insertion time (packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs:287-307). Later, Drive fetches stored contracts by deserializing them with the current platform version (packages/rs-drive/src/drive/contract/get_fetch/fetch_contract/v0/mod.rs:81-85), and that reconstruction path calls create_document_types_from_document_schemas() / DocumentType::try_from_schema() with the current version (packages/rs-dpp/src/data_contract/v1/serialization/mod.rs:154-165). After a network upgrade to v12, the same stored raw schema is reparsed through the v2 parser, which now reads those keys and makes document operations use CountTree / ProvableCountTree semantics (packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs:71-77) even though the underlying tree was created as NormalTree. That is a consensus-critical storage-layout mismatch across upgrades. The v1 path still needs to reject or normalize these keys for pre-v12 contracts instead of silently preserving them in serialized state.
source: ['codex-general', 'codex-security-auditor']
🤖 Fix this 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/v1/mod.rs`:
- [BLOCKING] lines 666-670: Relaxing the V1 parser reopens pre-v12 schema smuggling for countable document types
These lines now intentionally ignore `documentsCountable` / `blastSyncable` in the v1 parser. That is unsafe because the shared document meta-schema already allows both keys (`packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json:508-515`), and pre-v12 platform versions still dispatch `DocumentType::try_from_schema()` to the v1 parser (`packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rs:35`, `v3.rs:37`). A protocol-11 contract can therefore submit those raw schema fields, pass validation, and get stored with a `DocumentTypeV1` that creates a `NormalTree` at contract insertion time (`packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs:287-307`). Later, Drive fetches stored contracts by deserializing them with the *current* platform version (`packages/rs-drive/src/drive/contract/get_fetch/fetch_contract/v0/mod.rs:81-85`), and that reconstruction path calls `create_document_types_from_document_schemas()` / `DocumentType::try_from_schema()` with the current version (`packages/rs-dpp/src/data_contract/v1/serialization/mod.rs:154-165`). After a network upgrade to v12, the same stored raw schema is reparsed through the v2 parser, which now reads those keys and makes document operations use `CountTree` / `ProvableCountTree` semantics (`packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs:71-77`) even though the underlying tree was created as `NormalTree`. That is a consensus-critical storage-layout mismatch across upgrades. The v1 path still needs to reject or normalize these keys for pre-v12 contracts instead of silently preserving them in serialized state.
| if new_document_type.documents_countable() != self.documents_countable() { | ||
| return SimpleConsensusValidationResult::new_with_error( | ||
| DocumentTypeUpdateError::new( | ||
| self.data_contract_id(), | ||
| self.name(), | ||
| format!( | ||
| "document type can not change whether its documents are countable: changing from {} to {}", | ||
| self.documents_countable(), | ||
| new_document_type.documents_countable() | ||
| ), | ||
| ) | ||
| .into(), | ||
| ); | ||
| } | ||
|
|
||
| if new_document_type.blast_syncable() != self.blast_syncable() { | ||
| return SimpleConsensusValidationResult::new_with_error( | ||
| DocumentTypeUpdateError::new( | ||
| self.data_contract_id(), | ||
| self.name(), | ||
| format!( | ||
| "document type can not change whether it is blast syncable: changing from {} to {}", | ||
| self.blast_syncable(), | ||
| new_document_type.blast_syncable() | ||
| ), | ||
| ) | ||
| .into(), | ||
| ); |
There was a problem hiding this comment.
🟡 Suggestion: The new countable/blast immutability guards still have no direct regression tests
validate_config() now correctly rejects changes to documents_countable and blast_syncable, which is the guard that prevents an update from changing the primary tree type after creation. But the surrounding test module still only covers the older immutability rules (creation restriction, trade mode, transferable, deletable, keep-history, mutable, bounded-key requirements, security level, etc.). There is still no focused test that builds two v2 document types differing only in documentsCountable or blastSyncable and asserts DocumentTypeUpdateError is returned. Because these flags affect storage/proof layout, they deserve the same regression coverage as the neighboring config invariants.
source: ['claude-general', 'claude-security-auditor', 'claude-rust-quality', 'codex-general', 'codex-rust-quality']
🤖 Fix this 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/methods/validate_update/v0/mod.rs`:
- [SUGGESTION] lines 183-210: The new countable/blast immutability guards still have no direct regression tests
`validate_config()` now correctly rejects changes to `documents_countable` and `blast_syncable`, which is the guard that prevents an update from changing the primary tree type after creation. But the surrounding test module still only covers the older immutability rules (creation restriction, trade mode, transferable, deletable, keep-history, mutable, bounded-key requirements, security level, etc.). There is still no focused test that builds two v2 document types differing only in `documentsCountable` or `blastSyncable` and asserts `DocumentTypeUpdateError` is returned. Because these flags affect storage/proof layout, they deserve the same regression coverage as the neighboring config invariants.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This force-push refactors rs-drive so inserts, deletes, and estimation paths all compute the primary-key tree type through one helper, which does remove some duplication. But it does not fix the actual blocker from the previous SHA: pre-v12 contracts can still persist documentsCountable / blastSyncable in raw schema JSON, and after a v12 upgrade those stored schemas are reparsed into count/provable document types even though the underlying tree was originally created as NormalTree. I also re-checked the non-blocking follow-up and the new immutability guards still lack direct regression tests in validate_update.
Reviewed commit: cc855a8
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 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/v1/mod.rs`:
- [BLOCKING] lines 666-670: Pre-v12 contracts can still smuggle count-tree flags and be reinterpreted after upgrade
The v1 parser still intentionally ignores `documentsCountable` / `blastSyncable` instead of rejecting or normalizing them, while the shared document meta-schema still allows those keys. That means a pre-v12 contract can carry the raw fields in `document_schemas`, pass validation as a `DocumentTypeV1`, and be inserted with a physical `NormalTree`. Later, stored contracts are reconstructed from serialized schema bytes with the current platform version (`packages/rs-dpp/src/data_contract/v1/serialization/mod.rs:154-165` and `packages/rs-drive/src/drive/contract/get_fetch/fetch_contract/v0/mod.rs:81-85`). Once the node is on protocol v12, that same raw schema is reparsed through `DocumentTypeV2::try_from_schema`, which revives the flags and makes `primary_key_tree_type` select `CountTree` / `ProvableCountTree` for runtime document operations (`packages/rs-drive/src/drive/document/primary_key_tree_type.rs:18-48`, plus the new call sites in the touched insert/delete/estimation files). So this refactor centralizes the tree selection, but it still centralizes the wrong assumption: old on-disk `NormalTree` contracts can be reinterpreted post-upgrade as count/provable trees. That is a storage-layout mismatch in consensus-critical code, and this SHA does not close it.
In `packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs`:
- [SUGGESTION] lines 183-210: The countable and blast-syncable immutability guards still have no dedicated tests
`validate_config()` now rejects changes to `documents_countable` and `blast_syncable`, which is the right invariant because those flags determine the physical primary-key tree layout. But the test module in this file still only has explicit regression cases for the older immutable properties; a targeted search shows the new error strings at lines 189 and 204, yet there is still no corresponding test that builds two document types differing only in these flags and asserts `DocumentTypeUpdateError` is returned. Because these guards protect storage layout rather than a cosmetic schema field, they deserve the same direct coverage as the neighboring transferability/history/mutability/security-level checks.
| // Note: documentsCountable / blastSyncable schema keys are intentionally | ||
| // ignored here. The v1 parser produces DocumentTypeV1 which has no countable | ||
| // fields. When protocol v12+ is active, the v2 parser is used instead, which | ||
| // reads these keys and produces DocumentTypeV2. The v1 parser should never | ||
| // reject unknown keys — it simply doesn't map them to its output type. |
There was a problem hiding this comment.
🔴 Blocking: Pre-v12 contracts can still smuggle count-tree flags and be reinterpreted after upgrade
The v1 parser still intentionally ignores documentsCountable / blastSyncable instead of rejecting or normalizing them, while the shared document meta-schema still allows those keys. That means a pre-v12 contract can carry the raw fields in document_schemas, pass validation as a DocumentTypeV1, and be inserted with a physical NormalTree. Later, stored contracts are reconstructed from serialized schema bytes with the current platform version (packages/rs-dpp/src/data_contract/v1/serialization/mod.rs:154-165 and packages/rs-drive/src/drive/contract/get_fetch/fetch_contract/v0/mod.rs:81-85). Once the node is on protocol v12, that same raw schema is reparsed through DocumentTypeV2::try_from_schema, which revives the flags and makes primary_key_tree_type select CountTree / ProvableCountTree for runtime document operations (packages/rs-drive/src/drive/document/primary_key_tree_type.rs:18-48, plus the new call sites in the touched insert/delete/estimation files). So this refactor centralizes the tree selection, but it still centralizes the wrong assumption: old on-disk NormalTree contracts can be reinterpreted post-upgrade as count/provable trees. That is a storage-layout mismatch in consensus-critical code, and this SHA does not close it.
source: ['codex-general', 'codex-security-auditor', 'codex-rust-quality']
🤖 Fix this 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/v1/mod.rs`:
- [BLOCKING] lines 666-670: Pre-v12 contracts can still smuggle count-tree flags and be reinterpreted after upgrade
The v1 parser still intentionally ignores `documentsCountable` / `blastSyncable` instead of rejecting or normalizing them, while the shared document meta-schema still allows those keys. That means a pre-v12 contract can carry the raw fields in `document_schemas`, pass validation as a `DocumentTypeV1`, and be inserted with a physical `NormalTree`. Later, stored contracts are reconstructed from serialized schema bytes with the current platform version (`packages/rs-dpp/src/data_contract/v1/serialization/mod.rs:154-165` and `packages/rs-drive/src/drive/contract/get_fetch/fetch_contract/v0/mod.rs:81-85`). Once the node is on protocol v12, that same raw schema is reparsed through `DocumentTypeV2::try_from_schema`, which revives the flags and makes `primary_key_tree_type` select `CountTree` / `ProvableCountTree` for runtime document operations (`packages/rs-drive/src/drive/document/primary_key_tree_type.rs:18-48`, plus the new call sites in the touched insert/delete/estimation files). So this refactor centralizes the tree selection, but it still centralizes the wrong assumption: old on-disk `NormalTree` contracts can be reinterpreted post-upgrade as count/provable trees. That is a storage-layout mismatch in consensus-critical code, and this SHA does not close it.
| if new_document_type.documents_countable() != self.documents_countable() { | ||
| return SimpleConsensusValidationResult::new_with_error( | ||
| DocumentTypeUpdateError::new( | ||
| self.data_contract_id(), | ||
| self.name(), | ||
| format!( | ||
| "document type can not change whether its documents are countable: changing from {} to {}", | ||
| self.documents_countable(), | ||
| new_document_type.documents_countable() | ||
| ), | ||
| ) | ||
| .into(), | ||
| ); | ||
| } | ||
|
|
||
| if new_document_type.blast_syncable() != self.blast_syncable() { | ||
| return SimpleConsensusValidationResult::new_with_error( | ||
| DocumentTypeUpdateError::new( | ||
| self.data_contract_id(), | ||
| self.name(), | ||
| format!( | ||
| "document type can not change whether it is blast syncable: changing from {} to {}", | ||
| self.blast_syncable(), | ||
| new_document_type.blast_syncable() | ||
| ), | ||
| ) | ||
| .into(), | ||
| ); |
There was a problem hiding this comment.
🟡 Suggestion: The countable and blast-syncable immutability guards still have no dedicated tests
validate_config() now rejects changes to documents_countable and blast_syncable, which is the right invariant because those flags determine the physical primary-key tree layout. But the test module in this file still only has explicit regression cases for the older immutable properties; a targeted search shows the new error strings at lines 189 and 204, yet there is still no corresponding test that builds two document types differing only in these flags and asserts DocumentTypeUpdateError is returned. Because these guards protect storage layout rather than a cosmetic schema field, they deserve the same direct coverage as the neighboring transferability/history/mutability/security-level checks.
source: ['codex-general', 'codex-rust-quality']
🤖 Fix this 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/methods/validate_update/v0/mod.rs`:
- [SUGGESTION] lines 183-210: The countable and blast-syncable immutability guards still have no dedicated tests
`validate_config()` now rejects changes to `documents_countable` and `blast_syncable`, which is the right invariant because those flags determine the physical primary-key tree layout. But the test module in this file still only has explicit regression cases for the older immutable properties; a targeted search shows the new error strings at lines 189 and 204, yet there is still no corresponding test that builds two document types differing only in these flags and asserts `DocumentTypeUpdateError` is returned. Because these guards protect storage layout rather than a cosmetic schema field, they deserve the same direct coverage as the neighboring transferability/history/mutability/security-level checks.
Adds two new fields to DocumentTypeV2: - `documents_countable: bool` — primary key tree uses CountTree for O(1) total document count queries - `range_countable: bool` — primary key tree uses ProvableCountTree for provable range counting. Implies documents_countable = true. Changes: - DocumentTypeV2 struct with both fields - V2 variant added to DocumentType/DocumentTypeRef/DocumentTypeMutRef - DocumentTypeV2Getters/Setters traits - Schema parsing v2 with protocol version 12+ gating - Drive: CountTree for countable, ProvableCountTree for range_countable - DocumentTypePrimaryKeyTreeType extension trait in rs-drive with own versioning (primary_key_tree_type: 0) - Meta-schema: documentsCountable and rangeCountable properties - Update validation: reject changes to countable/range_countable - All stateless insert/delete/estimation paths use correct tree type Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
QuantumExplorer
force-pushed
the
feat/documents-countable
branch
from
cc855a8 to
51d1808
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs`:
- Around line 77-89: The dispatcher is choosing DocumentType::try_from_schema_v2
based solely on platform_version, which can mis-parse legacy contracts; modify
the dispatcher in try_from_schema so that before calling
DocumentType::try_from_schema_v2 you check data_contract_system_version (the
stored contract system version) and if data_contract_system_version < 2 either
(a) route to the v1 parser (e.g., call the existing v1 path) or (b)
sanitize/remove V2-only flags like documentsCountable/rangeCountable from schema
when full_validation == false, and only allow V2 parsing when
data_contract_system_version >= 2; update uses of full_validation and
validation_operations accordingly to ensure safe deserialization of pre-v12
contracts.
In
`@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs`:
- Around line 45-63: The code currently reads documents_countable and
range_countable from the schema (variables documents_countable and
range_countable in try_from_schema v2) and later applies them unconditionally,
which allows legacy schemas to be reinterpreted; change try_from_schema (mod.rs)
so these two flags are only honored when the persisted contract/parser version
indicates V2 (or newer) — e.g., check the persisted contract/parser version
field available during deserialization and if the version is older than the V2
threshold, ignore/sanitize DOCUMENTS_COUNTABLE and RANGE_COUNTABLE (force false
or use the original NormalTree semantics) before applying them at the
application site (the code path that currently uses the variables around lines
82–83). Ensure the gating uses the persisted version from the contract/metadata
used to parse the schema rather than the current runtime platform version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 470928c6-ae21-43df-b985-629e49e53ca4
📒 Files selected for processing (30)
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/accessors/mod.rspackages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-dpp/src/data_contract/document_type/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rspackages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-drive/src/drive/contract/update/update_contract/v0/mod.rspackages/rs-drive/src/drive/document/delete/delete_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/delete/internal/add_estimation_costs_for_remove_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_document_from_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/mod.rspackages/rs-drive/src/drive/document/primary_key_tree_type.rspackages/rs-drive/src/fees/op.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v1.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v2.rspackages/rs-platform-version/src/version/v12.rs
✅ Files skipped from review due to trivial changes (5)
- packages/rs-drive/src/drive/document/delete/delete_document_for_contract_operations/v0/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rs
- packages/rs-drive/src/drive/document/insert/add_document_for_contract_operations/v0/mod.rs
- packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs
- packages/rs-drive/src/fees/op.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/rs-drive/src/drive/document/delete/remove_document_from_primary_storage/v0/mod.rs
- packages/rs-drive/src/drive/document/mod.rs
- packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs
- packages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rs
- packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json
- packages/rs-platform-version/src/version/v12.rs
- packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs
- packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs
- packages/rs-drive/src/drive/contract/update/update_contract/v0/mod.rs
- packages/rs-drive/src/drive/document/primary_key_tree_type.rs
- packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs
- packages/rs-drive/src/drive/document/delete/internal/add_estimation_costs_for_remove_document_to_primary_storage/v0/mod.rs
| 2 => DocumentType::try_from_schema_v2( | ||
| data_contract_id, | ||
| data_contract_system_version, | ||
| contract_config_version, | ||
| name, | ||
| schema, | ||
| schema_defs, | ||
| token_configurations, | ||
| data_contact_config, | ||
| full_validation, | ||
| validation_operations, | ||
| platform_version, | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find deserialization call sites and whether `full_validation = false` is used.
rg -n -C3 --type=rust 'versioned_deserialize\s*\('
rg -n -C3 --type=rust 'versioned_deserialize\s*\([^)]*false'
# 2) Inspect document-type parser version routing and where it is sourced from.
rg -n -C4 --type=rust 'document_type_versions|class_method_versions|try_from_schema'
# 3) Locate schema handling of V2-only keys.
rg -n -C3 --type=rust '"documentsCountable"|"rangeCountable"|DOCUMENTS_COUNTABLE|RANGE_COUNTABLE'Repository: dashpay/platform
Length of output: 50373
🏁 Script executed:
cat -n packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs | head -100Repository: dashpay/platform
Length of output: 4285
🏁 Script executed:
# Check the versioning config and how parser version is selected
rg -n "try_from_schema_v2|SCHEMA_VERSION|parser.*version" packages/rs-dpp/src/data_contract/ -A 2 -B 2 | head -80Repository: dashpay/platform
Length of output: 2147
🏁 Script executed:
# Look for documentsCountable and rangeCountable handling in V2
rg -n "documentsCountable|rangeCountable" packages/rs-dpp/src/ -B 3 -A 3 | head -100Repository: dashpay/platform
Length of output: 3040
🏁 Script executed:
# Check if contract version is evaluated before platform version in deserialization
rg -n "try_from_schema|document_type_version" packages/rs-dpp/src/data_contract/mod.rs -B 5 -A 5 | head -60Repository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Check how deserialization of existing contracts uses version info
rg -n "versioned_deserialize.*DataContract\|from_bytes.*schema" packages/rs-dpp/src/data_contract/mod.rs -B 3 -A 3Repository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Look for any migration or version guarding during contract deserialization
rg -n "system_version|contract_version.*from" packages/rs-dpp/src/data_contract/ -B 2 -A 2 | head -100Repository: dashpay/platform
Length of output: 9192
🏁 Script executed:
# Check if data_contract_system_version is used to guard the parser selection
rg -n "data_contract_system_version|contract_structure_version" packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/ -B 2 -A 2Repository: dashpay/platform
Length of output: 8092
🏁 Script executed:
# Check how deserialization flow works - from versioned_deserialize to try_from_schema
cat -n packages/rs-dpp/src/data_contract/mod.rs | sed -n '145,200p'Repository: dashpay/platform
Length of output: 2360
🏁 Script executed:
# Check if there's any version check before trying V2 parser, or safeguard against legacy contracts
rg -n "contract_structure_version|system_version" packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/ -B 3 -A 3Repository: dashpay/platform
Length of output: 3613
🏁 Script executed:
# Check how try_from_platform_versioned routes to try_from_schema
cat -n packages/rs-dpp/src/data_contract/mod.rs | sed -n '200,280p'Repository: dashpay/platform
Length of output: 2906
🏁 Script executed:
# Look for where the contract structure version is determined and if it gates parser selection
rg -n "try_from_platform_versioned" packages/rs-dpp/src/data_contract/ -A 20 | head -100Repository: dashpay/platform
Length of output: 9607
🏁 Script executed:
# Check if serialization includes schema version or if schema is always re-parsed from raw value
rg -n "DataContractInSerializationFormat" packages/rs-dpp/src/data_contract/serialization/ -B 2 -A 8 | head -80Repository: dashpay/platform
Length of output: 148
🏁 Script executed:
# Find where DataContractInSerializationFormat is defined
fd -i "serialization" packages/rs-dpp/src/data_contract/ -type f | head -20Repository: dashpay/platform
Length of output: 230
🏁 Script executed:
# Check main contract try_from_platform_versioned implementation
rg -n "fn try_from_platform_versioned" packages/rs-dpp/src/data_contract/mod.rs -A 30 | head -80Repository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Check the document types creation method
rg -n "create_document_types_from_document_schemas" packages/rs-dpp/src/data_contract/ -A 15 | head -100Repository: dashpay/platform
Length of output: 8591
🏁 Script executed:
# Check the actual dispatcher logic more carefully - is there any guard based on contract version?
cat -n packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs | sed -n '28,96p'Repository: dashpay/platform
Length of output: 2998
🏁 Script executed:
# Check if data_contract_system_version is used to gate V2 parser access
rg -n "data_contract_system_version.*2|system_version.*[>=<].*2" packages/rs-dpp/src/data_contract/ -B 2 -A 2Repository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Look at v2 parser implementation to see if it validates contract version
cat -n packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs | head -120Repository: dashpay/platform
Length of output: 5863
🏁 Script executed:
# Check if there's any contract version guard preventing V2-specific behavior
rg -n "data_contract_system_version.*<|system_version.*<" packages/rs-dpp/src/data_contract/document_type/ -B 2 -A 2Repository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Check the comment more carefully - look for what "primary key tree type" is and if it's affected by countable flags
rg -n "primary.*key|tree.*type|NormalTree" packages/rs-dpp/src/data_contract/ -B 1 -A 1 | head -40Repository: dashpay/platform
Length of output: 3723
🏁 Script executed:
# Verify: does the V2 parser always get used for deserialization in v12+, or is there a guard?
rg -n "CONTRACT_VERSIONS_V4|contract_structure_version.*2" packages/rs-dpp/ -B 2 -A 2 | head -60Repository: dashpay/platform
Length of output: 759
🏁 Script executed:
# Final verification: check if full_validation is true in schema parsing paths during deserialization
rg -n "create_document_types_from_document_schemas" packages/rs-dpp/src/data_contract/ -B 5 -A 1 | grep -E "(full_validation|create_document)" | head -20Repository: dashpay/platform
Length of output: 2971
🏁 Script executed:
# Check the immutability safeguard mentioned in code
cat -n packages/rs-dpp/src/data_contract/document_type/index_level/mod.rs | sed -n '285,310p'Repository: dashpay/platform
Length of output: 1139
Clarify version-gating strategy for V2 schema parser on pre-v12 contracts.
The V2 parser for document schemas (line 77) is dispatched based on the current platform version (v12+), not the contract's stored system version. When legacy contracts stored under v11 are deserialized in v12+, the V2 parser will read documentsCountable and rangeCountable fields if present, affecting the primary key tree type (NormalTree → CountTree/ProvableCountTree). While the code comment acknowledges this and notes that updates are gated by immutability checks (line 291), the parser selection itself has no explicit guard.
Strengthen this by either:
- Adding a check: if
data_contract_system_version < 2, reject or sanitize V2-only flags during deserialization, or - Document the defensive assumption (that pre-v12 contracts never contain these flags) more explicitly in the dispatcher.
The current immutability validation prevents updates from changing countable state, but does not prevent initial deserialization with full_validation=false from misinterpreting a legacy schema that happens to include these optional fields.
🤖 Prompt for AI Agents
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`
around lines 77 - 89, The dispatcher is choosing
DocumentType::try_from_schema_v2 based solely on platform_version, which can
mis-parse legacy contracts; modify the dispatcher in try_from_schema so that
before calling DocumentType::try_from_schema_v2 you check
data_contract_system_version (the stored contract system version) and if
data_contract_system_version < 2 either (a) route to the v1 parser (e.g., call
the existing v1 path) or (b) sanitize/remove V2-only flags like
documentsCountable/rangeCountable from schema when full_validation == false, and
only allow V2 parsing when data_contract_system_version >= 2; update uses of
full_validation and validation_operations accordingly to ensure safe
deserialization of pre-v12 contracts.
| let documents_countable = schema_map_opt | ||
| .as_ref() | ||
| .and_then(|schema_map| { | ||
| Value::inner_optional_bool_value(schema_map, DOCUMENTS_COUNTABLE) | ||
| .map_err(consensus_or_protocol_value_error) | ||
| .transpose() | ||
| }) | ||
| .transpose()? | ||
| .unwrap_or(false); | ||
|
|
||
| let range_countable = schema_map_opt | ||
| .as_ref() | ||
| .and_then(|schema_map| { | ||
| Value::inner_optional_bool_value(schema_map, RANGE_COUNTABLE) | ||
| .map_err(consensus_or_protocol_value_error) | ||
| .transpose() | ||
| }) | ||
| .transpose()? | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
Blocker: prevent legacy schema flag replay during deserialization.
Line 45–63 parses documentsCountable / rangeCountable directly from schema content, and Line 82–83 applies them unconditionally. This allows legacy stored schemas to be reinterpreted as countable/range-countable after upgrade, which can diverge from the already-created primary-key tree layout (originally NormalTree).
Please gate these fields by persisted contract/parser version (not just current runtime platform context), or sanitize legacy schemas before applying V2 flags.
Also applies to: 82-83
🤖 Prompt for AI Agents
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/v2/mod.rs`
around lines 45 - 63, The code currently reads documents_countable and
range_countable from the schema (variables documents_countable and
range_countable in try_from_schema v2) and later applies them unconditionally,
which allows legacy schemas to be reinterpreted; change try_from_schema (mod.rs)
so these two flags are only honored when the persisted contract/parser version
indicates V2 (or newer) — e.g., check the persisted contract/parser version
field available during deserialization and if the version is older than the V2
threshold, ignore/sanitize DOCUMENTS_COUNTABLE and RANGE_COUNTABLE (force false
or use the original NormalTree semantics) before applying them at the
application site (the code path that currently uses the variables around lines
82–83). Ensure the gating uses the persisted version from the contract/metadata
used to parse the schema rather than the current runtime platform version.
|
Re-checked the current head ( The remaining blocker is the same old-state/deserialization hole, and it is still live on this SHA:
So a pre-v12 contract can still be accepted/stored with those raw keys ignored by the old parser, get its physical primary tree created as So from my side: not good yet. I'd want the parser/deserialization path gated on the persisted contract system version (or those v2-only keys stripped/rejected for older contracts during reload), and then I'd be happy to clear it. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both agents converge on a real storage-layout reinterpretation vulnerability: pre-v12 contracts can carry documentsCountable/rangeCountable in their raw schema, get a physical NormalTree, and then be deserialized through the v2 parser post-upgrade, causing all fee estimation paths to use the wrong tree type. The immutability guards for these flags also lack the regression tests that every other guard in the same file has. Additional architectural concerns around tree-type logic duplication are valid but lower priority.
Reviewed commit: 51d1808
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)
1 additional finding
🔴 blocking: V1 parser preserves smuggled countable flags that the V2 parser will revive after v12 upgrade
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs (lines 666-684)
The v1 parser (lines 666-670) explicitly documents that it ignores documentsCountable/rangeCountable, but these keys are preserved in the raw schema Value stored on DocumentTypeV1.schema (line 684) and persisted to GroveDB. Meanwhile, the document meta-schema (document-meta.json:508-514) explicitly declares both fields as allowed boolean properties at any protocol version.
After v12 activation, stored contracts are deserialized using the current platform_version, which dispatches to try_from_schema: 2 (confirmed in v4.rs:36). The v2 parser (try_from_schema/v2/mod.rs:45-63) reads these flags from the raw schema and sets them on DocumentTypeV2 (line 82). This changes the return value of primary_key_tree_type() from NormalTree to CountTree/ProvableCountTree for a tree that was physically created as NormalTree.
The 6+ call sites using primary_key_tree_type() (document insert, delete, estimation cost paths) will then compute fees against the wrong tree type. While all nodes agree on the same incorrect result (no consensus fork), fee calculations are wrong, and future count query support would produce incorrect results on these contracts.
The comment at v2/mod.rs:36-42 claims this is safe because "the primary key tree type is set correctly at contract creation time," but this conflates one-time physical tree creation with per-operation tree type selection during deserialization.
Fix: Strip documentsCountable and rangeCountable from the schema Value in the v1 parser before storing it on DocumentTypeV1.schema. This prevents the flags from reaching persistent storage regardless of which parser later deserializes the contract. Alternatively, have the v2 parser force these flags to false for contracts with created_at_epoch predating v12.
🤖 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/v1/mod.rs`:
- [BLOCKING] lines 666-684: V1 parser preserves smuggled countable flags that the V2 parser will revive after v12 upgrade
The v1 parser (lines 666-670) explicitly documents that it ignores `documentsCountable`/`rangeCountable`, but these keys are preserved in the raw `schema` Value stored on `DocumentTypeV1.schema` (line 684) and persisted to GroveDB. Meanwhile, the document meta-schema (`document-meta.json:508-514`) explicitly declares both fields as allowed boolean properties at any protocol version.
After v12 activation, stored contracts are deserialized using the current `platform_version`, which dispatches to `try_from_schema: 2` (confirmed in `v4.rs:36`). The v2 parser (`try_from_schema/v2/mod.rs:45-63`) reads these flags from the raw schema and sets them on `DocumentTypeV2` (line 82). This changes the return value of `primary_key_tree_type()` from `NormalTree` to `CountTree`/`ProvableCountTree` for a tree that was physically created as `NormalTree`.
The 6+ call sites using `primary_key_tree_type()` (document insert, delete, estimation cost paths) will then compute fees against the wrong tree type. While all nodes agree on the same incorrect result (no consensus fork), fee calculations are wrong, and future count query support would produce incorrect results on these contracts.
The comment at `v2/mod.rs:36-42` claims this is safe because "the primary key tree type is set correctly at contract creation time," but this conflates one-time physical tree creation with per-operation tree type selection during deserialization.
**Fix**: Strip `documentsCountable` and `rangeCountable` from the schema `Value` in the v1 parser before storing it on `DocumentTypeV1.schema`. This prevents the flags from reaching persistent storage regardless of which parser later deserializes the contract. Alternatively, have the v2 parser force these flags to `false` for contracts with `created_at_epoch` predating v12.
In `packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs`:
- [SUGGESTION] lines 183-211: No regression tests for documents_countable and range_countable immutability guards
The `validate_config()` method correctly rejects changes to `documents_countable` (line 183) and `range_countable` (line 198), which is critical because these flags determine the physical primary key tree layout. However, the `validate_config` test module (lines 288-984) has explicit regression tests for all 9 other immutable config properties — `creation_restriction_mode`, `trade_mode`, `transferable`, `can_be_deleted`, `keep_history`, `mutable`, `encryption_key_req`, `decryption_key_req`, `security_level_requirement` — but none for the two new guards.
Because these flags protect physical storage layout (not just schema semantics), they are higher-stakes than most config immutability checks. A future refactor could inadvertently remove them without any test catching the regression. Add two tests following the existing `should_return_invalid_result_when_<property>_is_changed()` pattern.
In `packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs`:
- [SUGGESTION] lines 288-315: Tree-type selection logic duplicated instead of using centralized primary_key_tree_type()
The contract insert path (lines 288-315) and update path (`update_contract/v0/mod.rs:352-380`) both inline the same if/else chain for selecting NormalTree vs CountTree vs ProvableCountTree. Document operations use the centralized `DocumentTypePrimaryKeyTreeType::primary_key_tree_type()` from `primary_key_tree_type.rs`, which participates in version dispatch (`platform_version.drive.methods.document.primary_key_tree_type`). The inline copies skip version dispatch entirely — if a v1 of `primary_key_tree_type` is introduced with different selection logic, contract creation will diverge from document operations. Both sites should call the centralized method to maintain a single source of truth.
| if new_document_type.documents_countable() != self.documents_countable() { | ||
| return SimpleConsensusValidationResult::new_with_error( | ||
| DocumentTypeUpdateError::new( | ||
| self.data_contract_id(), | ||
| self.name(), | ||
| format!( | ||
| "document type can not change whether its documents are countable: changing from {} to {}", | ||
| self.documents_countable(), | ||
| new_document_type.documents_countable() | ||
| ), | ||
| ) | ||
| .into(), | ||
| ); | ||
| } | ||
|
|
||
| if new_document_type.range_countable() != self.range_countable() { | ||
| return SimpleConsensusValidationResult::new_with_error( | ||
| DocumentTypeUpdateError::new( | ||
| self.data_contract_id(), | ||
| self.name(), | ||
| format!( | ||
| "document type can not change whether it is range countable: changing from {} to {}", | ||
| self.range_countable(), | ||
| new_document_type.range_countable() | ||
| ), | ||
| ) | ||
| .into(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: No regression tests for documents_countable and range_countable immutability guards
The validate_config() method correctly rejects changes to documents_countable (line 183) and range_countable (line 198), which is critical because these flags determine the physical primary key tree layout. However, the validate_config test module (lines 288-984) has explicit regression tests for all 9 other immutable config properties — creation_restriction_mode, trade_mode, transferable, can_be_deleted, keep_history, mutable, encryption_key_req, decryption_key_req, security_level_requirement — but none for the two new guards.
Because these flags protect physical storage layout (not just schema semantics), they are higher-stakes than most config immutability checks. A future refactor could inadvertently remove them without any test catching the regression. Add two tests following the existing should_return_invalid_result_when_<property>_is_changed() pattern.
source: ['claude', 'codex']
🤖 Fix this 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/methods/validate_update/v0/mod.rs`:
- [SUGGESTION] lines 183-211: No regression tests for documents_countable and range_countable immutability guards
The `validate_config()` method correctly rejects changes to `documents_countable` (line 183) and `range_countable` (line 198), which is critical because these flags determine the physical primary key tree layout. However, the `validate_config` test module (lines 288-984) has explicit regression tests for all 9 other immutable config properties — `creation_restriction_mode`, `trade_mode`, `transferable`, `can_be_deleted`, `keep_history`, `mutable`, `encryption_key_req`, `decryption_key_req`, `security_level_requirement` — but none for the two new guards.
Because these flags protect physical storage layout (not just schema semantics), they are higher-stakes than most config immutability checks. A future refactor could inadvertently remove them without any test catching the regression. Add two tests following the existing `should_return_invalid_result_when_<property>_is_changed()` pattern.
| if document_type.range_countable() { | ||
| // Use a ProvableCountTree for range countable support (implies countable) | ||
| let path_items: Vec<Vec<u8>> = type_path.iter().map(|s| s.to_vec()).collect(); | ||
| batch_operations.push( | ||
| LowLevelDriveOperation::for_known_path_key_empty_provable_count_tree( | ||
| path_items, | ||
| vec![0], | ||
| storage_flags.as_ref(), | ||
| ), | ||
| ); | ||
| } else if document_type.documents_countable() { | ||
| // Use a CountTree so total document count is available in O(1) | ||
| let path_items: Vec<Vec<u8>> = type_path.iter().map(|s| s.to_vec()).collect(); | ||
| batch_operations.push(LowLevelDriveOperation::for_known_path_key_empty_count_tree( | ||
| path_items, | ||
| vec![0], | ||
| storage_flags.as_ref(), | ||
| )); | ||
| } else { | ||
| let key_info = Key(vec![0]); | ||
| self.batch_insert_empty_tree( | ||
| type_path, | ||
| key_info, | ||
| storage_flags.as_ref(), | ||
| &mut batch_operations, | ||
| &platform_version.drive, | ||
| )?; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Tree-type selection logic duplicated instead of using centralized primary_key_tree_type()
The contract insert path (lines 288-315) and update path (update_contract/v0/mod.rs:352-380) both inline the same if/else chain for selecting NormalTree vs CountTree vs ProvableCountTree. Document operations use the centralized DocumentTypePrimaryKeyTreeType::primary_key_tree_type() from primary_key_tree_type.rs, which participates in version dispatch (platform_version.drive.methods.document.primary_key_tree_type). The inline copies skip version dispatch entirely — if a v1 of primary_key_tree_type is introduced with different selection logic, contract creation will diverge from document operations. Both sites should call the centralized method to maintain a single source of truth.
source: ['claude']
🤖 Fix this 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/src/drive/contract/insert/insert_contract/v0/mod.rs`:
- [SUGGESTION] lines 288-315: Tree-type selection logic duplicated instead of using centralized primary_key_tree_type()
The contract insert path (lines 288-315) and update path (`update_contract/v0/mod.rs:352-380`) both inline the same if/else chain for selecting NormalTree vs CountTree vs ProvableCountTree. Document operations use the centralized `DocumentTypePrimaryKeyTreeType::primary_key_tree_type()` from `primary_key_tree_type.rs`, which participates in version dispatch (`platform_version.drive.methods.document.primary_key_tree_type`). The inline copies skip version dispatch entirely — if a v1 of `primary_key_tree_type` is introduced with different selection logic, contract creation will diverge from document operations. Both sites should call the centralized method to maintain a single source of truth.
| impl DocumentTypeV2Getters for DocumentTypeV2 { | ||
| fn documents_countable(&self) -> bool { | ||
| self.documents_countable || self.range_countable | ||
| } | ||
|
|
||
| fn range_countable(&self) -> bool { | ||
| self.range_countable | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Redundant invariant enforcement — documents_countable getter duplicates constructor's OR logic
The documents_countable() getter (line 203) returns self.documents_countable || self.range_countable, but the v2 constructor (try_from_schema/v2/mod.rs:82) already sets v2.documents_countable = documents_countable || range_countable, and set_range_countable() also enforces this invariant. The dual enforcement is harmless but creates ambiguity about which layer is authoritative. If the field always reflects the invariant at construction time, the getter can simply return self.documents_countable. This prevents subtle inconsistencies if a third code path constructs DocumentTypeV2 without the OR.
💡 Suggested change
| impl DocumentTypeV2Getters for DocumentTypeV2 { | |
| fn documents_countable(&self) -> bool { | |
| self.documents_countable || self.range_countable | |
| } | |
| fn range_countable(&self) -> bool { | |
| self.range_countable | |
| } | |
| fn documents_countable(&self) -> bool { | |
| self.documents_countable | |
| } |
source: ['claude']
3 participants
Summary
Adds two new fields to
DocumentTypeV2for O(1) document counting:documents_countable: bool— primary key tree usesCountTree, enabling O(1) total document count queriesblast_syncable: bool— primary key tree usesProvableCountTreefor BLAST sync support. Impliesdocuments_countable = true.Changes
V2variant with full match arm coverageDocumentTypeV2Getters/DocumentTypeV2Setterstry_from_schemav2 parses both"documentsCountable"and"blastSyncable"(protocol version 12+ required)CountTreefor countable,ProvableCountTreefor blast_syncableCONTRACT_VERSIONS_V4withtry_from_schema: 2, used in v12Schema example
{ "person": { "type": "object", "documentsCountable": true, "properties": { ... } } }Or with BLAST sync:
{ "person": { "type": "object", "blastSyncable": true, "properties": { ... } } }NOT in this PR
Count query extensions — follow-up to PR #3435.
Test plan
cargo check -p dpp -p drivepassescargo fmt --allclean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Platform
Tests