test(drive): pin the batch-transition cap and land the phantom-group evidence - #4383
Conversation
…evidence `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<DriveOperation>` 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) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe pull request adds ranked-index group-drain tests for document and keyword operations. It documents and verifies the single-transition batch limit across platform versions, including GroveDB integrity, ranking, proofs, and residual empty-group behavior. ChangesRanked-index state consistency
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: ⚪ Minimal · up to This PR documents and tests the existing batch-transition cap without changing runtime behavior, so no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
✅ Final review complete — no blockers (commit e2f4480) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/rs-platform-version/src/version/system_limits/mod.rs (1)
84-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving the registry-size floor instead of hardcoding
14.The literal
14must be bumped by hand whenever a protocol version is added, and a stale value silently weakens the guard. A simple alternative pins the count against the highest registeredprotocol_version, which self-updates.♻️ Alternative floor assertion
- assert!( - PLATFORM_VERSIONS.len() >= 14, - "the protocol version registry lost entries; this test only covers what it holds" - ); + assert_eq!( + PLATFORM_VERSIONS.len(), + PlatformVersion::latest().protocol_version as usize, + "the protocol version registry lost entries; this test only covers what it holds" + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-version/src/version/system_limits/mod.rs` around lines 84 - 87, Update the registry-size assertion in the system-limits test to derive its minimum from the highest registered protocol_version rather than the hardcoded 14. Reuse the existing PLATFORM_VERSIONS entries and preserve the current failure message and guard intent.packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/batched_group_drain.rs (1)
1072-1235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the shared driver for the two "never lands silently wrong" cases.
deleting_a_groups_only_document_and_creating_another_in_one_batch_never_lands_silently_wrongandmoving_one_document_out_of_a_group_while_creating_another_into_it_never_lands_silently_wrongshare the same triple loop, label construction,report/badaccumulation, and match arms. Only three things differ: the departing operation, the two order labels, anddeparture_storedin the success expectation. A single driver that accepts those three inputs would keep the two cases in sync as the assertions evolve.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/batched_group_drain.rs` around lines 1072 - 1235, Extract the duplicated test execution logic from the two “never_lands_silently_wrong” tests into a shared helper or driver. Parameterize it with the departing operation, the two order labels, and the expected departure_stored value, while keeping each test responsible only for constructing its scenario-specific inputs; preserve the existing loops, reporting, refusal assertions, and success expectations.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/ranked_group_drain.rs (1)
544-586: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the repeated platform and fixture setup.
The three tests repeat the same block: build the platform, load the state and platform version, set up the identity, register the restaurants contract, resolve the
visitdoctype, seed the RNG, and create the[(H, 2), (G, 4), (G, 6)]documents.a_multi_document_batch_transition_is_refused_by_the_one_transition_limitand this test create an identical population. One async fixture helper returning the platform, contract, doctype, key, signer, and documents would keep the three cases in sync, matching thesetup_g_two_h_onepattern already used inpackages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/batched_group_drain.rs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/ranked_group_drain.rs` around lines 544 - 586, Extract the duplicated platform and fixture initialization from the affected tests into one async helper, following the existing setup_g_two_h_one pattern. Have the helper build the platform, establish identity and signing data, register the restaurant contract, resolve the visit document type, seed the RNG, and create the [(H, 2), (G, 4), (G, 6)] documents, returning all values needed by the tests; update a_multi_document_batch_transition_is_refused_by_the_one_transition_limit and deleting_the_same_two_documents_in_separate_blocks_removes_the_group to reuse it.
🔇 Additional comments (30)
packages/rs-platform-version/src/version/system_limits/mod.rs (2)
41-44: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the keyword-guard claim against the keyword-clearing behavior.
This bullet states that the caller of
Drive::update_contract_keywords_operationsskips the call when the new keyword set is empty, so "any batch that empties the group also refills it". The PR adds a test namedclearing_every_keyword_leaves_an_empty_by_contract_id_group_behind, andbatched_group_drain.rs(lines 31-35) points a reader to that test "for what that guard is worth". Those two statements suggest the guard does not fully prevent a residual empty group on the clearing path. If clearing can leave an emptyby_contract_idgroup, soften this bullet so the documentation does not overstate the guard.Run the following script to inspect the keyword-update path and the clearing test:
73-137: LGTM!packages/rs-platform-version/src/version/system_limits/v1.rs (1)
17-21: LGTM!packages/rs-platform-version/src/version/system_limits/v2.rs (1)
14-15: LGTM!packages/rs-platform-version/src/version/system_limits/v3.rs (1)
16-17: LGTM!packages/rs-platform-version/src/version/mocks/v2_test.rs (1)
505-507: LGTM!packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/batched_group_drain.rs (7)
1-43: LGTM!Also applies to: 60-96, 98-167, 173-227
45-58: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that every parent-module helper this file relies on exists.
This module is attached with
#[path]toranked_index_e2e_tests, souse super::*must supplyread_grove_element,setup_restaurants,build_doc,insert_doc,count_top_k,sum_top_k,avg_top_k,group_keys,expected_avg_fixed_point,indexed_property_name_tree_path,assert_grovedb_is_consistent,GROUP_PROPERTY, andplatform_version. A rename in the parent breaks this file only at compile time in that crate's test profile, which is easy to miss locally.Run the following script to confirm each symbol is defined in the parent module:
398-468: LGTM!
559-648: LGTM!
650-799: LGTM!Also applies to: 840-901
1245-1299: LGTM!
497-527: 🩺 Stability & Availability
⚠️ Unverified finding
Sandbox verification was unavailable.Both new suites load the same restaurants fixture through crate-relative paths. The shared root cause is one assumption:
restaurants-contract.jsonsits atpackages/rs-drive/tests/supporting_files/contract/restaurants/, and Cargo runs each crate's unit tests with that crate's root as the working directory. If the fixture moves, both suites break at runtime rather than at compile time.
packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/batched_group_drain.rs#L497-L527: confirm"tests/supporting_files/contract/restaurants/restaurants-contract.json"resolves from thers-drivecrate root.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/ranked_group_drain.rs#L53-L54: confirm"../rs-drive/tests/supporting_files/contract/restaurants/restaurants-contract.json"resolves from thers-drive-abcicrate root to the same file.packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/mod.rs (1)
24-27: LGTM!packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/ranked_index_e2e_tests.rs (2)
49-53: LGTM!
55-58: 📐 Maintainability & Code QualityNo change required.
#[path = "batched_group_drain.rs"]resolves relative to the directory containingranked_index_e2e_tests.rs.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/mod.rs (1)
5-5: LGTM!packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/ranked_group_drain.rs (6)
58-120: LGTM!
145-202: LGTM!
206-233: LGTM!
297-403: LGTM!
405-542: LGTM!
588-618: LGTM!packages/rs-drive/src/drive/contract/update/update_contract/v1/mod.rs (3)
256-271: LGTM!
471-516: LGTM!
518-589: LGTM!packages/rs-drive/src/drive/contract/update/update_keywords/v0/mod.rs (4)
183-203: LGTM!
378-398: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Reuse the canonical path helpers instead of hardcoding the subtree markers.
contract_keywords_pathandby_contract_id_reference_pathencode the storage layout as literals:vec![1]for the document-types subtree andvec![0]for the primary/reference subtree. These literals carry no name and no comment.This matters for one assertion in particular.
subtree_keysreturns an empty vector when the path does not resolve. Line 759 asserts thatsubtree_keys(&drive, storage_path, platform_version)is empty. If the layout markers ever change, that assertion passes without reading the intended subtree. The assertions that compare againstsorted_document_keys(&after)do not have this problem, so the doc comment at lines 400-402 is accurate for those callers only.
rs-drivealready exposes path constructors for contract document storage. Build these helpers on top of them, or name the markers with the existing layout constants.Run the following script to find the canonical helpers:
522-680: LGTM!
682-801: LGTM!
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/ranked_group_drain.rs`:
- Around line 544-586: Extract the duplicated platform and fixture
initialization from the affected tests into one async helper, following the
existing setup_g_two_h_one pattern. Have the helper build the platform,
establish identity and signing data, register the restaurant contract, resolve
the visit document type, seed the RNG, and create the [(H, 2), (G, 4), (G, 6)]
documents, returning all values needed by the tests; update
a_multi_document_batch_transition_is_refused_by_the_one_transition_limit and
deleting_the_same_two_documents_in_separate_blocks_removes_the_group to reuse
it.
In
`@packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/batched_group_drain.rs`:
- Around line 1072-1235: Extract the duplicated test execution logic from the
two “never_lands_silently_wrong” tests into a shared helper or driver.
Parameterize it with the departing operation, the two order labels, and the
expected departure_stored value, while keeping each test responsible only for
constructing its scenario-specific inputs; preserve the existing loops,
reporting, refusal assertions, and success expectations.
In `@packages/rs-platform-version/src/version/system_limits/mod.rs`:
- Around line 84-87: Update the registry-size assertion in the system-limits
test to derive its minimum from the highest registered protocol_version rather
than the hardcoded 14. Reuse the existing PLATFORM_VERSIONS entries and preserve
the current failure message and guard intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f3ba5b36-f193-4ab3-9f91-ffe896753933
📒 Files selected for processing (12)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/ranked_group_drain.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/batched_group_drain.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/mod.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/ranked_index_e2e_tests.rspackages/rs-drive/src/drive/contract/update/update_contract/v1/mod.rspackages/rs-drive/src/drive/contract/update/update_keywords/v0/mod.rspackages/rs-platform-version/src/version/mocks/v2_test.rspackages/rs-platform-version/src/version/system_limits/mod.rspackages/rs-platform-version/src/version/system_limits/v1.rspackages/rs-platform-version/src/version/system_limits/v2.rspackages/rs-platform-version/src/version/system_limits/v3.rs
…ust 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) <noreply@anthropic.com>
|
Thanks for the review. Responses to the three nitpicks and the flagged finding, in order.
Not applying this one — I think it inverts the guard. The premise is also slightly off: a
Leaving these as two explicit tests. They differ in the departing operation, the order labels, and the expected
Leaving as is. The substantive helpers are already extracted —
Good catch, and correct — fixed in c05b9b3. You inferred it from the two documents disagreeing and you were right: the guard-site comment in It now says that every batch the keyword update actually emits refills the group it empties, but that the skip is a shield rather than a fix — called directly with an empty set it does strand the group, and the skip leaves the previous keyword documents in place, so a contract that clears its keywords keeps being found under them. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The PR documents and characterizes the existing batch-index defect without changing consensus behavior, but the new registry-wide cap test does not remain complete when another protocol version is declared. Its independent completeness assertion should use the authoritative latest protocol version rather than a fixed floor.
Source: reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-version/src/version/system_limits/mod.rs`:
- [SUGGESTION] packages/rs-platform-version/src/version/system_limits/mod.rs:87-90: Derive the registry completeness check from LATEST_VERSION
The fixed `>= 14` floor only detects truncation below the current registry size. If protocol version 15 is declared by updating `LATEST_VERSION` but `PLATFORM_V15` is accidentally omitted from `PLATFORM_VERSIONS`, the registry will still contain 14 entries, this assertion will pass, and the loop will never inspect version 15. Compare the registry length with the independent, authoritative `LATEST_VERSION` constant so the test continues to verify every declared protocol version. Unlike `PlatformVersion::latest()`, that constant is not derived from the registry being checked.
| assert!( | ||
| PLATFORM_VERSIONS.len() >= 14, | ||
| "the protocol version registry lost entries; this test only covers what it holds" | ||
| ); |
There was a problem hiding this comment.
🟡 Suggestion: Derive the registry completeness check from LATEST_VERSION
The fixed >= 14 floor only detects truncation below the current registry size. If protocol version 15 is declared by updating LATEST_VERSION but PLATFORM_V15 is accidentally omitted from PLATFORM_VERSIONS, the registry will still contain 14 entries, this assertion will pass, and the loop will never inspect version 15. Compare the registry length with the independent, authoritative LATEST_VERSION constant so the test continues to verify every declared protocol version. Unlike PlatformVersion::latest(), that constant is not derived from the registry being checked.
| assert!( | |
| PLATFORM_VERSIONS.len() >= 14, | |
| "the protocol version registry lost entries; this test only covers what it holds" | |
| ); | |
| assert_eq!( | |
| PLATFORM_VERSIONS.len(), | |
| crate::version::LATEST_VERSION as usize, | |
| "the protocol version registry does not contain every declared protocol version" | |
| ); |
source: ['codex']
There was a problem hiding this comment.
Resolved in e2f4480 — Derive the registry completeness check from LATEST_VERSION no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4383 +/- ##
============================================
- Coverage 87.49% 86.75% -0.74%
============================================
Files 2672 2676 +4
Lines 340400 344267 +3867
============================================
+ Hits 297819 298668 +849
- Misses 42581 45599 +3018
🚀 New features to boost your workflow:
|
…SION, not a fixed floor 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) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The sole prior suggestion is fixed at the exact head: the cap test now compares PLATFORM_VERSIONS.len() against the independently declared LATEST_VERSION, so a declared but unregistered protocol version fails the completeness guard. No additional in-scope findings were reported or identified, and the PR remains documentation-and-test-only with respect to runtime behavior.
Source: Reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
2 participants
Issue being fixed or feature implemented
max_transitions_in_documents_batchhas been1at every protocol version since the first mainnet release, and nothing in the codebase said why that matters. It reads as a throughput knob. It is not — it is the only thing keeping a consensus-visible index defect off the network, and raising it (an attractive, plausible future change) would arm that defect silently.This PR does not fix the defect. It documents the cap, pins it with tests, and lands the evidence so the next person to touch it cannot miss what it is holding up.
What was done?
The mechanism.
BatchTransitionAction::into_high_level_drive_operationsflattens every transition of a batch into oneVec<DriveOperation>, andapply_drive_operationsturns that into a single GroveDB batch. Within that batch the ordinary document Add/Update/Delete conversions are blind to each other:batch_delete_up_tree_while_emptydecides whether an index group has become empty from committed state plus only its own operations. Two operations that jointly empty a group each observe the other's document still committed, neither removes the group tree, and on a ranked index the leftover tree is mirrored into the aggregate secondary — so the group keeps ranking at zero with nothing behind it, sorting ahead of every group with a positive aggregate. Primary and secondary agree the empty group exists, soverify_grovedbis clean and the proof reconstructs the live root hash. A wrong ranking that verifies.Why it is not fixed here. Fixing it at the correct site (the flatten loop in
apply_drive_operations_v0, not the five&mut Nonecall sites — those are a no-op) changes the number of emitted GroveDB operations, hence fees, hence identity balances, hence the app hash. It would also breakDrive::update_contract_keywords_operations, which concatenates N deletes then M adds over a sharedbyContractIdgroup and is correct because its deletes are blind — sibling awareness would make the second delete conclude the group is empty and delete it, then the adds would insert into a tree the same batch deleted. That needs a protocol-version gate and its own PR.Landed instead:
SystemLimits::max_transitions_in_documents_batch, with pointers from all threeSYSTEM_LIMITS_V*constants and from the one hand-writtenSystemLimitsliteral in the mocks. It also names the two other guards holding the same line, because "the cap is the only thing" is not true in general:MultipleDocumentOperationsForSameContractDocumentType, which does thread accumulated operations and is why the withdrawal paths batch many documents safely — and which is not a drop-in for batch transitions, because it carries no delete variant. That is the detail that makes the obvious fix non-obvious.PLATFORM_VERSIONS, and (undermock-versions) across the mock registry, since one mock hand-writes itsSystemLimitsand the main loop cannot see it.rs-drive-abcireachability coverage (ranked_group_drain.rs): a real signed two-transitionBatchTransitionis refused by the cap before any drive operation is built, and the reachable near-miss — two transitions draining the same group in one block — produces the correct index, becauseexecute_eventcallsapply_drive_operationsonce per state transition.rs-drivemechanism coverage (batched_group_drain.rs): five cases that expose the defect land#[ignore]d rather than deleted or weakened, so raising the cap has something concrete to un-ignore. They fail today, for the right reason. Everything else in the file runs: a characterization test asserting the phantom exactly as it is on all three axes (including that it verifies and proves), the fail-loud inverse shapes, and sequential controls for both the delete and move paths.byContractIdgroup holding exactly the new members. Clearing it entirely strands an empty group tree — and the only thing preventing that from the network is!contract.keywords().is_empty()inupdate_contract_v1. That guard is a shield, not a fix: it also skips the deletes, so a contract that clears its keywords keeps being returned by keyword search under the old ones. Both halves are commented at the guard and pinned by characterization tests, because removing the guard to fix the stale index would silently arm the stranded group tree.How Has This Been Tested?
Rebased onto
v4.2-dev@f05bf82dc9and re-run there. No conflicts (this branch touches noCargo.lock).cargo clippy --workspace --all-features --all-targetscargo fmt --check --allcargo test -p drive --libcargo test -p drive --lib batched_group_drain -- --ignoredcargo test -p drive-abci --lib batch::testscargo test -p platform-version --lib--features mock-versionsNot overstating this: the clippy gate is green only after retries. Two third-party build scripts download GitHub artifacts (
tenderdash-protofetching the Tenderdash source zip,grovedb'sgrovedbgfeature fetching a release zip) and both failed repeatedly from this machine withPeer disconnected/ HTTP2REFUSED_STREAM. I retried until the downloads succeeded, verified the grovedbg artifact against the SHA-256 the build script pins, seeded both build scripts' own documented caches, and then got a clean full-workspace run. Nothing was skipped or narrowed to work around it — the green run above is the real full-feature, all-targets workspace lint. Flagging it because it is the exact command a reviewer will re-run, and it may flake for them too.Three independent review passes were run over the diff (correctness, adversarial test-quality, consensus/version-safety) and their must-fixes are folded in. Two are worth naming because the first version was genuinely weaker than it looked:
Err". They now assert the refusal kind per batching configuration — Drive's pre-flight check whenbatching_consistency_verificationis on, grovedb's batch applier on the shipped default, which is the only guard a real node has — and that a refused batch leaves state unchanged.One verification limit, stated rather than papered over:
clearing_a_contracts_keywords_leaves_the_old_ones_indexeddrives Drive-layerupdate_contract, not a signedDataContractUpdateend to end. Reachability of the stale-keyword behaviour from the network is strongly indicated by the call path but is not executed here.Breaking Changes
None. No wire format, serialization, fee, or state change. Every non-test hunk in this PR is a comment; no value, version mapping, or dispatch entry is touched, and the
1literals themselves are unchanged.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests