From 8a4f1526ada57848856f73175ad3cb5802ea6024 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Wed, 12 Aug 2026 23:58:47 +0700 Subject: [PATCH 1/3] test(drive): pin the batch-transition cap and land the phantom-group evidence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `max_transitions_in_documents_batch` is 1 at every protocol version and has been since the first mainnet release. Nothing said why that matters. It is load-bearing for state correctness, not throughput. `BatchTransitionAction::into_high_level_drive_operations` flattens every transition of a batch into one `Vec` and `apply_drive_operations` turns that into a single GroveDB batch, where the ordinary document Add/Update/Delete conversions are blind to each other. Two operations that jointly empty an index group each observe the other's document still committed, neither removes the group tree, and on a ranked index the leftover tree keeps ranking at zero with nothing behind it — internally consistent, so `verify_grovedb` passes and the proof reconstructs the live root hash. A wrong ranking that verifies. The defect is not fixed here. Fixing it at the correct site changes emitted operation counts, hence fees, hence the app hash, and would break `update_contract_keywords_operations`, which is correct precisely because its deletes are blind. That needs a protocol-version gate and its own change. What lands instead: - The rationale, on `SystemLimits::max_transitions_in_documents_batch`, with pointers from all three `SYSTEM_LIMITS_V*` constants and the one hand-written mock literal. It also names the two other guards holding the same line: the keyword path's caller guard, and the sibling-aware `MultipleDocumentOperationsForSameContractDocumentType` variant, which is not a drop-in for batch transitions because it carries no delete variant. - Tests pinning the cap at 1 across `PLATFORM_VERSIONS` and, under `mock-versions`, across the mock registry. - `rs-drive-abci` coverage proving the cap is enforced end to end: a real signed two-transition batch is refused before any drive operation is built, and the reachable near-miss — two transitions draining the same group in one block — produces the correct index. - `rs-drive` coverage of the mechanism. Five cases that expose the defect land `#[ignore]`d rather than deleted or weakened, so raising the cap has something to un-ignore; they fail today for the right reason. The rest run: a characterization test asserting the phantom exactly as it is on all three axes, the fail-loud inverse shapes, and the sequential controls. - The keyword path, previously argued safe only by code reading, is now executed. Replacing a whole keyword set keeps the `byContractId` group with exactly the new members; clearing it entirely strands an empty group tree, and the only thing preventing that from the network is the `!keywords.is_empty()` guard in `update_contract_v1`. That guard is a shield, not a fix: it also skips the deletes, so a contract that clears its keywords keeps advertising the old ones through keyword search. Both halves are now commented at the guard and pinned by characterization tests, because removing it to fix the stale index would silently arm the stranded group tree. No production behaviour changes: every non-test hunk is a comment. Co-Authored-By: Claude Opus 5 (1M context) --- .../batch/tests/document/mod.rs | 1 + .../tests/document/ranked_group_drain.rs | 618 ++++++++ .../v0/tests/batched_group_drain.rs | 1299 +++++++++++++++++ .../insert/insert_contract/v0/tests/mod.rs | 5 +- .../v0/tests/ranked_index_e2e_tests.rs | 10 + .../contract/update/update_contract/v1/mod.rs | 136 ++ .../contract/update/update_keywords/v0/mod.rs | 439 ++++++ .../src/version/mocks/v2_test.rs | 3 + .../src/version/system_limits/mod.rs | 99 ++ .../src/version/system_limits/v1.rs | 5 + .../src/version/system_limits/v2.rs | 2 + .../src/version/system_limits/v3.rs | 2 + 12 files changed, 2618 insertions(+), 1 deletion(-) create mode 100644 packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/ranked_group_drain.rs create mode 100644 packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/batched_group_drain.rs diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/mod.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/mod.rs index d073097768c..714a696d900 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/mod.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/mod.rs @@ -2,6 +2,7 @@ mod creation; mod deletion; mod dpns; mod nft; +mod ranked_group_drain; mod replacement; mod transfer; diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/ranked_group_drain.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/ranked_group_drain.rs new file mode 100644 index 00000000000..ac11d80cc67 --- /dev/null +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/ranked_group_drain.rs @@ -0,0 +1,618 @@ +//! What the network can actually do to a ranked index group, driven by real +//! signed batch transitions through `process_raw_state_transitions`. +//! +//! Drive's `batched_group_drain` suite shows that several document operations +//! sharing one GroveDB batch can jointly empty a ranked group and leave the +//! group tree behind — a document-less group that keeps ranking at zero and +//! still proves against the live root hash. Those cases are `#[ignore]`d +//! because the shape cannot be produced from the network, and this module is +//! why: a batch state transition may carry only one transition +//! (`max_transitions_in_documents_batch`), and two transitions in the same +//! block are applied as two separate GroveDB batches, because `execute_event` +//! calls `apply_drive_operations` once per state transition. +//! +//! Both halves of that argument are executed here rather than read off the +//! source: the cap is observed refusing a two-transition batch, and the +//! reachable near-miss — two identities draining the same group in one block — +//! is observed producing the correct index. Everything downstream of the +//! signed transition is real: advanced structure validation, the state +//! transformer, `into_high_level_drive_operations`, `apply_drive_operations`, +//! and grovedb's final batch application. + +use super::*; + +use crate::platform_types::platform_state::PlatformState; +use crate::platform_types::state_transitions_processing_result::StateTransitionExecutionResult::UnpaidConsensusError; +use crate::rpc::core::MockCoreRPCLike; +use crate::test::helpers::setup::TempPlatform; +use dpp::consensus::basic::BasicError; +use dpp::data_contract::accessors::v0::DataContractV0Setters; +use dpp::data_contract::document_type::DocumentTypeRef; +use dpp::document::Document; +use dpp::identifier::Identifier; +use dpp::identity::signer::Signer; +use dpp::identity::IdentityPublicKey; +use dpp::platform_value::BinaryData; +use dpp::prelude::DataContract; +use dpp::state_transition::batch_transition::batched_transition::BatchedTransition; +use dpp::state_transition::batch_transition::document_delete_transition::DocumentDeleteTransition; +use dpp::state_transition::batch_transition::BatchTransitionV1; +use dpp::state_transition::StateTransition; +use drive::drive::RootTree; +use drive::grovedb::Element; + +/// The group that gets emptied. +const G: &str = "beta"; +/// The bystander group, so the ranked index is never globally empty. +const H: &str = "alpha"; +/// The single index property every doctype in the fixture ranks by. +const GROUP_PROPERTY: &str = "restaurantId"; + +/// Shared with rs-drive's ranked suite rather than copied — this is the +/// fixture both layers of the argument are written against. +const RESTAURANTS_CONTRACT: &str = + "../rs-drive/tests/supporting_files/contract/restaurants/restaurants-contract.json"; + +/// The path of the terminal property-name tree: the indexed tree whose +/// children are the groups and whose secondary carries the ranking. +fn indexed_property_name_tree_path( + contract_id: Identifier, + document_type_name: &str, +) -> Vec> { + vec![ + vec![RootTree::DataContractDocuments as u8], + contract_id.as_bytes().to_vec(), + vec![1], + document_type_name.as_bytes().to_vec(), + GROUP_PROPERTY.as_bytes().to_vec(), + ] +} + +/// `SELECT COUNT(*) GROUP BY restaurantId ORDER BY $count DESC LIMIT 100` +/// straight off the secondary, with no query grammar in between. +fn ranked_count_groups( + platform: &TempPlatform, + path: &[Vec], + platform_version: &PlatformVersion, +) -> Vec<(u64, String)> { + let path_refs: Vec<&[u8]> = path.iter().map(|v| v.as_slice()).collect(); + platform + .drive + .grove + .indexed_count_top_k( + path_refs.as_slice(), + 100, + true, + None, + &platform_version.drive.grove_version, + ) + .unwrap() + .expect("the ranked count read must succeed") + .into_iter() + .map(|(count, key)| { + ( + count, + String::from_utf8(key).expect("fixture group keys are utf-8"), + ) + }) + .collect() +} + +/// The group's own value tree in the primary, or `None` if the group is gone. +fn primary_group_element( + platform: &TempPlatform, + path: &[Vec], + group: &str, + platform_version: &PlatformVersion, +) -> Option { + let path_refs: Vec<&[u8]> = path.iter().map(|v| v.as_slice()).collect(); + platform + .drive + .grove + .get_raw_optional( + path_refs.as_slice().into(), + group.as_bytes(), + None, + &platform_version.drive.grove_version, + ) + .unwrap() + .expect("the raw read must succeed") +} + +/// Register the restaurants fixture owned by `owner_id`. +fn register_restaurants( + platform: &TempPlatform, + owner_id: Identifier, + platform_version: &PlatformVersion, +) -> DataContract { + let mut contract = json_document_to_contract(RESTAURANTS_CONTRACT, true, platform_version) + .expect("expected to parse the restaurants contract"); + contract.set_owner_id(owner_id); + platform + .drive + .apply_contract( + &contract, + BlockInfo::default(), + true, + StorageFlags::optional_default_as_cow(), + None, + platform_version, + ) + .expect("expected to apply the restaurants contract"); + contract +} + +/// A real, signed batch transition carrying **one delete transition per +/// document**. With more than one document it is the multi-transition shape +/// `into_high_level_drive_operations` flattens. +/// +/// All transitions share one `identity_contract_nonce`: nonce validation reads +/// the *committed* nonce for every transition in the batch, so a batch that +/// incremented per transition would be rejected for the wrong reason. +async fn signed_delete_batch>( + documents: Vec, + document_type: DocumentTypeRef<'_>, + key: &IdentityPublicKey, + identity_contract_nonce: u64, + signer: &S, + platform_version: &PlatformVersion, +) -> Vec { + let owner_id = documents.first().expect("at least one document").owner_id(); + + let transitions = documents + .into_iter() + .map(|document| { + let delete: DocumentDeleteTransition = DocumentDeleteTransition::from_document( + document, + document_type, + None, + identity_contract_nonce, + platform_version, + None, + None, + ) + .expect("expected to build a delete transition"); + BatchedTransition::Document(delete.into()) + }) + .collect(); + + let batch: BatchTransition = BatchTransitionV1 { + owner_id, + transitions, + user_fee_increase: 0, + signature_public_key_id: 0, + signature: BinaryData::default(), + } + .into(); + + let mut state_transition: StateTransition = batch.into(); + let required_security_level = document_type.security_level_requirement(); + state_transition + .sign_external( + key, + signer, + Some(|_: Identifier, _: String| Ok(required_security_level)), + ) + .await + .expect("expected to sign the batch"); + + state_transition + .serialize_to_bytes() + .expect("expected to serialize the batch") +} + +/// Process `state_transitions` as one block against one grovedb transaction, +/// commit, and return the results so the caller can assert on them. +fn process_block( + platform: &TempPlatform, + platform_state: &PlatformState, + state_transitions: Vec>, + platform_version: &PlatformVersion, +) -> Vec { + let transaction = platform.drive.grove.start_transaction(); + let result = platform + .platform + .process_raw_state_transitions( + &state_transitions, + platform_state, + &BlockInfo::default(), + &transaction, + platform_version, + false, + None, + ) + .expect("expected to process the block"); + let results = result.into_execution_results(); + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("expected to commit the block"); + results +} + +/// Create one `visit` document through a real create transition in its own +/// block, and return it. +#[allow(clippy::too_many_arguments)] +async fn create_visit>( + platform: &TempPlatform, + platform_state: &PlatformState, + visit: DocumentTypeRef<'_>, + owner_id: Identifier, + key: &IdentityPublicKey, + signer: &S, + identity_contract_nonce: u64, + group: &str, + guests: u64, + rng: &mut StdRng, + platform_version: &PlatformVersion, +) -> Document { + let entropy = Bytes32::random_with_rng(rng); + let mut document = visit + .random_document_with_identifier_and_entropy( + rng, + owner_id, + entropy, + DocumentFieldFillType::FillIfNotRequired, + DocumentFieldFillSize::AnyDocumentFillSize, + platform_version, + ) + .expect("expected a random visit document"); + document.set(GROUP_PROPERTY, Value::Text(group.to_string())); + document.set("guests", Value::U64(guests)); + + let create = BatchTransition::new_document_creation_transition_from_document( + document.clone(), + visit, + entropy.0, + key, + identity_contract_nonce, + 0, + None, + signer, + platform_version, + None, + ) + .await + .expect("expected to build the create transition"); + + let results = process_block( + platform, + platform_state, + vec![create + .serialize_to_bytes() + .expect("expected to serialize the create transition")], + platform_version, + ); + assert_matches!( + results.as_slice(), + [StateTransitionExecutionResult::SuccessfulExecution { .. }], + "the create must be accepted" + ); + + document +} + +/// **The cap, observed.** +/// +/// `G` holds two `visit` documents and `H` holds one; a single signed batch +/// transition tries to delete both of `G`'s documents. It never reaches the +/// write path: basic structure validation caps a batch transition at +/// `max_transitions_in_documents_batch`, which is 1 at every protocol version, +/// so the batch is refused with `MaxDocumentsTransitionsExceededError` before +/// any drive operation is built. +/// +/// That refusal is the whole reason the phantom-group cases in rs-drive's +/// `batched_group_drain` suite are ignored rather than fixed. If this test +/// starts failing because the cap was raised, those cases become live. +#[tokio::test] +async fn a_multi_document_batch_transition_is_refused_by_the_one_transition_limit() { + let mut platform = TestPlatformBuilder::new() + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_initial_state_structure(); + + let (identity, signer, key) = setup_identity(&mut platform, 958, dash_to_credits!(1.0)); + + let platform_state = platform.state.load(); + let platform_version = platform_state + .current_platform_version() + .expect("expected the current platform version"); + + // Asserted before anything is built, because everything below only has + // meaning while this holds: once the cap is above 1 the batch is accepted, + // and an assertion placed after the refusal would simply never run. + assert_eq!( + platform_version + .system_limits + .max_transitions_in_documents_batch, + 1, + "raising this cap lets two document operations share one grovedb batch, where the \ + index-group emptiness check cannot see its siblings — leaving a document-less \ + group that still ranks and still proves. Un-ignore rs-drive's batched_group_drain \ + cases and make them pass before raising it" + ); + + let contract = register_restaurants(&platform, identity.id(), platform_version); + let visit = contract + .document_type_for_name("visit") + .expect("expected the visit doctype"); + + let mut rng = StdRng::seed_from_u64(4266); + + let mut documents = Vec::new(); + for (index, (group, guests)) in [(H, 2u64), (G, 4), (G, 6)].into_iter().enumerate() { + documents.push( + create_visit( + &platform, + &platform_state, + visit, + identity.id(), + &key, + &signer, + (index + 1) as u64, + group, + guests, + &mut rng, + platform_version, + ) + .await, + ); + } + + let path = indexed_property_name_tree_path(contract.id(), "visit"); + assert_eq!( + ranked_count_groups(&platform, &path, platform_version), + vec![(2, G.to_string()), (1, H.to_string())], + "baseline: G holds two visits, H holds one" + ); + + let drain = signed_delete_batch( + documents[1..3].to_vec(), + visit, + &key, + 4, + &signer, + platform_version, + ) + .await; + + let results = process_block(&platform, &platform_state, vec![drain], platform_version); + + // The error carries the limit it enforced, which pins that the check reads + // the version's declared cap rather than a constant of its own. The cap's + // *value* is pinned above, and by platform-version's own unit test. + assert_matches!( + results.as_slice(), + [UnpaidConsensusError(ConsensusError::BasicError( + BasicError::MaxDocumentsTransitionsExceededError(error) + ))] if error.max_transitions() + == platform_version.system_limits.max_transitions_in_documents_batch, + "a two-transition documents batch must be refused by the max-transitions limit, \ + reporting the limit the version declares. If this batch was accepted, the cap was \ + raised and the phantom-group defect in rs-drive's batched_group_drain suite is now \ + reachable from the network; got {results:?}" + ); + + assert_eq!( + ranked_count_groups(&platform, &path, platform_version), + vec![(2, G.to_string()), (1, H.to_string())], + "a rejected batch must not touch the ranked secondary" + ); +} + +/// **The reachable shape of the same scenario.** +/// +/// Since one batch may carry only one document transition, the closest a real +/// network gets to "several mutations jointly empty a group" is several state +/// transitions in the same block: two identities each deleting their own +/// document from `G`, both landing in one `process_raw_state_transitions` call +/// against one grovedb transaction. +/// +/// This is the case that decides whether the Drive-level defect matters in +/// production. `execute_event` calls `apply_drive_operations` once per state +/// transition, so each delete is its own grovedb batch and the second observes +/// the first through the transaction — but that is exactly the kind of +/// assumption worth executing rather than asserting. +#[tokio::test] +async fn two_state_transitions_in_one_block_drain_a_ranked_group_correctly() { + let mut platform = TestPlatformBuilder::new() + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_initial_state_structure(); + + let (owner, owner_signer, owner_key) = + setup_identity(&mut platform, 958, dash_to_credits!(1.0)); + let (other, other_signer, other_key) = + setup_identity(&mut platform, 450, dash_to_credits!(1.0)); + + let platform_state = platform.state.load(); + let platform_version = platform_state + .current_platform_version() + .expect("expected the current platform version"); + + let contract = register_restaurants(&platform, owner.id(), platform_version); + let visit = contract + .document_type_for_name("visit") + .expect("expected the visit doctype"); + + let mut rng = StdRng::seed_from_u64(4266); + + // H's single visit and G's first visit belong to `owner`; G's second + // belongs to `other`, so the two drains are independent transitions with + // independent nonces. + let mut owner_documents = Vec::new(); + for (index, (group, guests)) in [(H, 2u64), (G, 4)].into_iter().enumerate() { + owner_documents.push( + create_visit( + &platform, + &platform_state, + visit, + owner.id(), + &owner_key, + &owner_signer, + (index + 1) as u64, + group, + guests, + &mut rng, + platform_version, + ) + .await, + ); + } + let other_document = create_visit( + &platform, + &platform_state, + visit, + other.id(), + &other_key, + &other_signer, + 1, + G, + 6, + &mut rng, + platform_version, + ) + .await; + + let path = indexed_property_name_tree_path(contract.id(), "visit"); + assert_eq!( + ranked_count_groups(&platform, &path, platform_version), + vec![(2, G.to_string()), (1, H.to_string())], + "baseline: G holds two visits, H holds one" + ); + + // Both deletes in ONE block, one grovedb transaction, two transitions. + let first = signed_delete_batch( + vec![owner_documents[1].clone()], + visit, + &owner_key, + 3, + &owner_signer, + platform_version, + ) + .await; + let second = signed_delete_batch( + vec![other_document], + visit, + &other_key, + 2, + &other_signer, + platform_version, + ) + .await; + + let results = process_block( + &platform, + &platform_state, + vec![first, second], + platform_version, + ); + assert_matches!( + results.as_slice(), + [ + StateTransitionExecutionResult::SuccessfulExecution { .. }, + StateTransitionExecutionResult::SuccessfulExecution { .. } + ], + "both deletes must execute, got {results:?}" + ); + + let primary_g = primary_group_element(&platform, &path, G, platform_version); + assert_eq!( + primary_g, None, + "G's primary value tree must be gone after both its documents are \ + deleted in one block; got {primary_g:?}" + ); + assert_eq!( + ranked_count_groups(&platform, &path, platform_version), + vec![(1, H.to_string())], + "two deletes in one block must not leave a zero-valued phantom group" + ); + + let issues = platform + .drive + .grove + .verify_grovedb(None, true, false, &platform_version.drive.grove_version) + .expect("verify_grovedb must run"); + assert!( + issues.is_empty(), + "grovedb integrity verification reported issues: {issues:?}" + ); +} + +/// The control: the identical two deletes, one per batch transition, in +/// separate blocks. If this fails too, the behaviour under test is not +/// specific to how a block's transitions are batched. +#[tokio::test] +async fn deleting_the_same_two_documents_in_separate_blocks_removes_the_group() { + let mut platform = TestPlatformBuilder::new() + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_initial_state_structure(); + + let (identity, signer, key) = setup_identity(&mut platform, 958, dash_to_credits!(1.0)); + + let platform_state = platform.state.load(); + let platform_version = platform_state + .current_platform_version() + .expect("expected the current platform version"); + + let contract = register_restaurants(&platform, identity.id(), platform_version); + let visit = contract + .document_type_for_name("visit") + .expect("expected the visit doctype"); + + let mut rng = StdRng::seed_from_u64(4266); + + let mut documents = Vec::new(); + for (index, (group, guests)) in [(H, 2u64), (G, 4), (G, 6)].into_iter().enumerate() { + documents.push( + create_visit( + &platform, + &platform_state, + visit, + identity.id(), + &key, + &signer, + (index + 1) as u64, + group, + guests, + &mut rng, + platform_version, + ) + .await, + ); + } + + for (offset, document) in documents[1..3].iter().enumerate() { + let delete = signed_delete_batch( + vec![document.clone()], + visit, + &key, + (4 + offset) as u64, + &signer, + platform_version, + ) + .await; + + let results = process_block(&platform, &platform_state, vec![delete], platform_version); + assert_matches!( + results.as_slice(), + [StateTransitionExecutionResult::SuccessfulExecution { .. }], + "the single delete must be accepted, got {results:?}" + ); + } + + let path = indexed_property_name_tree_path(contract.id(), "visit"); + assert_eq!( + primary_group_element(&platform, &path, G, platform_version), + None, + "the control must drain G's primary value tree" + ); + assert_eq!( + ranked_count_groups(&platform, &path, platform_version), + vec![(1, H.to_string())], + "the control must remove G from the ranked secondary" + ); +} diff --git a/packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/batched_group_drain.rs b/packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/batched_group_drain.rs new file mode 100644 index 00000000000..f178e6bf7da --- /dev/null +++ b/packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/batched_group_drain.rs @@ -0,0 +1,1299 @@ +//! What several document operations in **one** GroveDB batch do to a ranked +//! index group they jointly empty. +//! +//! `BatchTransitionAction::into_high_level_drive_operations` flattens every +//! transition of a batch state transition into one `Vec`, and +//! `apply_drive_operations` converts each of those independently: the ordinary +//! Add / Update / Delete arms hand `batch_delete_up_tree_while_empty` no +//! sibling operations, so every conversion decides whether a group tree has +//! become empty from committed state alone. Two deletes that jointly empty a +//! group therefore each observe the other's document still committed, each +//! conclude the group is not empty, and the group tree survives with nothing +//! behind it. On a ranked index that leftover tree is mirrored into the +//! aggregate secondary, so the group keeps ranking at zero — sorting ahead of +//! every group with a positive aggregate — while primary and secondary agree +//! it exists, which means `verify_grovedb` is clean and the proof reconstructs +//! the live root hash. A wrong ranking that verifies. +//! +//! **That defect is real, and no batch state transition can reach it.** +//! `max_transitions_in_documents_batch` is 1 at every protocol version, so a +//! batch state transition carries at most one document transition, and two +//! transitions in the same block become two separate GroveDB batches +//! (`execute_event` calls `apply_drive_operations` once per state transition). +//! The cases that expose the defect are therefore `#[ignore]`d rather than +//! deleted or weakened: they are the specification of one hazard raising that +//! cap would have to fix — not of the whole shape, since a real multi-transition +//! batch would also fold several writes to the same identity-contract nonce key +//! into one batch. The cases that run pin what the system relies on today — the +//! phantom is characterized as it actually is, the inverse shape fails loud, +//! and the sequential controls drain correctly. +//! +//! The cap is not the only guard of its kind. `update_contract_keywords` builds +//! its own multi-document blind batch over one shared index group and is kept +//! correct by a guard in its caller rather than by this cap; see +//! `clearing_every_keyword_leaves_an_empty_by_contract_id_group_behind` in that +//! module for what that guard is worth. +//! +//! Every case drives its mutations through the same `apply_drive_operations` +//! call a batch transition produces, rather than through the one-op-per-call +//! `add/update/delete_document_for_contract` helpers the rest of this suite +//! uses. That difference is the whole point. Only the document operations are +//! reproduced, not the `UpdateIdentityContractNonce` operations a real +//! transition prepends: those touch the identity tree and have no bearing on +//! index maintenance. + +use super::*; + +use crate::error::drive::DriveError; +use crate::error::Error; +use crate::query::drive_document_ranked_query::index_picker::find_ranked_index_for_axis; +use crate::query::{ + DocumentRankedRequest, DocumentRankedResponse, DriveDocumentRankedQuery, OrderClause, + RankedAxis, RankedEntry, RankedEntryValue, RankedPage, SelectProjection, + RANKED_COUNT_ORDER_KEY, +}; +use crate::util::batch::{DocumentOperationType, DriveOperation}; +use crate::util::object_size_info::{DataContractInfo, DocumentTypeInfo}; +use dpp::data_contract::document_type::accessors::DocumentTypeV0Getters; +use dpp::prelude::Identifier; + +/// The group under test — the one that gets emptied. +const G: &str = "beta"; +/// The bystander group, so the index is never globally empty. +const H: &str = "alpha"; + +// --------------------------------------------------------------------------- +// One test body per ranking axis +// --------------------------------------------------------------------------- + +/// The fixture carries one doctype per axis (two ranked indexes on the same +/// property of one doctype is a `DuplicateIndexError`), so "run this scenario +/// on all three axes" means "run it on three doctypes". +#[derive(Clone, Copy, Debug)] +struct Axis { + doctype: &'static str, + /// The document property the index aggregates. + property: &'static str, + ranked: RankedAxis, +} + +const AXES: [Axis; 3] = [ + Axis { + doctype: "visit", + property: "guests", + ranked: RankedAxis::Count, + }, + Axis { + doctype: "tip", + property: "amount", + ranked: RankedAxis::Sum, + }, + Axis { + doctype: "review", + property: "grade", + ranked: RankedAxis::Avg, + }, +]; + +impl Axis { + fn select(&self) -> SelectProjection { + match self.ranked { + RankedAxis::Count => SelectProjection::count_star(), + RankedAxis::Sum => SelectProjection::sum(self.property), + RankedAxis::Avg => SelectProjection::avg(self.property), + } + } + + /// `ORDER BY` must name the selected aggregate: the `$count` sentinel for + /// `COUNT(*)`, the aggregated property otherwise. + fn order_field(&self) -> &'static str { + match self.ranked { + RankedAxis::Count => RANKED_COUNT_ORDER_KEY, + _ => self.property, + } + } + + /// What the index picker matches on — empty for `COUNT(*)`. + fn aggregate_field(&self) -> &'static str { + match self.ranked { + RankedAxis::Count => "", + _ => self.property, + } + } + + /// The aggregate a group of `values` must show on this axis. + fn expected_value(&self, values: &[i64]) -> RankedEntryValue { + let sum: i64 = values.iter().sum(); + match self.ranked { + RankedAxis::Count => RankedEntryValue::Count(values.len() as u64), + RankedAxis::Sum => RankedEntryValue::Sum(sum), + RankedAxis::Avg => { + RankedEntryValue::AvgFixedPoint(expected_avg_fixed_point(sum, values.len() as u64)) + } + } + } + + /// The aggregate a group with no documents left behind it shows. + /// `expected_value` cannot express this: an average over zero documents is + /// a division by zero, whereas the phantom reports a flat zero. + fn phantom_value(&self) -> RankedEntryValue { + match self.ranked { + RankedAxis::Count => RankedEntryValue::Count(0), + RankedAxis::Sum => RankedEntryValue::Sum(0), + RankedAxis::Avg => RankedEntryValue::AvgFixedPoint(0), + } + } + + /// The `(count, sum)` a phantom's surviving primary value tree carries. + /// Which halves are present is decided by the index's countability and + /// summability, so it differs per axis. + fn phantom_primary(&self) -> GroupAggregate { + match self.ranked { + RankedAxis::Count => Some((Some(0), None)), + RankedAxis::Sum => Some((None, Some(0))), + RankedAxis::Avg => Some((Some(0), Some(0))), + } + } + + /// Group keys straight out of grovedb's secondary, bypassing the query + /// layer entirely — so a query-layer filter cannot hide a phantom. + fn raw_group_keys(&self, drive: &Drive, path: &[Vec], descending: bool) -> Vec { + match self.ranked { + RankedAxis::Count => group_keys(&count_top_k(drive, path, 100, descending)), + RankedAxis::Sum => group_keys(&sum_top_k(drive, path, 100, descending)), + RankedAxis::Avg => group_keys(&avg_top_k(drive, path, 100, descending)), + } + } +} + +// --------------------------------------------------------------------------- +// Batch plumbing — the operations a batch transition flattens into +// --------------------------------------------------------------------------- + +fn delete_op<'a>( + contract: &'a DataContract, + doctype: &'a str, + document_id: Identifier, +) -> DriveOperation<'a> { + DriveOperation::DocumentOperation(DocumentOperationType::DeleteDocument { + document_id, + contract_info: DataContractInfo::BorrowedDataContract(contract), + document_type_info: DocumentTypeInfo::DocumentTypeNameAsStr(doctype), + }) +} + +fn add_op<'a>( + contract: &'a DataContract, + doctype: &'a str, + document: &'a Document, +) -> DriveOperation<'a> { + DriveOperation::DocumentOperation(DocumentOperationType::AddDocument { + owned_document_info: OwnedDocumentInfo { + document_info: DocumentRefInfo((document, None)), + owner_id: Some(document.owner_id().to_buffer()), + }, + contract_info: DataContractInfo::BorrowedDataContract(contract), + document_type_info: DocumentTypeInfo::DocumentTypeNameAsStr(doctype), + override_document: false, + }) +} + +fn update_op<'a>( + contract: &'a DataContract, + doctype: &'a str, + document: &'a Document, +) -> DriveOperation<'a> { + DriveOperation::DocumentOperation(DocumentOperationType::UpdateDocument { + owned_document_info: OwnedDocumentInfo { + document_info: DocumentRefInfo((document, None)), + owner_id: Some(document.owner_id().to_buffer()), + }, + contract_info: DataContractInfo::BorrowedDataContract(contract), + document_type_info: DocumentTypeInfo::DocumentTypeNameAsStr(doctype), + }) +} + +fn apply_batch(drive: &Drive, operations: Vec) { + drive + .apply_drive_operations( + operations, + true, + &BlockInfo::default(), + None, + platform_version(), + None, + ) + .expect("the batch must apply"); +} + +// --------------------------------------------------------------------------- +// Reading the result back — raw, unproved, and proved +// --------------------------------------------------------------------------- + +/// A group's primary aggregates: `(count, sum)`, each present only on the axes +/// that record it, and `None` altogether when the group has no value tree. +type GroupAggregate = Option<(Option, Option)>; + +/// The group's `(count, sum)` as recorded on its primary value tree, with the +/// Merk root key deliberately dropped. +/// +/// Tree *shape* is history-dependent, so the root key — and the app hash above +/// it — legitimately differ between a state reached in one batch and the same +/// state reached in several. The aggregates do not, which is why they are what +/// the batched-versus-sequential comparisons below use. +fn primary_group_aggregate(drive: &Drive, path: &[Vec], group: &str) -> GroupAggregate { + read_grove_element(drive, path, group.as_bytes()).map(|element| match element { + Element::ProvableCountProvableSumTree(_, count, sum, _) => (Some(count), Some(sum)), + Element::CountTree(_, count, _) => (Some(count), None), + Element::SumTree(_, sum, _) => (None, Some(sum)), + other => panic!("unexpected group element under a ranked primary: {other:?}"), + }) +} + +/// Everything about a ranked doctype that must be identical however the +/// mutations were batched: the full ranked entries in both directions (keys +/// *and* values), and each group's presence plus primary aggregates. +/// +/// Deliberately **not** the root hash — see [`primary_group_aggregate`]. +#[derive(Debug, PartialEq)] +struct LogicalState { + descending: Vec, + ascending: Vec, + primary: Vec<(String, GroupAggregate)>, +} + +fn logical_state(drive: &Drive, contract: &DataContract, axis: Axis) -> LogicalState { + let path = indexed_property_name_tree_path(contract, axis.doctype); + LogicalState { + descending: entries_of(run(drive, contract, axis, false, false)), + ascending: entries_of(run(drive, contract, axis, true, false)), + primary: [G, H] + .iter() + .map(|group| { + ( + group.to_string(), + primary_group_aggregate(drive, &path, group), + ) + }) + .collect(), + } +} + +fn grovedb_root_hash(drive: &Drive) -> [u8; 32] { + drive + .grove + .root_hash(None, &platform_version().drive.grove_version) + .unwrap() + .expect("root hash must be readable") +} + +/// `SELECT GROUP BY restaurantId ORDER BY [ASC|DESC] LIMIT 100 +/// OFFSET 0` through the public dispatcher — the same call drive-abci makes. +fn run( + drive: &Drive, + contract: &DataContract, + axis: Axis, + ascending: bool, + prove: bool, +) -> DocumentRankedResponse { + let group_by = vec![GROUP_PROPERTY.to_string()]; + let order_by = vec![OrderClause { + field: axis.order_field().to_string(), + ascending, + }]; + drive + .execute_document_ranked_request( + DocumentRankedRequest { + contract, + document_type: contract + .document_type_for_name(axis.doctype) + .expect("doctype exists"), + group_by: &group_by, + select: axis.select(), + having: &[], + order_by: &order_by, + where_clauses: &[], + limit: Some(100), + offset: Some(0), + has_start_at: false, + prove, + }, + None, + platform_version(), + ) + .expect("the ranked request must succeed") +} + +fn entries_of(response: DocumentRankedResponse) -> Vec { + match response { + DocumentRankedResponse::Entries(page) => page.entries, + DocumentRankedResponse::Proof(_) => panic!("expected entries, got a proof"), + } +} + +fn proof_of(response: DocumentRankedResponse) -> Vec { + match response { + DocumentRankedResponse::Proof(proof) => proof, + DocumentRankedResponse::Entries(_) => panic!("expected a proof, got entries"), + } +} + +fn entry_keys(entries: &[RankedEntry]) -> Vec { + entries + .iter() + .map(|entry| String::from_utf8(entry.key.clone()).expect("fixture group keys are utf-8")) + .collect() +} + +/// Prove the same page, verify it, and compare the **reconstructed root hash** +/// against the live one rather than merely asserting that verification +/// succeeded: a proof can verify against a root hash that is not the chain's. +fn verified_page( + drive: &Drive, + contract: &DataContract, + axis: Axis, + ascending: bool, +) -> RankedPage { + let proof = proof_of(run(drive, contract, axis, ascending, true)); + let indexes = contract + .document_types() + .get(axis.doctype) + .expect("doctype exists") + .indexes(); + let query = DriveDocumentRankedQuery { + document_type: contract + .document_type_for_name(axis.doctype) + .expect("doctype exists"), + contract_id: contract.id().to_buffer(), + document_type_name: axis.doctype.to_string(), + index: find_ranked_index_for_axis( + indexes, + GROUP_PROPERTY, + axis.ranked, + axis.aggregate_field(), + ) + .expect("the fixture declares this axis"), + axis: axis.ranked, + descending: !ascending, + k: 100, + offset: 0, + }; + let (root_hash, page) = query + .verify_ranked_top_k_proof(&proof, platform_version()) + .expect("the proof must verify"); + assert_eq!( + root_hash, + grovedb_root_hash(drive), + "the proof must reconstruct the live grovedb root hash" + ); + page +} + +/// The full observation battery every case ends with: both directions, +/// unproved and proved, plus the primary tree and grovedb's own integrity +/// sweep. +/// +/// `expected_descending` maps group key → the values still contributing to it, +/// in ranking order. A group that must be gone is simply absent. +fn assert_ranking_is( + drive: &Drive, + contract: &DataContract, + axis: Axis, + expected_descending: &[(&str, &[i64])], + context: &str, +) { + let path = indexed_property_name_tree_path(contract, axis.doctype); + + let descending_keys: Vec = expected_descending + .iter() + .map(|(key, _)| key.to_string()) + .collect(); + let ascending_keys: Vec = descending_keys.iter().rev().cloned().collect(); + + for (ascending, expected_keys) in [(false, &descending_keys), (true, &ascending_keys)] { + let direction = if ascending { "ASC" } else { "DESC" }; + + // Raw secondary, no query layer in the way. + assert_eq!( + &axis.raw_group_keys(drive, &path, !ascending), + expected_keys, + "{context}: raw {direction} secondary for the {:?} axis", + axis.ranked + ); + + // Unproved dispatcher read. + let entries = entries_of(run(drive, contract, axis, ascending, false)); + assert_eq!( + &entry_keys(&entries), + expected_keys, + "{context}: unproved {direction} ranking for the {:?} axis", + axis.ranked + ); + for (entry, (key, values)) in entries.iter().zip(if ascending { + expected_descending.iter().rev().collect::>() + } else { + expected_descending.iter().collect::>() + }) { + assert_eq!( + entry.value, + axis.expected_value(values), + "{context}: {direction} aggregate for group {key} on the {:?} axis", + axis.ranked + ); + } + + // Proved path, root hash compared. + let verified = verified_page(drive, contract, axis, ascending); + assert_eq!( + verified.entries, entries, + "{context}: the proved {direction} page must equal the unproved one \ + for the {:?} axis", + axis.ranked + ); + } + + // The primary side: a group with no documents must have no value tree. + for group in [G, H] { + let present = expected_descending.iter().any(|(key, _)| *key == group); + assert_eq!( + read_grove_element(drive, &path, group.as_bytes()).is_some(), + present, + "{context}: group {group}'s primary value tree presence on the {:?} axis \ + must match its presence in the ranking", + axis.ranked + ); + } + + assert_grovedb_is_consistent(drive); +} + +/// Insert `(group, value, seed)` rows with explicit seeds so two independently +/// built drives get byte-identical documents. +fn insert_seeded( + drive: &Drive, + contract: &DataContract, + axis: Axis, + rows: &[(&str, i64, u64)], +) -> Vec { + rows.iter() + .map(|(group, value, seed)| { + let doc = build_doc(contract, axis.doctype, axis.property, group, *value, *seed); + insert_doc(drive, contract, axis.doctype, &doc); + doc + }) + .collect() +} + +/// The fixture on a Drive configured the way a **shipped node** is. +/// +/// `setup_drive_with_initial_state_structure` forces +/// `batching_consistency_verification: true`, but the shipped default is +/// `false` (`DEFAULT_GROVE_BATCHING_CONSISTENCY_VERIFICATION_ENABLED`), and +/// with it off Drive additionally hands grovedb +/// `disable_operation_consistency_check: true`. The two configurations reject +/// a malformed batch at different places, so a batch the test configuration +/// refuses has to be re-run here before that refusal can be called the +/// system's real behaviour. +fn setup_restaurants_with_shipped_batching_config() -> (Drive, DataContract) { + use crate::config::DriveConfig; + use crate::util::test_helpers::setup::setup_drive; + + let pv = platform_version(); + let drive = setup_drive(Some(DriveConfig::default())); + assert!( + !drive.config.batching_consistency_verification, + "this fixture exists to exercise the shipped default" + ); + drive + .create_initial_state_structure(None, pv) + .expect("should create root tree successfully"); + let contract = dpp::tests::json_document::json_document_to_contract( + "tests/supporting_files/contract/restaurants/restaurants-contract.json", + false, + pv, + ) + .expect("expected to parse the restaurants contract"); + drive + .apply_contract( + &contract, + BlockInfo::default(), + true, + StorageFlags::optional_default_as_cow(), + None, + pv, + ) + .expect("expected to apply the restaurants contract"); + (drive, contract) +} + +/// `H` holds one document worth 10; `G` holds two, worth 20 and 30. +/// Returned in that order. +fn setup_g_two_h_one(axis: Axis) -> (Drive, DataContract, Vec) { + let (drive, contract) = setup_restaurants(); + let docs = insert_seeded( + &drive, + &contract, + axis, + &[(H, 10, 1), (G, 20, 2), (G, 30, 3)], + ); + (drive, contract, docs) +} + +/// The population every drain case starts from, asserted before the mutation +/// so a failure afterwards cannot be blamed on a bad fixture. +fn assert_baseline(drive: &Drive, contract: &DataContract, axis: Axis) { + // On every axis G(20,30) outranks H(10): count 2>1, sum 50>10, avg 25>10. + assert_ranking_is( + drive, + contract, + axis, + &[(G, &[20, 30]), (H, &[10])], + "baseline", + ); +} + +// --------------------------------------------------------------------------- +// Two last deletes in one batch — the core defect +// --------------------------------------------------------------------------- + +/// **This test asserts the defect, not the desired behaviour.** It is green, +/// and it is the evidence behind the severity claim documented on +/// `SystemLimits::max_transitions_in_documents_batch`: that the group a +/// batched drain leaves behind is not a detectable inconsistency but a +/// perfectly self-consistent lie. Every claim in that documentation is checked +/// here on every axis — the group is present with a zero aggregate, it sorts +/// *first* on ascending order, both its documents really are gone from primary +/// storage, grovedb's integrity sweep reports nothing, and the proved page +/// both equals the unproved one and reconstructs the live root hash. +/// +/// It goes red the day the defect is fixed, or the day grovedb starts +/// detecting it. That is the intended signal, not a regression: delete this +/// test then, and un-ignore the cases below. +#[test] +fn a_batched_drain_leaves_a_phantom_group_that_verifies_and_proves() { + for axis in AXES { + let (drive, contract, docs) = setup_g_two_h_one(axis); + assert_baseline(&drive, &contract, axis); + + apply_batch( + &drive, + vec![ + delete_op(&contract, axis.doctype, docs[1].id()), + delete_op(&contract, axis.doctype, docs[2].id()), + ], + ); + + let path = indexed_property_name_tree_path(&contract, axis.doctype); + let descending = entries_of(run(&drive, &contract, axis, false, false)); + let ascending = entries_of(run(&drive, &contract, axis, true, false)); + + assert_eq!( + entry_keys(&descending), + vec![H.to_string(), G.to_string()], + "{:?}: the drained group must still be ranked — that is the defect", + axis.ranked + ); + assert_eq!( + entry_keys(&ascending), + vec![G.to_string(), H.to_string()], + "{:?}: and it must sort ahead of a group with a positive aggregate ascending, \ + so a BOTTOM(k) query returns it first", + axis.ranked + ); + assert_eq!( + descending.last().expect("the phantom is ranked").value, + axis.phantom_value(), + "{:?}: the phantom's aggregate", + axis.ranked + ); + assert_eq!( + primary_group_aggregate(&drive, &path, G), + axis.phantom_primary(), + "{:?}: the primary value tree survives the drain, empty", + axis.ranked + ); + + for document in &docs[1..3] { + assert!( + !document_is_stored(&drive, &contract, axis, document.id()), + "{:?}: the documents really are deleted — the group ranks with nothing \ + behind it", + axis.ranked + ); + } + + let issues = drive + .grove + .verify_grovedb(None, true, false, &platform_version().drive.grove_version) + .expect("verify_grovedb must run"); + assert!( + issues.is_empty(), + "{:?}: integrity verification is structurally incapable of catching this — \ + primary and secondary agree that an empty group exists; got {issues:?}", + axis.ranked + ); + + for (direction_ascending, expected) in [(false, &descending), (true, &ascending)] { + // `verified_page` compares the reconstructed root hash against the + // live one, so this is the claim that the phantom proves against + // the real chain state rather than merely verifying. + assert_eq!( + &verified_page(&drive, &contract, axis, direction_ascending).entries, + expected, + "{:?}: the proof attests the phantom", + axis.ranked + ); + } + } +} + +/// **Known latent defect.** Both of `G`'s documents are deleted in one +/// `apply_drive_operations` call. Neither delete can see the other, so neither +/// removes `G`'s group tree: `G` stays in the ranked secondary with a zero +/// aggregate and no documents behind it, on all three axes, with +/// `verify_grovedb` clean and the proof reconstructing the live root hash. +/// +/// Unreachable today because `max_transitions_in_documents_batch` is 1, which +/// keeps any two document operations out of a shared GroveDB batch. Raising +/// that cap arms this; see the documentation on +/// `SystemLimits::max_transitions_in_documents_batch`. +#[test] +#[ignore = "documents a latent defect in shared write-path machinery that is unreachable while max_transitions_in_documents_batch is 1; un-ignore before raising that cap"] +fn deleting_a_groups_last_two_documents_in_one_batch_removes_the_group() { + for axis in AXES { + let (drive, contract, docs) = setup_g_two_h_one(axis); + assert_baseline(&drive, &contract, axis); + + // The whole experiment: both deletes in ONE apply_drive_operations call. + apply_batch( + &drive, + vec![ + delete_op(&contract, axis.doctype, docs[1].id()), + delete_op(&contract, axis.doctype, docs[2].id()), + ], + ); + + assert_ranking_is( + &drive, + &contract, + axis, + &[(H, &[10])], + "after batching G's last two deletes", + ); + } +} + +/// **Known latent defect**, same as +/// [`deleting_a_groups_last_two_documents_in_one_batch_removes_the_group`], +/// with the two deletes in the opposite order — the group's emptiness is +/// mis-observed either way, so the defect is not an artefact of one ordering. +/// +/// Unreachable while `max_transitions_in_documents_batch` is 1. +#[test] +#[ignore = "documents a latent defect in shared write-path machinery that is unreachable while max_transitions_in_documents_batch is 1; un-ignore before raising that cap"] +fn reverse_ordered_batched_deletes_remove_the_group_too() { + for axis in AXES { + let (drive, contract, docs) = setup_g_two_h_one(axis); + assert_baseline(&drive, &contract, axis); + + apply_batch( + &drive, + vec![ + delete_op(&contract, axis.doctype, docs[2].id()), + delete_op(&contract, axis.doctype, docs[1].id()), + ], + ); + + assert_ranking_is( + &drive, + &contract, + axis, + &[(H, &[10])], + "after batching G's last two deletes in reverse order", + ); + } +} + +/// **Known latent defect**, same as +/// [`deleting_a_groups_last_two_documents_in_one_batch_removes_the_group`], on +/// a Drive configured the way a shipped node is — so it cannot be dismissed as +/// an artefact of the test harness's `batching_consistency_verification: true`. +/// +/// Unreachable while `max_transitions_in_documents_batch` is 1. +#[test] +#[ignore = "documents a latent defect in shared write-path machinery that is unreachable while max_transitions_in_documents_batch is 1; un-ignore before raising that cap"] +fn batched_deletes_remove_the_group_under_the_shipped_batching_config() { + for axis in AXES { + let (drive, contract) = setup_restaurants_with_shipped_batching_config(); + let docs = insert_seeded( + &drive, + &contract, + axis, + &[(H, 10, 1), (G, 20, 2), (G, 30, 3)], + ); + + apply_batch( + &drive, + vec![ + delete_op(&contract, axis.doctype, docs[1].id()), + delete_op(&contract, axis.doctype, docs[2].id()), + ], + ); + + assert_ranking_is( + &drive, + &contract, + axis, + &[(H, &[10])], + "after batching G's last two deletes on a shipped-config Drive", + ); + } +} + +/// **Known latent defect.** The batched drain and the identical drain applied +/// one delete at a time must land the same logical index; they do not. +/// +/// The comparison is over entries and primary aggregates rather than the root +/// hash: a secondary Merk's shape depends on the order and grouping of the +/// writes that built it, so one batch and two batches reaching the same +/// logical state can legitimately hash differently. That is still +/// deterministic for nodes replaying identical history, so it is not itself a +/// consensus hazard — but it makes the app hash the wrong instrument here. +/// +/// Unreachable while `max_transitions_in_documents_batch` is 1. +#[test] +#[ignore = "documents a latent defect in shared write-path machinery that is unreachable while max_transitions_in_documents_batch is 1; un-ignore before raising that cap"] +fn batched_and_sequential_drains_agree_on_the_logical_state() { + for axis in AXES { + let (batched, batched_contract, batched_docs) = setup_g_two_h_one(axis); + let (sequential, sequential_contract, sequential_docs) = setup_g_two_h_one(axis); + assert_eq!( + logical_state(&batched, &batched_contract, axis), + logical_state(&sequential, &sequential_contract, axis), + "{:?}: the two fixtures must start from the same logical state", + axis.ranked + ); + + apply_batch( + &batched, + vec![ + delete_op(&batched_contract, axis.doctype, batched_docs[1].id()), + delete_op(&batched_contract, axis.doctype, batched_docs[2].id()), + ], + ); + for doc in &sequential_docs[1..3] { + apply_batch( + &sequential, + vec![delete_op(&sequential_contract, axis.doctype, doc.id())], + ); + } + + assert_eq!( + logical_state(&batched, &batched_contract, axis), + logical_state(&sequential, &sequential_contract, axis), + "{:?}: draining G in one batch must land the same logical state as draining \ + it one delete at a time", + axis.ranked + ); + } +} + +// --------------------------------------------------------------------------- +// Multi-move drain in one batch — the same defect through UpdateDocument +// --------------------------------------------------------------------------- + +/// Move `documents` into group `H` by rewriting their index property, and +/// return them so they can be handed to update operations. +fn moved_to_h(documents: &[Document]) -> Vec { + documents + .iter() + .map(|doc| { + let mut moved = doc.clone(); + let mut props = moved.properties().clone(); + props.insert(GROUP_PROPERTY.to_string(), Value::Text(H.to_string())); + moved.set_properties(props); + moved.set_revision(Some(2)); + moved + }) + .collect() +} + +fn update_document_singly(drive: &Drive, contract: &DataContract, axis: Axis, doc: &Document) { + drive + .update_document_for_contract( + doc, + contract, + contract + .document_type_for_name(axis.doctype) + .expect("doctype exists"), + Some(doc.owner_id().to_buffer()), + BlockInfo::default(), + true, + StorageFlags::optional_default_as_cow(), + None, + platform_version(), + None, + ) + .expect("the single-document update must succeed"); +} + +/// **Known latent defect**, reached through `UpdateDocument` rather than +/// `DeleteDocument`: one batch moves both of `G`'s documents into `H`. `H` +/// correctly gains all three contributions, but `G` survives as a zero-valued +/// phantom, so the defect is not specific to deletes — any operation that +/// empties a group while a sibling operation in the same batch has not been +/// observed reproduces it. +/// +/// Unreachable while `max_transitions_in_documents_batch` is 1. +#[test] +#[ignore = "documents a latent defect in shared write-path machinery that is unreachable while max_transitions_in_documents_batch is 1; un-ignore before raising that cap"] +fn moving_a_groups_last_two_documents_to_another_group_in_one_batch_drains_it() { + for axis in AXES { + let (drive, contract, docs) = setup_g_two_h_one(axis); + assert_baseline(&drive, &contract, axis); + + // A second drive that will receive the identical moves one at a time. + let (sequential_drive, sequential_contract, sequential_docs) = setup_g_two_h_one(axis); + assert_eq!( + logical_state(&drive, &contract, axis), + logical_state(&sequential_drive, &sequential_contract, axis), + "{:?}: the two fixtures must start from the same logical state, otherwise \ + the comparison below proves nothing", + axis.ranked + ); + + let moved = moved_to_h(&docs[1..3]); + apply_batch( + &drive, + moved + .iter() + .map(|doc| update_op(&contract, axis.doctype, doc)) + .collect(), + ); + + assert_ranking_is( + &drive, + &contract, + axis, + &[(H, &[10, 20, 30])], + "after batching both of G's documents into H", + ); + + for doc in moved_to_h(&sequential_docs[1..3]) { + update_document_singly(&sequential_drive, &sequential_contract, axis, &doc); + } + assert_ranking_is( + &sequential_drive, + &sequential_contract, + axis, + &[(H, &[10, 20, 30])], + "after moving both of G's documents into H sequentially", + ); + + assert_eq!( + logical_state(&drive, &contract, axis), + logical_state(&sequential_drive, &sequential_contract, axis), + "{:?}: batching the two moves must land the same logical state as applying \ + them one at a time", + axis.ranked + ); + } +} + +// --------------------------------------------------------------------------- +// Emptying and refilling a group in one batch — the inverse shape +// --------------------------------------------------------------------------- + +/// Everything the delete-and-refill cases need to tell a correct outcome from +/// a silently wrong one, observed *after* the batch either applied or was +/// refused. +#[derive(Debug, PartialEq)] +struct Observation { + /// `G`'s entry in the descending ranking, if it has one. + g_in_ranking: Option, + /// `G`'s primary value tree aggregates, if the tree survives. + g_primary_present: bool, + /// Is the arriving document in primary document storage? + arrival_stored: bool, + /// Is the departing document still in primary document storage? + departure_stored: bool, + /// grovedb's own integrity sweep. + grovedb_issues: usize, +} + +/// Primary document storage for a doctype: the `0` child of the doctype tree, +/// alongside the per-index property-name trees. +fn document_storage_path(contract: &DataContract, document_type_name: &str) -> Vec> { + vec![ + vec![crate::drive::RootTree::DataContractDocuments as u8], + contract.id().as_bytes().to_vec(), + vec![1], + document_type_name.as_bytes().to_vec(), + vec![0], + ] +} + +fn document_is_stored(drive: &Drive, contract: &DataContract, axis: Axis, id: Identifier) -> bool { + read_grove_element( + drive, + &document_storage_path(contract, axis.doctype), + id.as_bytes(), + ) + .is_some() +} + +/// Read back everything the refill cases assert on. +fn observe( + drive: &Drive, + contract: &DataContract, + axis: Axis, + arrival: Identifier, + departure: Identifier, +) -> Observation { + let path = indexed_property_name_tree_path(contract, axis.doctype); + let ranking = entries_of(run(drive, contract, axis, false, false)); + Observation { + g_in_ranking: ranking + .iter() + .find(|entry| entry.key == G.as_bytes()) + .map(|entry| entry.value), + g_primary_present: primary_group_aggregate(drive, &path, G).is_some(), + arrival_stored: document_is_stored(drive, contract, axis, arrival), + departure_stored: document_is_stored(drive, contract, axis, departure), + grovedb_issues: drive + .grove + .verify_grovedb(None, true, false, &platform_version().drive.grove_version) + .expect("verify_grovedb must run") + .len(), + } +} + +/// Apply `operations` without panicking on a refusal — a refusal is a result +/// here, not a harness failure — and observe the state either way. +fn apply_and_observe( + drive: &Drive, + contract: &DataContract, + axis: Axis, + operations: Vec, + arrival: Identifier, + departure: Identifier, +) -> (Result<(), Error>, Observation) { + let applied = drive + .apply_drive_operations( + operations, + true, + &BlockInfo::default(), + None, + platform_version(), + None, + ) + .map(|_| ()); + let observation = observe(drive, contract, axis, arrival, departure); + (applied, observation) +} + +/// The state a refused batch must leave behind: exactly the state before it, +/// with `G` still holding only its original document and the arrival nowhere. +/// +/// `departure_stored` differs between the two cases — the delete case removes +/// the departing document if it applies, the update case keeps it — but on a +/// *refusal* both must still have it. +fn untouched(axis: Axis) -> Observation { + Observation { + g_in_ranking: Some(axis.expected_value(&[20])), + g_primary_present: true, + arrival_stored: false, + departure_stored: true, + grovedb_issues: 0, + } +} + +/// Assert a refusal is the loud, shipped-node kind. +/// +/// With `batching_consistency_verification` on, Drive's own pre-flight check +/// on the assembled batch catches the collision. With it off — the shipped +/// default — Drive skips that check *and* tells grovedb to skip its +/// operation-consistency check, so the batch reaches the applier and grovedb's +/// tree-building pass is what refuses it. Only the second is what a real node +/// relies on, so the two are asserted apart rather than lumped together as +/// "some error". +fn assert_refusal_is_loud(error: &Error, consistency_verification: bool, label: &str) { + if consistency_verification { + assert!( + matches!(error, Error::Drive(DriveError::GroveDBInsertion(_))), + "{label}: with Drive's pre-flight batch check on, the refusal must come from \ + that check; got {error:?}" + ); + } else { + assert!( + matches!( + error, + Error::GroveDB(inner) if matches!(**inner, grovedb::Error::InvalidBatchOperation(_)) + ), + "{label}: on the shipped batching configuration the refusal must come from \ + grovedb's batch applier — that is the only guard a real node has here; \ + got {error:?}" + ); + } +} + +/// The fixture for both refill cases, on whichever batching configuration is +/// under test. +fn setup_g_one_h_one( + axis: Axis, + consistency_verification: bool, +) -> (Drive, DataContract, Document) { + let (drive, contract) = if consistency_verification { + setup_restaurants() + } else { + setup_restaurants_with_shipped_batching_config() + }; + let mut docs = insert_seeded(&drive, &contract, axis, &[(H, 10, 1), (G, 20, 2)]); + let g_document = docs.remove(1); + (drive, contract, g_document) +} + +/// `G` holds exactly one document; one batch removes it and puts a *different* +/// document in the same group. +/// +/// This is the inverse of the phantom: the mutation that empties `G` schedules +/// its group tree for deletion while the mutation that refills it sees the +/// group still present pre-batch and emits no tree insert, so the arriving +/// document could end up in primary storage with no index entry pointing at +/// it. `verify_grovedb` cannot see that, which is why the assertions check +/// reachability through the ranked index *and* presence in primary document +/// storage. +/// +/// The only two acceptable outcomes are a loud refusal and a fully correct +/// apply. Both operation orders and both batching configurations are swept, +/// because Drive's own pre-flight consistency check and grovedb's applier +/// reject a malformed batch at different places and only the second is what a +/// shipped node relies on. +#[test] +fn deleting_a_groups_only_document_and_creating_another_in_one_batch_never_lands_silently_wrong() { + let mut report = String::new(); + let mut bad = Vec::new(); + + for axis in AXES { + for delete_first in [true, false] { + for consistency_verification in [true, false] { + let (drive, contract, departing) = + setup_g_one_h_one(axis, consistency_verification); + let arrival = build_doc(&contract, axis.doctype, axis.property, G, 40, 99); + assert_ne!( + arrival.id(), + departing.id(), + "the replacement must be a genuinely different document" + ); + + let delete = delete_op(&contract, axis.doctype, departing.id()); + let add = add_op(&contract, axis.doctype, &arrival); + let operations = if delete_first { + vec![delete, add] + } else { + vec![add, delete] + }; + + let label = format!( + "{:?} axis / {} / consistency_verification={consistency_verification}", + axis.ranked, + if delete_first { + "delete-then-create" + } else { + "create-then-delete" + }, + ); + + let (applied, observed) = apply_and_observe( + &drive, + &contract, + axis, + operations, + arrival.id(), + departing.id(), + ); + report.push_str(&format!(" {label}\n → {applied:?} {observed:?}\n")); + + match &applied { + Err(error) => { + assert_refusal_is_loud(error, consistency_verification, &label); + assert_eq!( + observed, + untouched(axis), + "{label}: a refused batch must leave the state exactly as it was" + ); + } + Ok(()) => { + // G keeps exactly the arriving document, and the + // deleted one is gone from primary storage. + let correct = Observation { + g_in_ranking: Some(axis.expected_value(&[40])), + g_primary_present: true, + arrival_stored: true, + departure_stored: false, + grovedb_issues: 0, + }; + if observed != correct { + bad.push(label); + } + } + } + } + } + } + + assert!( + bad.is_empty(), + "these combinations applied but landed a wrong index: {bad:?}\n{report}" + ); +} + +/// The mirror image: one batch moves `dA` *out* of `G` with an update while +/// creating `dB` *into* `G`. Same blind spot — the update's delete walker can +/// schedule `G`'s tree for removal while the create sees `G` still present and +/// emits nothing — but reached through the update path rather than the delete +/// path, and with `G`'s membership never actually dropping to zero. +/// +/// Same acceptance rule as +/// [`deleting_a_groups_only_document_and_creating_another_in_one_batch_never_lands_silently_wrong`], +/// except that the departing document is updated rather than deleted, so it +/// must still be in primary storage afterwards. +#[test] +fn moving_one_document_out_of_a_group_while_creating_another_into_it_never_lands_silently_wrong() { + let mut report = String::new(); + let mut bad = Vec::new(); + + for axis in AXES { + for update_first in [true, false] { + for consistency_verification in [true, false] { + let (drive, contract, departing) = + setup_g_one_h_one(axis, consistency_verification); + + let moved = moved_to_h(&[departing]).remove(0); + let arrival = build_doc(&contract, axis.doctype, axis.property, G, 40, 99); + + let update = update_op(&contract, axis.doctype, &moved); + let add = add_op(&contract, axis.doctype, &arrival); + let operations = if update_first { + vec![update, add] + } else { + vec![add, update] + }; + + let label = format!( + "{:?} axis / {} / consistency_verification={consistency_verification}", + axis.ranked, + if update_first { + "update-out-then-create-in" + } else { + "create-in-then-update-out" + }, + ); + + let (applied, observed) = apply_and_observe( + &drive, + &contract, + axis, + operations, + arrival.id(), + moved.id(), + ); + report.push_str(&format!(" {label}\n → {applied:?} {observed:?}\n")); + + match &applied { + Err(error) => { + assert_refusal_is_loud(error, consistency_verification, &label); + assert_eq!( + observed, + untouched(axis), + "{label}: a refused batch must leave the state exactly as it was" + ); + } + Ok(()) => { + // G keeps exactly the arriving document; the moved one + // stays in primary storage (it was updated, not deleted). + let correct = Observation { + g_in_ranking: Some(axis.expected_value(&[40])), + g_primary_present: true, + arrival_stored: true, + departure_stored: true, + grovedb_issues: 0, + }; + if observed != correct { + bad.push(label); + } + } + } + } + } + } + + assert!( + bad.is_empty(), + "these combinations applied but landed a wrong index: {bad:?}\n{report}" + ); +} + +// --------------------------------------------------------------------------- +// The control: the same mutations, separate batches +// --------------------------------------------------------------------------- + +/// The control for the move path: the same two moves applied one at a time +/// drain `G` cleanly. Without it, a failure of the ignored batched-move case +/// would be ambiguous between "batching is the problem" and "moves are the +/// problem". +#[test] +fn moving_the_same_two_documents_to_another_group_one_at_a_time_drains_it() { + for axis in AXES { + let (drive, contract, docs) = setup_g_two_h_one(axis); + assert_baseline(&drive, &contract, axis); + + for doc in moved_to_h(&docs[1..3]) { + update_document_singly(&drive, &contract, axis, &doc); + } + + assert_ranking_is( + &drive, + &contract, + axis, + &[(H, &[10, 20, 30])], + "after moving both of G's documents into H one at a time", + ); + } +} + +/// The control that makes the ignored cases above mean something: the same two +/// deletes in two separate `apply_drive_operations` calls drain `G` cleanly +/// from both the primary and the secondary. If this ever fails, the defect is +/// not batch-specific and is much larger than the one documented here. +#[test] +fn deleting_the_same_two_documents_in_separate_batches_removes_the_group() { + for axis in AXES { + let (drive, contract, docs) = setup_g_two_h_one(axis); + assert_baseline(&drive, &contract, axis); + + apply_batch( + &drive, + vec![delete_op(&contract, axis.doctype, docs[1].id())], + ); + assert_ranking_is( + &drive, + &contract, + axis, + &[(G, &[30]), (H, &[10])], + "after the first of two separate deletes", + ); + + apply_batch( + &drive, + vec![delete_op(&contract, axis.doctype, docs[2].id())], + ); + assert_ranking_is( + &drive, + &contract, + axis, + &[(H, &[10])], + "after the second of two separate deletes", + ); + } +} diff --git a/packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/mod.rs b/packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/mod.rs index 723c77e937b..e0349634b83 100644 --- a/packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/mod.rs +++ b/packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/mod.rs @@ -21,7 +21,10 @@ //! - [`ranked_index_e2e_tests`] — per-index `rankedCountable` / //! `rankedSummable` / `rankedAverageable` (meta schema v3 / PV14): //! the indexed-tree variants and their ordered secondaries, end to -//! end through insert / update / delete. +//! end through insert / update / delete. Its child module +//! `batched_group_drain` lives in `batched_group_drain.rs` beside +//! this file and is declared with `#[path]` so it can reuse that +//! suite's fixture and assertion helpers. mod countable_e2e_tests; mod range_countable_index_e2e_tests; diff --git a/packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/ranked_index_e2e_tests.rs b/packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/ranked_index_e2e_tests.rs index b043fd6fe4e..36ad01f54e9 100644 --- a/packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/ranked_index_e2e_tests.rs +++ b/packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/ranked_index_e2e_tests.rs @@ -46,6 +46,16 @@ //! secondary entries `indexed_*_top_k` returns are keyed by those same group //! keys. A compound index `[a, b]` inserts ` / ` between the //! doctype and the terminal `` level. +//! +//! [`batched_group_drain`] extends this suite to the one shape it does not +//! otherwise reach: several document operations applied in a *single* grovedb +//! batch. It reuses the fixture and the assertion helpers below, which is why +//! it is a child module rather than a sibling. + +/// Declared with `#[path]` so it can sit beside the other test files while +/// still reaching this module's fixture and assertion helpers. +#[path = "batched_group_drain.rs"] +mod batched_group_drain; use crate::drive::Drive; use crate::util::grove_operations::DirectQueryType; diff --git a/packages/rs-drive/src/drive/contract/update/update_contract/v1/mod.rs b/packages/rs-drive/src/drive/contract/update/update_contract/v1/mod.rs index 129badb99b4..bb43c9e20ce 100644 --- a/packages/rs-drive/src/drive/contract/update/update_contract/v1/mod.rs +++ b/packages/rs-drive/src/drive/contract/update/update_contract/v1/mod.rs @@ -253,6 +253,22 @@ impl Drive { )?); } + // Skipping an empty keyword set is load-bearing, but it is a shield + // rather than a fix, and both halves matter to anyone changing it. + // + // What it prevents: the keyword update emits its deletes blind to each + // other in one batch, so several of them jointly emptying the shared + // `byContractId/` group would leave that group tree behind + // with nothing in it — and emptying the group without refilling it + // requires exactly this empty-set case. + // + // What it costs: the previous keyword documents are not deleted either, + // so a contract that clears its keywords advertises none while keyword + // search still returns it under the old ones. Removing this guard to fix + // that trades a stale index for a stranded group tree; the deletes have + // to become sibling-aware first. Both halves are pinned — + // `clearing_a_contracts_keywords_leaves_the_old_ones_indexed` and + // `clearing_every_keyword_leaves_an_empty_by_contract_id_group_behind`. if !contract.keywords().is_empty() { batch_operations.extend(self.update_contract_keywords_operations( contract.id(), @@ -452,6 +468,126 @@ mod tests { .expect("update keyword delta via update_contract should succeed"); } + /// The keywords the keyword search index currently returns for `contract_id`. + fn indexed_keywords( + drive: &crate::drive::Drive, + keyword_search: &dpp::prelude::DataContract, + contract_id: Identifier, + platform_version: &PlatformVersion, + ) -> Vec { + use crate::drive::document::query::QueryDocumentsOutcomeV0Methods; + use crate::query::{DriveDocumentQuery, WhereClause, WhereOperator}; + use dpp::document::DocumentV0Getters; + use dpp::platform_value::btreemap_extensions::BTreeValueMapHelper; + use dpp::platform_value::Value; + + let document_type = keyword_search + .document_type_for_name("contractKeywords") + .expect("contractKeywords doctype"); + let mut query = DriveDocumentQuery::all_items_query(keyword_search, document_type, None); + query.internal_clauses.equal_clauses.insert( + "contractId".to_string(), + WhereClause { + field: "contractId".to_string(), + operator: WhereOperator::Equal, + value: Value::Identifier(contract_id.to_buffer()), + }, + ); + let mut keywords: Vec = drive + .query_documents( + query, + None, + false, + None, + Some(platform_version.protocol_version), + ) + .expect("the byContractId query must succeed") + .documents_owned() + .into_iter() + .map(|document| { + document + .properties() + .get_string("keyword") + .expect("every keyword document carries a keyword") + }) + .collect(); + keywords.sort(); + keywords + } + + /// **This test asserts a defect, not the desired behaviour**, and it is the + /// other half of the empty-keyword-set skip above. + /// + /// Clearing a contract's keywords does not delete its keyword documents: an + /// empty set skips the keyword update entirely, so the previous documents + /// survive and stay indexed. The contract then advertises no keywords while + /// keyword search still returns it under the old ones, permanently. + /// + /// The skip is a shield, not a fix. It is what keeps the deletes from + /// jointly emptying the shared `byContractId` group and stranding it — see + /// `clearing_every_keyword_leaves_an_empty_by_contract_id_group_behind` — + /// so removing it to make this test go green trades a stale index for an + /// empty group tree. Making the deletes sibling-aware has to come first. + #[test] + fn clearing_a_contracts_keywords_leaves_the_old_ones_indexed() { + let drive = setup_drive_with_initial_state_structure(None); + let platform_version = PlatformVersion::latest(); + + let keyword_search = + load_system_data_contract(SystemDataContract::KeywordSearch, platform_version) + .expect("load keyword_search"); + drive + .apply_contract( + &keyword_search, + BlockInfo::default(), + true, + None, + None, + platform_version, + ) + .expect("apply keyword_search"); + + let mut contract = get_dashpay_contract_fixture(None, 0, platform_version.protocol_version) + .data_contract_owned(); + contract.set_keywords(vec!["alpha".to_string(), "bravo".to_string()]); + drive + .apply_contract( + &contract, + BlockInfo::default(), + true, + StorageFlags::optional_default_as_cow(), + None, + platform_version, + ) + .expect("initial insert with keywords"); + + assert_eq!( + indexed_keywords(&drive, &keyword_search, contract.id(), platform_version), + vec!["alpha".to_string(), "bravo".to_string()], + "baseline: both keywords are indexed" + ); + + contract.set_keywords(vec![]); + contract.increment_version(); + drive + .update_contract( + &contract, + BlockInfo::default(), + true, + None, + platform_version, + None, + ) + .expect("clearing keywords via update_contract should succeed"); + + assert_eq!( + indexed_keywords(&drive, &keyword_search, contract.id(), platform_version), + vec!["alpha".to_string(), "bravo".to_string()], + "the old keyword documents are expected to survive: an empty keyword set skips \ + the keyword update rather than performing it" + ); + } + /// Exercises `update_contract_operations_v1`'s description-update branch: /// changing contract description routes through /// `update_contract_description_operations`. Covers the `if let Some(description)` diff --git a/packages/rs-drive/src/drive/contract/update/update_keywords/v0/mod.rs b/packages/rs-drive/src/drive/contract/update/update_keywords/v0/mod.rs index 8e6ab6cdaec..15a847c7e8a 100644 --- a/packages/rs-drive/src/drive/contract/update/update_keywords/v0/mod.rs +++ b/packages/rs-drive/src/drive/contract/update/update_keywords/v0/mod.rs @@ -180,14 +180,27 @@ impl Drive { #[cfg(test)] mod tests { + use crate::drive::document::query::QueryDocumentsOutcomeV0Methods; + use crate::drive::Drive; + use crate::fees::op::LowLevelDriveOperation; + use crate::query::{DriveDocumentQuery, WhereClause, WhereOperator}; + use crate::util::grove_operations::DirectQueryType; use crate::util::storage_flags::StorageFlags; use crate::util::test_helpers::setup::setup_drive_with_initial_state_structure; use dpp::block::block_info::BlockInfo; use dpp::data_contract::accessors::v0::DataContractV0Getters; use dpp::data_contract::accessors::v1::DataContractV1Setters; + use dpp::document::DocumentV0Getters; + use dpp::identifier::Identifier; + use dpp::platform_value::btreemap_extensions::BTreeValueMapHelper; + use dpp::platform_value::Value; + use dpp::prelude::DataContract; use dpp::system_data_contracts::{load_system_data_contract, SystemDataContract}; use dpp::tests::fixtures::get_data_contract_fixture; use dpp::version::PlatformVersion; + use grovedb::batch::GroveOp; + use grovedb::query_result_type::QueryResultType; + use grovedb::{PathQuery, Query}; /// Exercises `update_contract_keywords_v0` in apply=false estimation mode /// with a fresh contract (no prior keywords). The inner @@ -360,4 +373,430 @@ mod tests { "partial keyword update should produce non-zero fee" ); } + + /// The `contractKeywords` doctype tree of the keyword search contract. + fn contract_keywords_path(keyword_search: &DataContract) -> Vec> { + vec![ + vec![crate::drive::RootTree::DataContractDocuments as u8], + keyword_search.id().as_bytes().to_vec(), + vec![1], + b"contractKeywords".to_vec(), + ] + } + + /// The subtree of `byContractId` references belonging to one contract: + /// the group tree the deletes and the adds of a keyword replacement share. + fn by_contract_id_reference_path( + keyword_search: &DataContract, + contract_id: Identifier, + ) -> Vec> { + let mut path = contract_keywords_path(keyword_search); + path.push(b"contractId".to_vec()); + path.push(contract_id.as_bytes().to_vec()); + path.push(vec![0]); + path + } + + /// The keys directly under `path`, sorted. Every caller compares the + /// result against an exact expectation, so a missing or empty subtree + /// fails the caller's assertion rather than passing quietly. + fn subtree_keys( + drive: &Drive, + path: Vec>, + platform_version: &PlatformVersion, + ) -> Vec> { + let (result, _) = drive + .grove + .query_raw( + &PathQuery::new_unsized(path, Query::new_range_full()), + false, + true, + true, + QueryResultType::QueryKeyElementPairResultType, + None, + &platform_version.drive.grove_version, + ) + .unwrap() + .expect("the subtree read must succeed"); + let mut keys: Vec> = result + .to_key_elements() + .into_iter() + .map(|(key, _)| key) + .collect(); + keys.sort(); + keys + } + + /// Is there a `byKeyword` group tree for `keyword`? + fn by_keyword_group_exists( + drive: &Drive, + keyword_search: &DataContract, + keyword: &str, + platform_version: &PlatformVersion, + ) -> bool { + let mut path = contract_keywords_path(keyword_search); + path.push(b"keyword".to_vec()); + let path_refs: Vec<&[u8]> = path.iter().map(|v| v.as_slice()).collect(); + drive + .grove_get_raw_optional( + path_refs.as_slice().into(), + keyword.as_bytes(), + DirectQueryType::StatefulDirectQuery, + None, + &mut vec![], + &platform_version.drive, + ) + .expect("the raw read must succeed") + .is_some() + } + + /// The keyword documents currently indexed under `byContractId` for + /// `contract_id`, read back through that index — the same query + /// `update_contract_keywords_operations_v0` uses to find what exists. + /// Returned as `(keyword, document id)` sorted by keyword, because the + /// document ids are what the raw tree-level assertions need. + fn keywords_indexed_by_contract_id( + drive: &Drive, + contract_id: Identifier, + platform_version: &PlatformVersion, + ) -> Vec<(String, Identifier)> { + let keyword_search = drive + .cache + .system_data_contracts + .load_keyword_search(platform_version) + .expect("load keyword_search"); + let document_type = keyword_search + .document_type_for_name("contractKeywords") + .expect("contractKeywords doctype"); + + let mut query = DriveDocumentQuery::all_items_query(&keyword_search, document_type, None); + query.internal_clauses.equal_clauses.insert( + "contractId".to_string(), + WhereClause { + field: "contractId".to_string(), + operator: WhereOperator::Equal, + value: Value::Identifier(contract_id.to_buffer()), + }, + ); + + let mut keywords: Vec<(String, Identifier)> = drive + .query_documents( + query, + None, + false, + None, + Some(platform_version.protocol_version), + ) + .expect("the byContractId query must succeed") + .documents_owned() + .into_iter() + .map(|document| { + ( + document + .properties() + .get_string("keyword") + .expect("every keyword document carries a keyword"), + document.id(), + ) + }) + .collect(); + keywords.sort(); + keywords + } + + /// The keywords alone, for the assertions that do not care about ids. + fn keyword_names(indexed: &[(String, Identifier)]) -> Vec { + indexed.iter().map(|(keyword, _)| keyword.clone()).collect() + } + + /// Document ids sorted the way grovedb orders subtree keys. + fn sorted_document_keys(indexed: &[(String, Identifier)]) -> Vec> { + let mut keys: Vec> = indexed + .iter() + .map(|(_, id)| id.as_bytes().to_vec()) + .collect(); + keys.sort(); + keys + } + + /// Replacing a contract's entire keyword set deletes every existing + /// keyword document and adds the new ones in **one** grovedb batch, all + /// sharing the single `byContractId/` group tree. Nothing in + /// that batch is aware of its siblings, so the deletes each decide the + /// group is not yet empty and leave it standing — which happens to be the + /// correct answer, because the adds land in the same group in the same + /// batch. + /// + /// That coincidence is what keeps the path correct, so it is worth + /// executing rather than reasoning about: if the group tree were ever + /// removed here, the new keyword documents would be written into a tree + /// the same batch deleted. The case where the coincidence runs out is + /// [`clearing_every_keyword_leaves_an_empty_by_contract_id_group_behind`]. + #[test] + fn replacing_a_contracts_whole_keyword_set_keeps_the_by_contract_id_group() { + let drive = setup_drive_with_initial_state_structure(None); + let platform_version = PlatformVersion::latest(); + + let keyword_search = + load_system_data_contract(SystemDataContract::KeywordSearch, platform_version) + .expect("load keyword_search"); + drive + .apply_contract( + &keyword_search, + BlockInfo::default(), + true, + None, + None, + platform_version, + ) + .expect("apply keyword_search"); + + let mut contract = get_data_contract_fixture(None, 0, platform_version.protocol_version) + .data_contract_owned(); + contract.set_keywords(vec!["alpha".to_string(), "bravo".to_string()]); + + drive + .apply_contract( + &contract, + BlockInfo::default(), + true, + StorageFlags::optional_default_as_cow(), + None, + platform_version, + ) + .expect("insert contract with keywords"); + + let before = keywords_indexed_by_contract_id(&drive, contract.id(), platform_version); + assert_eq!( + keyword_names(&before), + vec!["alpha".to_string(), "bravo".to_string()], + "baseline: both original keywords are indexed under byContractId" + ); + + let replacement = vec!["charlie".to_string(), "delta".to_string()]; + let group_path = by_contract_id_reference_path(&keyword_search, contract.id()); + + // The premise the correctness of this path rests on: a *single* + // operations vector carries both the deletes and the adds over that one + // group tree. Building the operations does not mutate anything, so this + // can be checked before applying them. + let operations = drive + .update_contract_keywords_operations( + contract.id(), + contract.owner_id(), + &replacement, + &BlockInfo::default(), + &mut None, + None, + platform_version, + ) + .expect("building the keyword update operations must succeed"); + let (deletes, inserts) = operations + .iter() + .fold((0, 0), |(deletes, inserts), operation| match operation { + LowLevelDriveOperation::GroveOperation(op) if op.path.to_path() == group_path => { + match op.op { + GroveOp::Delete | GroveOp::DeleteTree(..) => (deletes + 1, inserts), + _ => (deletes, inserts + 1), + } + } + _ => (deletes, inserts), + }); + assert_eq!( + (deletes, inserts), + (2, 2), + "one operations vector must carry both keyword deletes and both keyword adds \ + over the shared byContractId group; if these ever split into separate batches \ + this test no longer exercises the shape it exists to document" + ); + + // The disjoint replacement, applied. + drive + .update_contract_keywords( + contract.id(), + contract.owner_id(), + &replacement, + &BlockInfo::default(), + true, + None, + platform_version, + ) + .expect("replacing the whole keyword set should succeed"); + + let after = keywords_indexed_by_contract_id(&drive, contract.id(), platform_version); + assert_eq!( + keyword_names(&after), + replacement, + "the byContractId group must survive holding exactly the new keywords" + ); + + // The group tree itself, read raw rather than through a query: its + // children must be exactly the two new documents. This is the assertion + // that would catch a surviving reference to a deleted document, a + // duplicate, or a group that was removed and rebuilt with the wrong + // membership. + assert_eq!( + subtree_keys(&drive, group_path, platform_version), + sorted_document_keys(&after), + "the byContractId group's references must be exactly the new keyword documents" + ); + + // Primary document storage: the index can only show what it references, + // so orphaned documents would be invisible to the assertions above and + // to verify_grovedb alike. + let mut storage_path = contract_keywords_path(&keyword_search); + storage_path.push(vec![0]); + assert_eq!( + subtree_keys(&drive, storage_path, platform_version), + sorted_document_keys(&after), + "the deleted keyword documents must be gone from primary storage, not merely \ + unreferenced" + ); + + // The other index over the same documents. A delete that maintained + // byContractId but not byKeyword would leave keyword search — the + // contract's entire purpose — returning dead entries. + for stale in ["alpha", "bravo"] { + assert!( + !by_keyword_group_exists(&drive, &keyword_search, stale, platform_version), + "the byKeyword group for the removed keyword {stale} must be gone" + ); + } + for fresh in ["charlie", "delta"] { + assert!( + by_keyword_group_exists(&drive, &keyword_search, fresh, platform_version), + "the byKeyword group for the new keyword {fresh} must exist" + ); + } + + let issues = drive + .grove + .verify_grovedb(None, true, false, &platform_version.drive.grove_version) + .expect("verify_grovedb must run"); + assert!( + issues.is_empty(), + "grovedb integrity verification reported issues: {issues:?}" + ); + } + + /// **This test asserts a defect, not the desired behaviour**, and it exists + /// to put a price on one line in the caller. + /// + /// The deletes this function emits are blind to each other + /// (`previous_batch_operations` is `None`), so when two or more of them + /// jointly empty the shared `byContractId/` group each sees the + /// other's reference still committed, and the group tree survives with + /// nothing behind it. `verify_grovedb` cannot see that — primary and + /// secondary agree the empty group exists — so it is permanent state. + /// + /// Emptying the group without refilling it requires the *new* keyword set + /// to be empty: deletes cover `existing - new` and adds cover + /// `new - existing`, so if every existing document is deleted then + /// `existing` and `new` are disjoint, and the adds are empty only when + /// `new` is. `update_contract_v1` never calls into here with an empty set — + /// it guards the call with `!contract.keywords().is_empty()` — which is the + /// only reason a `DataContractUpdate` cannot produce this. The guard is + /// load-bearing, not an optimisation; this is what it is worth. + /// + /// `contractKeywords` has no ranked index, so the residue is state bloat + /// and a broken "a group tree exists therefore the group is non-empty" + /// invariant rather than a wrong query answer. It would not stay that way + /// if the same shape were ever built over a ranked index. + #[test] + fn clearing_every_keyword_leaves_an_empty_by_contract_id_group_behind() { + let drive = setup_drive_with_initial_state_structure(None); + let platform_version = PlatformVersion::latest(); + + let keyword_search = + load_system_data_contract(SystemDataContract::KeywordSearch, platform_version) + .expect("load keyword_search"); + drive + .apply_contract( + &keyword_search, + BlockInfo::default(), + true, + None, + None, + platform_version, + ) + .expect("apply keyword_search"); + + let mut contract = get_data_contract_fixture(None, 0, platform_version.protocol_version) + .data_contract_owned(); + contract.set_keywords(vec!["alpha".to_string(), "bravo".to_string()]); + drive + .apply_contract( + &contract, + BlockInfo::default(), + true, + StorageFlags::optional_default_as_cow(), + None, + platform_version, + ) + .expect("insert contract with keywords"); + + // Two blind deletes, no adds — reachable only by calling this API + // directly, which is what makes the caller's guard the real protection. + drive + .update_contract_keywords( + contract.id(), + contract.owner_id(), + &[], + &BlockInfo::default(), + true, + None, + platform_version, + ) + .expect("clearing every keyword should succeed"); + + assert!( + keywords_indexed_by_contract_id(&drive, contract.id(), platform_version).is_empty(), + "every keyword document must be gone" + ); + + let mut storage_path = contract_keywords_path(&keyword_search); + storage_path.push(vec![0]); + assert!( + subtree_keys(&drive, storage_path, platform_version).is_empty(), + "primary document storage must be empty" + ); + for stale in ["alpha", "bravo"] { + assert!( + !by_keyword_group_exists(&drive, &keyword_search, stale, platform_version), + "the byKeyword group for {stale} must be gone — those groups hold one \ + document each, so their deletes are never blind to a sibling" + ); + } + + // And the residue itself. If this ever starts returning `None` the + // defect has been fixed: delete this test and relax the guard note on + // `update_contract_v1`'s call site. + let mut group_level = contract_keywords_path(&keyword_search); + group_level.push(b"contractId".to_vec()); + let path_refs: Vec<&[u8]> = group_level.iter().map(|v| v.as_slice()).collect(); + let group = drive + .grove_get_raw_optional( + path_refs.as_slice().into(), + contract.id().as_bytes(), + DirectQueryType::StatefulDirectQuery, + None, + &mut vec![], + &platform_version.drive, + ) + .expect("the raw read must succeed"); + assert!( + group.is_some(), + "the emptied byContractId group tree is expected to survive — two blind deletes \ + each conclude the group is not yet empty" + ); + + let issues = drive + .grove + .verify_grovedb(None, true, false, &platform_version.drive.grove_version) + .expect("verify_grovedb must run"); + assert!( + issues.is_empty(), + "integrity verification cannot see the residue; got {issues:?}" + ); + } } diff --git a/packages/rs-platform-version/src/version/mocks/v2_test.rs b/packages/rs-platform-version/src/version/mocks/v2_test.rs index 3c7b3341245..4286f9fb97d 100644 --- a/packages/rs-platform-version/src/version/mocks/v2_test.rs +++ b/packages/rs-platform-version/src/version/mocks/v2_test.rs @@ -502,6 +502,9 @@ pub const TEST_PLATFORM_V2: PlatformVersion = PlatformVersion { max_field_value_size: 5000, max_document_value_depth: None, max_state_transition_size: 20000, // Is different in this test version, not sure if this was a mistake + // Load-bearing for state correctness, not just for throughput — see + // SystemLimits::max_transitions_in_documents_batch. Raising it here + // arms the defect inside drive-abci's own protocol-upgrade suite. max_transitions_in_documents_batch: 1, withdrawal_transactions_per_block_limit: 4, retry_signing_expired_withdrawal_documents_per_block_limit: 1, diff --git a/packages/rs-platform-version/src/version/system_limits/mod.rs b/packages/rs-platform-version/src/version/system_limits/mod.rs index a976af1e5d7..0431fd64231 100644 --- a/packages/rs-platform-version/src/version/system_limits/mod.rs +++ b/packages/rs-platform-version/src/version/system_limits/mod.rs @@ -14,6 +14,42 @@ pub struct SystemLimits { /// /// NOTE: This must be equal to the `max-tx-bytes` in the Tenderdash config pub max_state_transition_size: u64, + /// Maximum number of batched transitions (document and token transitions counted together) + /// one batch state transition may carry. + /// + /// This cap is load-bearing for state correctness, not merely a size or throughput limit. + /// `BatchTransitionAction::into_high_level_drive_operations` flattens every transition of a + /// batch into one `Vec`, and `apply_drive_operations` turns that vector into + /// a single GroveDB batch. Within one such batch the ordinary document Add/Update/Delete + /// conversions are blind to each other: the check that decides whether an index group tree + /// has become empty and should be removed sees committed state plus only the operations of + /// its own conversion. + /// + /// While the cap is 1, no two document operations can share a GroveDB batch *by way of a + /// batch state transition*, so on that route the blindness has nothing to act on. Raise it + /// and two operations that jointly empty a group each observe the other's document still + /// committed, each conclude the group is not yet empty, and the group tree survives with no + /// documents behind it. On a ranked index that leftover tree is mirrored into the aggregate + /// secondary, so the group keeps ranking with a zero aggregate — sorting ahead of every + /// group with a positive one — and, because primary and secondary agree that the empty + /// group exists, the state is internally consistent: integrity verification passes and + /// proofs attest the wrong ranking against the live root hash. + /// + /// The cap is not the only thing standing between that machinery and a live path, and a + /// reader raising it needs to know what the other two are: + /// + /// * `Drive::update_contract_keywords_operations` puts N blind document deletes and M adds + /// in one batch over a single shared index group. It stays correct only because its + /// caller skips it entirely when the new keyword set is empty, so any batch that empties + /// the group also refills it. That guard is load-bearing in the same way this cap is. + /// * `DocumentOperationType::MultipleDocumentOperationsForSameContractDocumentType` threads + /// the accumulated operations through, so document operations in *that* variant do see + /// their siblings — which is why the withdrawal paths batch many documents safely. It is + /// not a drop-in for batch transitions: it carries no delete variant. + /// + /// Five cases in `rs-drive`'s `batched_group_drain` suite are `#[ignore]`d for exactly this + /// reason; the rest of that suite runs. Anyone raising this cap should un-ignore those five + /// first and make them pass. pub max_transitions_in_documents_batch: u16, pub withdrawal_transactions_per_block_limit: u16, pub retry_signing_expired_withdrawal_documents_per_block_limit: u16, @@ -34,8 +70,71 @@ pub struct SystemLimits { #[cfg(test)] mod tests { + use crate::version::protocol_version::PLATFORM_VERSIONS; use crate::version::PlatformVersion; + /// The cap is what keeps two document operations out of a shared GroveDB batch, and with + /// them the phantom index groups described on `max_transitions_in_documents_batch`. It has + /// been 1 since the first mainnet release; a version that relaxes it must be a deliberate, + /// reviewed decision rather than a copy-paste into a new `SYSTEM_LIMITS_V*`. + #[test] + fn documents_batch_is_capped_at_one_transition_at_every_protocol_version() { + // Without this the loop below asserts nothing if the registry is ever + // emptied or truncated. + assert!( + PLATFORM_VERSIONS.len() >= 14, + "the protocol version registry lost entries; this test only covers what it holds" + ); + for platform_version in PLATFORM_VERSIONS { + assert_eq!( + platform_version + .system_limits + .max_transitions_in_documents_batch, + 1, + "protocol version {} allows more than one transition per documents batch; \ + see the documentation on SystemLimits::max_transitions_in_documents_batch \ + for what that exposes", + platform_version.protocol_version + ); + } + } + + /// The mock versions are never live, but they do execute state transitions + /// in drive-abci's protocol-upgrade suite, and one of them hand-writes its + /// `SystemLimits` rather than reusing a `SYSTEM_LIMITS_V*` — so it is the + /// one place the loop above cannot reach. A mock at a raised cap would + /// surface the phantom-group defect there as an unexplained failure. + /// + /// `PLATFORM_TEST_VERSIONS` is a process-global `OnceLock`, so if another + /// test in this binary initialised it first this asserts over whatever is + /// actually in use rather than over the defaults named here. That is the + /// more useful of the two, and deliberate. + #[cfg(feature = "mock-versions")] + #[test] + fn mock_platform_versions_carry_the_same_documents_batch_cap() { + use crate::version::mocks::v2_test::TEST_PLATFORM_V2; + use crate::version::mocks::v3_test::TEST_PLATFORM_V3; + use crate::version::protocol_version::PLATFORM_TEST_VERSIONS; + + let versions = + PLATFORM_TEST_VERSIONS.get_or_init(|| vec![TEST_PLATFORM_V2, TEST_PLATFORM_V3]); + assert!( + !versions.is_empty(), + "the mock version registry is empty; this test would assert nothing" + ); + for platform_version in versions { + assert_eq!( + platform_version + .system_limits + .max_transitions_in_documents_batch, + 1, + "mock platform version {} allows more than one transition per documents \ + batch; see SystemLimits::max_transitions_in_documents_batch", + platform_version.protocol_version + ); + } + } + #[test] fn document_value_depth_limit_starts_at_protocol_version_13() { // v12 is already active on live networks, so the limit must not apply there. diff --git a/packages/rs-platform-version/src/version/system_limits/v1.rs b/packages/rs-platform-version/src/version/system_limits/v1.rs index ca7da24bf5a..f36f5dd05f8 100644 --- a/packages/rs-platform-version/src/version/system_limits/v1.rs +++ b/packages/rs-platform-version/src/version/system_limits/v1.rs @@ -14,6 +14,11 @@ pub const SYSTEM_LIMITS_V1: SystemLimits = SystemLimits { // failed transition only, for all transitions, or for none — and the // transformer/dispatch code does not consistently express any of // those policies (see issue #2867). + // - The transitions of one batch are flattened into a single GroveDB + // batch whose document operations cannot see each other, so two of + // them that jointly empty an index group leave the group tree behind + // — a document-less group that still ranks, and still proves. See + // SystemLimits::max_transitions_in_documents_batch. // Before lifting this cap above 1, the whole batch validation + // transformer + nonce-bump path must be reviewed and the atomicity / // nonce semantics fixed. Pulling the cap higher today would expose diff --git a/packages/rs-platform-version/src/version/system_limits/v2.rs b/packages/rs-platform-version/src/version/system_limits/v2.rs index 295bbd6feea..1fcb2205aa0 100644 --- a/packages/rs-platform-version/src/version/system_limits/v2.rs +++ b/packages/rs-platform-version/src/version/system_limits/v2.rs @@ -11,6 +11,8 @@ pub const SYSTEM_LIMITS_V2: SystemLimits = SystemLimits { // v12 is already active on live networks; the depth limit activates in v13 (see v3). max_document_value_depth: None, max_state_transition_size: 20480, //20 KiB + // Load-bearing for state correctness, not just for throughput — see + // SystemLimits::max_transitions_in_documents_batch and SYSTEM_LIMITS_V1. max_transitions_in_documents_batch: 1, withdrawal_transactions_per_block_limit: 4, retry_signing_expired_withdrawal_documents_per_block_limit: 1, diff --git a/packages/rs-platform-version/src/version/system_limits/v3.rs b/packages/rs-platform-version/src/version/system_limits/v3.rs index 1181c4a405e..99472f10562 100644 --- a/packages/rs-platform-version/src/version/system_limits/v3.rs +++ b/packages/rs-platform-version/src/version/system_limits/v3.rs @@ -13,6 +13,8 @@ pub const SYSTEM_LIMITS_V3: SystemLimits = SystemLimits { // instance budget, bounding pre-schema work well above known document requirements. max_document_value_depth: Some(256), max_state_transition_size: 20480, //20 KiB + // Load-bearing for state correctness, not just for throughput — see + // SystemLimits::max_transitions_in_documents_batch and SYSTEM_LIMITS_V1. max_transitions_in_documents_batch: 1, withdrawal_transactions_per_block_limit: 4, retry_signing_expired_withdrawal_documents_per_block_limit: 1, From c05b9b3155c9cc9263f9549fe1e57cfc83111d98 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Thu, 13 Aug 2026 01:40:02 +0700 Subject: [PATCH 2/3] docs(platform-version): state what the keyword-set guard costs, not just what it prevents The bullet on max_transitions_in_documents_batch described the empty-keyword-set skip in update_contract_v1 as though it made the keyword path safe. It does not: it is a shield, not a fix. Calling the keyword update directly with an empty set still strands the shared byContractId group, and the skip leaves the previous keyword documents in place, so a contract that clears its keywords keeps being returned by keyword search under them. Both halves already had tests and a comment at the call site; only this bullet still read optimistically. Co-Authored-By: Claude Opus 5 (1M context) --- .../rs-platform-version/src/version/system_limits/mod.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/rs-platform-version/src/version/system_limits/mod.rs b/packages/rs-platform-version/src/version/system_limits/mod.rs index 0431fd64231..db13c74e02f 100644 --- a/packages/rs-platform-version/src/version/system_limits/mod.rs +++ b/packages/rs-platform-version/src/version/system_limits/mod.rs @@ -39,9 +39,12 @@ pub struct SystemLimits { /// reader raising it needs to know what the other two are: /// /// * `Drive::update_contract_keywords_operations` puts N blind document deletes and M adds - /// in one batch over a single shared index group. It stays correct only because its - /// caller skips it entirely when the new keyword set is empty, so any batch that empties - /// the group also refills it. That guard is load-bearing in the same way this cap is. + /// in one batch over a single shared index group. Every batch it actually emits refills + /// the group it empties, but only because its caller skips it outright when the new + /// keyword set is empty — and that skip is a shield, not a fix. Called directly with an + /// empty set it does strand the group, and the skip leaves the old keyword documents in + /// place, so a contract that clears its keywords keeps being found under them. Both + /// halves are pinned by tests; see the call site in `update_contract_v1`. /// * `DocumentOperationType::MultipleDocumentOperationsForSameContractDocumentType` threads /// the accumulated operations through, so document operations in *that* variant do see /// their siblings — which is why the withdrawal paths batch many documents safely. It is From e2f448083e5fa1a575daf3f8455609b0b6587299 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Thu, 13 Aug 2026 13:45:52 +0700 Subject: [PATCH 3/3] test(platform-version): check the version registry against LATEST_VERSION, not a fixed floor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The registry-completeness guard used a hardcoded ">= 14" floor, which only detects a registry that shrinks. Declare protocol version 15 by bumping LATEST_VERSION but forget to add PLATFORM_V15 to PLATFORM_VERSIONS, and the registry still holds 14 entries, the floor still passes, and the loop never inspects version 15 — so the batch-transition cap goes unchecked on the newest version, at exactly the moment someone is editing version machinery. Comparing the registry length against LATEST_VERSION closes that: it is declared independently of PLATFORM_VERSIONS, so it catches both a version declared but omitted and a registry that loses entries. Deriving the expectation from the registry instead — PlatformVersion::latest() is PLATFORM_VERSIONS.last() — would pass in both cases and is why that route was not taken. Verified by construction: with LATEST_VERSION at 15 and the registry holding 14, the old assertion passes and the new one fails (left: 14, right: 15); with PLATFORM_V14 dropped from the registry the new one fails (left: 13, right: 14). Co-Authored-By: Claude Opus 5 (1M context) --- .../src/version/system_limits/mod.rs | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/packages/rs-platform-version/src/version/system_limits/mod.rs b/packages/rs-platform-version/src/version/system_limits/mod.rs index db13c74e02f..5ceb6fceaf9 100644 --- a/packages/rs-platform-version/src/version/system_limits/mod.rs +++ b/packages/rs-platform-version/src/version/system_limits/mod.rs @@ -74,7 +74,7 @@ pub struct SystemLimits { #[cfg(test)] mod tests { use crate::version::protocol_version::PLATFORM_VERSIONS; - use crate::version::PlatformVersion; + use crate::version::{PlatformVersion, LATEST_VERSION}; /// The cap is what keeps two document operations out of a shared GroveDB batch, and with /// them the phantom index groups described on `max_transitions_in_documents_batch`. It has @@ -82,11 +82,19 @@ mod tests { /// reviewed decision rather than a copy-paste into a new `SYSTEM_LIMITS_V*`. #[test] fn documents_batch_is_capped_at_one_transition_at_every_protocol_version() { - // Without this the loop below asserts nothing if the registry is ever - // emptied or truncated. - assert!( - PLATFORM_VERSIONS.len() >= 14, - "the protocol version registry lost entries; this test only covers what it holds" + // The loop below only inspects what the registry holds, so the registry + // has to be known complete first. `LATEST_VERSION` is declared + // independently of `PLATFORM_VERSIONS`, which is what makes it a usable + // reference point: a version that is declared but never added to the + // registry leaves the count short and fails here, and so does a registry + // that loses entries. Deriving the expectation from the registry itself + // — `PlatformVersion::latest()` is `PLATFORM_VERSIONS.last()` — would + // pass in both cases. + assert_eq!( + PLATFORM_VERSIONS.len(), + LATEST_VERSION as usize, + "the protocol version registry does not hold every declared version, so the cap \ + would go unchecked on the ones it is missing" ); for platform_version in PLATFORM_VERSIONS { assert_eq!(