fix(drive-abci): paid/unpaid classification for invalid batch transitions#3616
Open
fix(drive-abci): paid/unpaid classification for invalid batch transitions#3616
Conversation
Closes #2867. The Documents Batch transformer's per-transition handler was emitting BumpIdentityDataContractNonceAction only on `find_replaced_document_v0` failure for the Replace arm. Every other failure path (ownership-check, revision-check, plus the analogous paths in Transfer / UpdatePrice / Purchase, and Purchase's listed-price / price-mismatch checks) returned ConsensusValidationResult errors-only with no action data. The empty BatchTransitionAction that flowed up the chain still triggered fee accounting (PaidConsensusError) but the bump action's UpdateIdentityContractNonce drive op was never created — so identity_contract_nonce in state stayed at the pre-tx value. Consequence on mainnet: a user's SDK could re-broadcast the same exact serialized bytes; CheckTx FirstTimeCheck would still pass (committed state still says the nonce is unused), the proposer would include the tx in a later block, deliver_tx would fail again with the same error, and the cycle could repeat. Result: the same state-transition hash appearing in multiple blocks — exactly what mainnet ST hash 35C08D574302D32D7160E603D8159042C8606BE12FD97D952CF5FD40DB57313C did on 2026-05-04 (block 361774 idx 1 + a later block past the explorer indexer's panic point). Fix: copy the bump-emission pattern from the existing find_replaced_document_v0 failure path (lines 672-686) to all other failure sites in the Replace / Transfer / UpdatePrice / Purchase handlers. Each now returns `new_with_data_and_errors(BumpIdentityDataContractNonce(bump_action), errors)` so the contract nonce advances regardless of where the per-transition validation fails. This is a consensus-affecting change on PROTOCOL_VERSION_12 (current v3.1-dev). Validators that fail the same transition under the new behavior pay a higher fee than under the old behavior because the fee now covers the document fetch + ownership/revision check work that ran before the failure (those drive ops were already happening, just charged as 0). Three sibling tests had hard-coded fee assertions matching the old (under-charged) numbers; they're updated with comments explaining the change. Test plan - Added regression test `replayed_failed_replace_with_consumed_nonce_must_be_rejected_at_check_tx` in batch/tests/document/replacement.rs that reproduces the Techawanka.dash mainnet 35C0 shape (Create at nonce 2 → Replace at nonce 3 with mismatching revision → assert state nonce advanced AND CheckTx of identical bytes returns InvalidIdentityNonceError). RED on parent commit, GREEN with the fix. - Added decoder pin `should_decode_mainnet_35c0_documents_batch_replace_replay` in rs-dpp serialization tests confirming the mainnet wire bytes deserialize cleanly to a Batch / Replace / nonce=3 — the explorer's reported "Cannot deserialize" is its own version-skew, not a chain-bytes issue. - `cargo test -p drive-abci --lib`: 2424 passed, 0 failed (full suite). - `cargo test -p drive-abci --lib batch::tests::`: 254 passed, 0 failed. Out of scope - platform-explorer's `UNIQUE(hash)` constraint on `state_transitions` is too strict regardless of this fix; Tenderdash never guaranteed cross-block tx-hash uniqueness (failed-but-paid txs are committed by protocol design, like Ethereum reverts). Recommend `UNIQUE(block_height, hash)` upstream in pshenmic/platform-explorer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes part of #2867 — the architectural side. The companion PR (#3608) handles the bump-on-failure side at PROTOCOL_VERSION_12; this PR fixes the deeper invariant violation that made bump-emission necessary in the first place. Root cause ---------- `ConsensusValidationResult::merge_many` and `ConsensusValidationResult::flatten` always produce `data: Some(Vec)` — even when the input was empty or every input had `data: None`. This violates the implicit contract `data.is_none() ⇔ no work done` that `process_validation_result_v0:241` keys on: if validation_result.data.is_none() { return Ok(StateTransitionExecutionResult::UnpaidConsensusError(...)); }; When per-transition validation in a Documents Batch fails for every transition (no fallback action is emitted), the per-transition results all carry `data: None`. The deprecated aggregators wrap that as `Some(empty_vec)` → `try_into_action_v0:278`'s `has_data()` check returns `true` → an empty `BatchTransitionAction { transitions: vec![] }` flows up → `process_state_transition_v0:282` `is_valid_with_data() == false` but `data.is_some()` → `map_result` produces an `ExecutionEvent::Paid` with no drive ops → `process_validation_result_v0` classifies as `PaidConsensusError`, charges the user a small fee for zero actual work, leaves the tx in the block. This is the "validating state transition for free" gap. Fix --- - New `merge_many_strict` / `flatten_strict` on `ValidationResult<Vec<TData>, E>`: identical semantics except they return `data: None` when no input contributed any data (empty input, all inputs `data: None`, or all inputs `Some(empty_vec)`). - Existing `merge_many` / `flatten` are marked `#[deprecated]`. Their behavior is unchanged and they remain in the codebase because removing them would break PROTOCOL_VERSION_11 and earlier chain reproducibility. - New `transformer/v1/mod.rs` is a near-copy of `transformer/v0/mod.rs` with all four aggregator call sites switched to the strict variants. v0 is preserved verbatim for v11 (it gets a file-level `#![allow(deprecated)]` to silence the deprecation warnings). - `batch/mod.rs` dispatches `transform_into_action == 1` to v1. - `PLATFORM_V12.batch_state_transition.transform_into_action` bumped from 0 to 1 (in `DRIVE_ABCI_VALIDATION_VERSIONS_V8`, used only by PLATFORM_V12; PLATFORM_V11's `_V7` is untouched). Effect ------ On PROTOCOL_VERSION_11 (mainnet today): unchanged. Same chain history. On PROTOCOL_VERSION_12+ (the v3.1 hard-fork target): a Documents Batch where every per-transition validation fails-without-action surfaces as `UnpaidConsensusError` instead of an empty `PaidConsensusError`. `prepare_proposal:223` removes the tx from the block. No fee charged. No replay surface (the tx never landed in the chain to begin with). Test plan --------- - `validation_result.rs` unit tests (in this PR): 9 new strict-variant tests + 2 new pinning tests for the deprecated buggy behavior. Local run: `57 passed; 0 failed`. - `batch/tests/document/{replacement,transfer,nft}.rs`: existing scenarios where the empty-action shape is reachable (immutable Replace, transfer-of-missing-doc, set-price-on-not-owned) factored into helper functions; default test (PlatformVersion::latest = v12) asserts the new `UnpaidConsensusError` shape with `processing_fee == 0`; new `*_protocol_version_11` siblings assert the historical `PaidConsensusError` + 41880/36200 fees are preserved bit-for-bit. - Full `drive-abci` test run is left to CI in this PR (the local run was time-prohibitive in a fresh worktree's empty `target/`). Out of scope ------------ Partial-batch failures (some transitions succeed, some fail-without- action) still leak the failed transitions' contract nonces — the strict aggregator only fixes the all-failed shape. PR #3608 (bump on failure) addresses the per-transition gap; together, the two PRs cover all known issue #2867 shapes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds v0/v1 implementations and platform-version facades for ValidationResult aggregators ( ChangesValidation Result Aggregation Versioning
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
✅ Review complete (commit 964df5b) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/replacement.rs (1)
702-712: ⚡ Quick winPin this regression to protocol 12 explicitly.
This helper is validating the protocol boundary introduced by the fix, so
PlatformVersion::latest()makes the test drift as soon as a newer protocol ships. Calling the helper with12keeps the v11/v12 contract stable.Suggested change
- async fn test_document_replace_on_document_type_that_is_not_mutable() { + async fn test_document_replace_on_document_type_that_is_not_mutable_protocol_version_12() { run_document_replace_on_immutable_doc_type_at_protocol_version( - PlatformVersion::latest().protocol_version, + 12, true, // architectural fix active 0, // no fee charged on UnpaidConsensus )🤖 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/replacement.rs` around lines 702 - 712, The test uses PlatformVersion::latest().protocol_version which makes the protocol boundary drift; change the call to pass the explicit protocol number 12 to run_document_replace_on_immutable_doc_type_at_protocol_version (i.e. replace PlatformVersion::latest().protocol_version with 12) so the test pins the v11/v12 contract as intended.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/nft.rs (1)
2847-2855: ⚡ Quick winPin this regression to protocol 12 explicitly.
PlatformVersion::latest()will drift once the next protocol lands, so this stops proving the exact v11/v12 split this PR is freezing. Use the explicit protocol-12 case here, and keep a separate forward-compat test if you also want “latest” coverage.Suggested change
- async fn test_document_set_price_on_not_owned_document() { + async fn test_document_set_price_on_not_owned_document_protocol_version_12() { run_document_set_price_on_not_owned_document_at_protocol_version( - PlatformVersion::latest().protocol_version, + 12, true, // architectural fix active 0, // no fee charged on UnpaidConsensus )🤖 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/nft.rs` around lines 2847 - 2855, The test uses PlatformVersion::latest().protocol_version which will drift; change the call in test_document_set_price_on_not_owned_document to pass the explicit protocol 12 value to run_document_set_price_on_not_owned_document_at_protocol_version (i.e., replace PlatformVersion::latest().protocol_version with the literal protocol 12) so this test pins the v11/v12 regression; if you still want "latest" coverage, add a separate test that calls run_document_set_price_on_not_owned_document_at_protocol_version with PlatformVersion::latest().protocol_version.
🤖 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/nft.rs`:
- Around line 2847-2855: The test uses
PlatformVersion::latest().protocol_version which will drift; change the call in
test_document_set_price_on_not_owned_document to pass the explicit protocol 12
value to run_document_set_price_on_not_owned_document_at_protocol_version (i.e.,
replace PlatformVersion::latest().protocol_version with the literal protocol 12)
so this test pins the v11/v12 regression; if you still want "latest" coverage,
add a separate test that calls
run_document_set_price_on_not_owned_document_at_protocol_version with
PlatformVersion::latest().protocol_version.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/replacement.rs`:
- Around line 702-712: The test uses PlatformVersion::latest().protocol_version
which makes the protocol boundary drift; change the call to pass the explicit
protocol number 12 to
run_document_replace_on_immutable_doc_type_at_protocol_version (i.e. replace
PlatformVersion::latest().protocol_version with 12) so the test pins the v11/v12
contract as intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0e7fe3e-b701-4837-bf57-b108158e8c42
📒 Files selected for processing (10)
packages/rs-dpp/src/validation/validation_result.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/state/v0/fetch_documents.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/nft.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/replacement.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/transfer.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/v1/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v8.rs
…acy → _or_empty_vec Followup on the previous commit's naming. The corrected aggregator behavior (returning data:None when no input contributed) is the intended semantic and gets the canonical names `merge_many` / `flatten`. The legacy `Some(empty_vec)`-on-no-data behavior is renamed to `merge_many_or_empty_vec` / `flatten_or_empty_vec` and kept in the codebase only for PROTOCOL_VERSION_11 chain reproducibility. This biases the codebase toward correctness: any future caller typing the canonical name gets the right semantics. The legacy variants are clearly documented as v11-compat only. - `validation_result.rs`: rename methods, swap doc comments, swap test assertions (the canonical-named tests now assert the corrected behavior; the `_or_empty_vec` tests pin the legacy v11 shape). - `transformer/v0/mod.rs`: 4 call sites migrated to `_or_empty_vec`. Remove the file-level `#![allow(deprecated)]` (nothing is deprecated anymore). - `fetch_documents.rs`: 1 call site migrated to `_or_empty_vec`. - `transformer/v1/mod.rs`: 4 call sites use canonical names. - `v8.rs`: comment updated to reference `_or_empty_vec`. - Doc comments updated throughout. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
shumkov
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3616 +/- ##
============================================
- Coverage 88.29% 88.29% -0.01%
============================================
Files 2479 2483 +4
Lines 301659 301901 +242
============================================
+ Hits 266347 266559 +212
- Misses 35312 35342 +30
🚀 New features to boost your workflow:
|
shumkov
changed the title
- Mark `merge_many_or_empty_vec` and `flatten_or_empty_vec` with
`#[deprecated]` so new code is steered toward the canonical
`merge_many` / `flatten` (which honor `data.is_none() ⇔ no work
done`). The legacy methods stay in the codebase strictly for
PROTOCOL_VERSION_11 chain reproducibility — the deprecation note
spells this out.
- Suppress the warning at the legitimate v11-compat call sites:
`#![allow(deprecated)]` at the top of `transformer/v0/mod.rs` (the
whole v0 transformer is dedicated to v11), `#[allow(deprecated)]`
on the single call in `fetch_documents.rs` and on each legacy-
pinning test in `validation_result.rs`.
- Replace the verbose `ValidationResult { errors, data: None }` struct
literal in the canonical `flatten` / `merge_many` with the existing
`new_with_errors` constructor.
- Document the issue #2867 batch-transformer version bump in v8.rs
alongside the existing basic_structure note.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/v1/mod.rs`:
- Around line 1003-1005: The current revision bump uses plain addition
(previous_revision + 1) which can overflow; replace it with checked_add on
previous_revision to compute expected_revision (e.g.,
previous_revision.checked_add(1)) and handle the None case as an
invalid/overflow error (the same failure path used elsewhere) before comparing
to transition_revision; update the if that compares transition_revision to
expected_revision to use the checked result and return the appropriate
validation error when checked_add returns None or when the revisions don't
match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8dc5c3bf-a3b1-4dc1-97dc-40a02c2fbce5
📒 Files selected for processing (5)
packages/rs-dpp/src/validation/validation_result.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/state/v0/fetch_documents.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/v1/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v8.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/state/v0/fetch_documents.rs
- packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v8.rs
- packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/v0/mod.rs
Closes the partial-failed-batch replay surface (issue #2867) without abandoning successful transitions. When a batch contains a mix of successful and failed per-transition results, the previous v1 design left the failed transitions' contract nonces unbumped, so identical bytes could be re-broadcast forever (CheckTx only sees committed state). Atomic-batch (drop everything) was an option but unnecessarily discards successful work. Approach: at the per-transition aggregation site in v1, after each per-transition handler returns, inject a `BumpIdentityDataContractNonce` action when the result is errors-only (data:None). This is a ~10-line change in two places (document and token aggregation branches) and centralises what would otherwise be 12+ inline bump emissions across the per-transition handlers. Why bumping only the failed transitions is sufficient: CheckTx's `validate_identity_contract_nonces_v0` iterates *every* transition's nonce against committed state and rejects on the first failure. Once the failing transition's nonce is consumed, re-broadcasting identical bytes fails CheckTx — even though the successful transitions' nonces remain unchanged. The user retries with a fresh nonce for the failed transition; the previously successful transitions stay untouched. Composes with the existing v1 strict aggregators (`flatten` / `merge_many`): after injection, every per-transition failure carries data:Some(bump), so the strict aggregators only return data:None in edge cases (empty batch). The all-failed case becomes `PaidConsensusError` with all bumps applied — closing the same replay loop that affected mainnet 35C0…313C. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ort batches" This reverts commit fdecf17.
…atch transformer Per review feedback on PR #3616: rather than versioning the ~1100-line batch transformer to switch aggregator behavior at PROTOCOL_VERSION_12, version `ConsensusValidationResult::flatten` / `merge_many` themselves. The semantic change for issue #2867 is at the aggregator layer (return `data: None` when no input contributed any data, instead of the legacy `Some(empty_vec)`), so the version gate now lives where the actual behavior change is — the transformer is single-version and just calls the facade with `platform_version`. - New `validation_result/{mod.rs,v0.rs,v1.rs}` — facade dispatches via `platform_version.dpp.validation.validation_result.{flatten, merge_many}` - `DPP_VALIDATION_VERSIONS_V3` (PROTOCOL_VERSION_12) bumps both fields to 1 - `transformer/v1/mod.rs` deleted; `batch/mod.rs` dispatcher reverted - Legacy `_or_empty_vec` aggregators removed (only the v0 internal module retains that behavior, gated by platform_version) - Net: +414 / −743 vs the previous +1700 / −44 shape of this PR Also adds a TODO at `system_limits/v1.rs::max_transitions_in_documents_batch` (currently 1) noting the broader batch-pipeline issues that must be fixed before lifting the cap: non-atomic application of partial-success batches, and unclear nonce-bump semantics for mixed success/failure batches (see issue #2867 and PR #3608).
…ebase convention
The previous commit landed v0/v1 as flat sibling files
(`validation_result/v{0,1}.rs`). The codebase convention for versioned
methods is `<method>/v{N}/mod.rs` (matching e.g. drive-abci's
`transform_into_action/v0/mod.rs`). Restructure to:
validation_result/
mod.rs — facade dispatch on platform_version
flatten/
mod.rs — declares v0/v1 submodules
v0/mod.rs — flatten_v0 (legacy Some(empty_vec))
v1/mod.rs — flatten_v1 (canonical None-on-empty)
merge_many/
mod.rs
v0/mod.rs — merge_many_v0
v1/mod.rs — merge_many_v1
No behavior change. The 61 unit tests in `validation_result/mod.rs`
remain green; only the function paths they reference change
(`flatten::v1::flatten_v1`, etc.).
…rsion modules The previous commit landed v0/v1-specific tests in `validation_result/mod.rs::tests` alongside facade dispatch tests, which duplicated coverage (the facade tests already exercise v0/v1 behavior through dispatch). Move per-version behavior tests into their own modules: flatten/v0/mod.rs::tests — 3 tests for legacy Some(empty_vec) flatten/v1/mod.rs::tests — 5 tests for canonical None-on-empty merge_many/v0/mod.rs::tests — 3 tests for legacy Some(empty_vec) merge_many/v1/mod.rs::tests — 4 tests for canonical None-on-empty mod.rs::tests — facade dispatch tests + struct/method tests Same 61 tests, same coverage, no duplication.
QuantumExplorer
previously approved these changes
May 7, 2026
…mer + tests
Reframe the change from a mainnet duplicate-tx narrative to its actual
purpose: the user pays for the per-transition validation work that ran
before a failure (document fetch + ownership / revision check), instead
of getting it for free because the failure path emitted no action.
- Drop the dead `let result = ConsensusValidationResult::new();` and
`if result.is_valid() { ... } else { Ok(result) }` tails in Replace,
Transfer, UpdatePrice and Purchase arms — `result` is never modified
after the per-path failure handlers were rewritten to return early.
- One short doc on `transform_document_transition_v0` explaining why
every per-transition failure emits a bump (charge for the work, advance
the nonce); strip per-site narrative comments that pointed at issue
#2867 / mainnet 35C0…313C.
- Slim the regression test doc: keep what it pins, drop the mainnet
escalation timeline.
- Slim the fee-change comments in replacement / nft / transfer to a
one-liner ("covers the fetch + check that ran before the failure").
- Drop the mainnet 35C0 decoder pin in
`packages/rs-dpp/src/state_transition/serialization.rs` — it was a
one-off debug artifact, not a behavior pin for this PR.
The architectural mainnet fix (return `data: None` from aggregators so
empty-action batches become UnpaidConsensusError → tx removed from
block) lives in PR #3616. This PR is the complementary "user pays for
advanced-structure validation work" layer for paths that DO produce a
real per-transition error inside an otherwise-valid batch.
6 tasks
…aired tests Address PR #3608 review comments from QuantumExplorer. - The previous commit emitted bump actions on every per-transition failure unconditionally, which would diverge PROTOCOL_VERSION_11 chain replay (the user-paid fees and stored contract nonce both shifted). Gate the bump emission on a new `failed_per_transition_action: FeatureVersion` field on `DriveAbciDocumentsStateTransitionValidationVersions`: - v0 (PROTOCOL_VERSION_11 and below): errors-only, no action data — preserves chain history bit-for-bit. - v1 (PROTOCOL_VERSION_12+, set in DRIVE_ABCI_VALIDATION_VERSIONS_V8): emit `BumpIdentityDataContractNonce` action. - Factor the 13 per-transition failure sites in `transform_document_transition` into a single `Self::failed_per_transition_action(...)` helper that dispatches on the new field. - Add a function-level doc on `transform_document_transition_v0` noting that the `0` user_fee_increase passed into each `BumpIdentityDataContractNonceAction::from_borrowed_document_base_transition` call is overridden by the outer Documents Batch's `user_fee_increase` when the per-transition action rolls up into the `BatchTransitionAction`, so any per-site value would be discarded. - Refactor three sibling tests into paired-version tests so the v11 baseline fees stay pinned alongside the v12 post-fix fees: - test_document_replace_on_document_type_that_is_not_mutable + _protocol_version_11 (445700 / 41880) - test_document_transfer_that_does_not_yet_exist + _protocol_version_11 (517400 / 36200) - test_document_set_price_on_not_owned_document + _protocol_version_11 (571240 / 36200)
…mp-nonce-on-failed-batch-document-transitions
…failures (issue #2867) Two pre-existing NFT tests asserted PaidConsensusError + invalid_paid_count=1 for single-transition batches whose lone Purchase fails on DocumentNotForSaleError or DocumentIncorrectPurchasePriceError. With the v1 `flatten` / `merge_many` aggregators introduced in this PR (PROTOCOL_VERSION_12+), an all-failed single-transition batch now returns `data: None` instead of the legacy `Some(empty_vec)`. The result flows down the unpaid path: `UnpaidConsensusError` and the tx is removed from the block by prepare_proposal. Update the assertions to reflect that. Note that PR #3608 (separate, pending) restores Paid behavior for these specific paths by emitting a BumpIdentityDataContractNonce action so the user pays for the validation work — at which point these tests will need to flip back to PaidConsensusError. For now this PR alone produces UnpaidConsensus for these cases, which matches v3.1-dev's current state without #3608's bump emission.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/nft.rs (1)
2853-2872: ⚡ Quick winConsider adding v11 regression tests for the two other updated scenarios.
The
not_owned_documentscenario now has a v11-pinned regression test, buttest_document_set_price_and_try_purchase_at_different_amount(updated at line 1856) andtest_document_set_price_and_purchase_then_try_buy_back(updated at line 2367) have no v11 counterparts. The helper pattern already established here makes it straightforward to add them, following the samerun_…_at_protocol_version+ two-entry-point idiom.🤖 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/nft.rs` around lines 2853 - 2872, Add v11-pinned regression tests for the two other updated scenarios by following the existing pattern: create two #[tokio::test] functions that call run_document_set_price_and_try_purchase_at_different_amount_at_protocol_version(...) and run_document_set_price_and_purchase_then_try_buy_back_at_protocol_version(...), each invoked twice like the example (first with PlatformVersion::latest().protocol_version, true, 0 to represent the architectural-fix active path, and second with 11, false, 36200 to pin the historical buggy v11 behavior/fee); name the tests similarly to test_document_set_price_and_try_purchase_at_different_amount_protocol_version_11 and test_document_set_price_and_purchase_then_try_buy_back_protocol_version_11 and mirror the call/await pattern used in test_document_set_price_on_not_owned_document_protocol_version_11.
🤖 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.
Inline comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/nft.rs`:
- Around line 1850-1864: The test
test_document_set_price_and_purchase_from_ones_self must be updated to match
v12+ aggregator behavior: change the assertions to expect an
UnpaidConsensusError and invalid_unpaid_count() == 1 (and invalid_paid_count()
== 0) when using PlatformVersion::latest() (v12+), mirroring the fixes in
test_document_set_price_and_try_purchase_at_different_amount and
test_document_set_price_and_purchase_then_try_buy_back; locate the assertion
block that checks processing_result.invalid_unpaid_count(),
processing_result.invalid_paid_count(), and unwraps
StateTransitionExecutionResult::UnpaidConsensusError and adjust the expectations
or parameterize the test by protocol version to assert the older
PaidConsensusError behavior for pre-v12 versions.
---
Nitpick comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/nft.rs`:
- Around line 2853-2872: Add v11-pinned regression tests for the two other
updated scenarios by following the existing pattern: create two #[tokio::test]
functions that call
run_document_set_price_and_try_purchase_at_different_amount_at_protocol_version(...)
and
run_document_set_price_and_purchase_then_try_buy_back_at_protocol_version(...),
each invoked twice like the example (first with
PlatformVersion::latest().protocol_version, true, 0 to represent the
architectural-fix active path, and second with 11, false, 36200 to pin the
historical buggy v11 behavior/fee); name the tests similarly to
test_document_set_price_and_try_purchase_at_different_amount_protocol_version_11
and test_document_set_price_and_purchase_then_try_buy_back_protocol_version_11
and mirror the call/await pattern used in
test_document_set_price_on_not_owned_document_protocol_version_11.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a377aff-4496-4cbe-9133-8d901f82560a
📒 Files selected for processing (7)
packages/rs-dpp/src/validation/validation_result/flatten/v0/mod.rspackages/rs-dpp/src/validation/validation_result/flatten/v1/mod.rspackages/rs-dpp/src/validation/validation_result/merge_many/v0/mod.rspackages/rs-dpp/src/validation/validation_result/merge_many/v1/mod.rspackages/rs-dpp/src/validation/validation_result/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/state/v0/fetch_documents.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/nft.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/state/v0/fetch_documents.rs
- packages/rs-dpp/src/validation/validation_result/flatten/v1/mod.rs
- packages/rs-dpp/src/validation/validation_result/merge_many/v0/mod.rs
- packages/rs-dpp/src/validation/validation_result/merge_many/v1/mod.rs
- packages/rs-dpp/src/validation/validation_result/flatten/v0/mod.rs
- packages/rs-dpp/src/validation/validation_result/mod.rs
thepastaclaw
reviewed
May 8, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The branch does not introduce any verified consensus-critical production-code defect at this SHA. The only confirmed issues are in the updated NFT regression tests: they now cover the v12+ UnpaidConsensusError shape, but they stop pinning the legacy v11 branch and they no longer assert the zero-processing-fee invariant that is part of the fix’s consensus contract.
Reviewed commit: 583bd6d
🟡 2 suggestion(s)
1 additional finding
🟡 suggestion: The updated unpaid-path assertions no longer verify that the all-failed batch charges zero processing fee
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/nft.rs (lines 2361-2377)
For these single-transition purchase failures, the architectural fix is not just the error variant change. The strict aggregation path returns no batched action, which means the batch does no work and processing_result.aggregated_fees().processing_fee must stay 0. Both updated tests now check only invalid_unpaid_count, invalid_paid_count, and the consensus error string. A regression that still returns UnpaidConsensusError but accidentally keeps the old non-zero fee would pass these tests. The protocol-pinned helpers added elsewhere in this PR explicitly assert the fee invariant and these cases should do the same.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/nft.rs`:
- [SUGGESTION] lines 1850-1864: The purchase-failure regressions are only pinned to `PlatformVersion::latest()`, so the v11 branch is no longer covered
These two updated tests now assert only the v12+ `UnpaidConsensusError` behavior via `PlatformVersion::latest()`. The underlying aggregator change is explicitly versioned: v11 preserves the legacy `PaidConsensusError` path, while v12+ switches to `UnpaidConsensusError`. This file already added a paired helper for the ownership-mismatch case, and adjacent batch tests in `replacement.rs` and `transfer.rs` use the same pattern to lock down both protocol branches. Leaving these purchase-failure scenarios on `latest()` means the historical v11 behavior can regress without test coverage, and the assertions will silently change meaning again when `latest()` advances.
- [SUGGESTION] lines 2361-2377: The updated unpaid-path assertions no longer verify that the all-failed batch charges zero processing fee
For these single-transition purchase failures, the architectural fix is not just the error variant change. The strict aggregation path returns no batched action, which means the batch does no work and `processing_result.aggregated_fees().processing_fee` must stay `0`. Both updated tests now check only `invalid_unpaid_count`, `invalid_paid_count`, and the consensus error string. A regression that still returns `UnpaidConsensusError` but accidentally keeps the old non-zero fee would pass these tests. The protocol-pinned helpers added elsewhere in this PR explicitly assert the fee invariant and these cases should do the same.
…led-batch-document-transitions' into fix/issue-2867-validating-state-transition-for-free # Conflicts: # packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/nft.rs # packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/replacement.rs # packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/transfer.rs
shumkov
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/transfer.rs (1)
1292-1297: ⚡ Quick winPin this regression to protocol 12 instead of
latest().This assertion is guarding the post-fork fee shape, but
PlatformVersion::latest().protocol_versionwill drift on the next protocol bump and can start failing for unrelated fee-table changes. Pinning this wrapper to12keeps the test aligned with the hard-fork boundary this PR is protecting.Suggested change
async fn test_document_transfer_that_does_not_yet_exist() { - run_document_transfer_that_does_not_yet_exist_at_protocol_version( - PlatformVersion::latest().protocol_version, - 517400, - ) + run_document_transfer_that_does_not_yet_exist_at_protocol_version(12, 517400) .await; }🤖 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/transfer.rs` around lines 1292 - 1297, The test currently calls run_document_transfer_that_does_not_yet_exist_at_protocol_version using PlatformVersion::latest().protocol_version which will drift; change the protocol argument to the hard-coded value 12 so the wrapper test test_document_transfer_that_does_not_yet_exist pins to the intended post-fork boundary. Locate the call in test_document_transfer_that_does_not_yet_exist and replace PlatformVersion::latest().protocol_version with 12 while leaving the other numeric parameter (517400) unchanged.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/nft.rs (1)
2844-2860: ⚡ Quick winAdd a replay regression on the purchase path too.
These wrappers pin the v11/v12 fee split for the ownership-mismatch
UpdatePricecase, but issue#2867was triggered by all-failed purchase batches. Right now this file still stops at fee/classification assertions, so a regression that makes failed purchase bytes replayable again could slip through whilereplacement.rsstays green on the Replace path. Please mirror the new CheckTx replay test here for a purchase failure such asdifferent_amountorpurchase_from_ones_self.🤖 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/nft.rs` around lines 2844 - 2860, Add a mirrored CheckTx replay regression for the purchase failure path: create a new test runner (e.g., run_document_purchase_different_amount_at_protocol_version) and two async tests mirroring test_document_set_price_on_not_owned_document and test_document_set_price_on_not_owned_document_protocol_version_11 that call this runner for the current latest protocol and for protocol 11; the runner should reproduce the same replay assertions as the new CheckTx replay test but for a failed purchase case such as "different_amount" or "purchase_from_one_self" so that all-failed purchase batches are covered as well. Ensure the new tests follow the same naming convention and parameterization as the existing functions (test_document_set_price_on_not_owned_document*, run_document_set_price_on_not_owned_document_at_protocol_version) so they pin the fee split for v11/v12 the same way.
🤖 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/nft.rs`:
- Around line 2844-2860: Add a mirrored CheckTx replay regression for the
purchase failure path: create a new test runner (e.g.,
run_document_purchase_different_amount_at_protocol_version) and two async tests
mirroring test_document_set_price_on_not_owned_document and
test_document_set_price_on_not_owned_document_protocol_version_11 that call this
runner for the current latest protocol and for protocol 11; the runner should
reproduce the same replay assertions as the new CheckTx replay test but for a
failed purchase case such as "different_amount" or "purchase_from_one_self" so
that all-failed purchase batches are covered as well. Ensure the new tests
follow the same naming convention and parameterization as the existing functions
(test_document_set_price_on_not_owned_document*,
run_document_set_price_on_not_owned_document_at_protocol_version) so they pin
the fee split for v11/v12 the same way.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/transfer.rs`:
- Around line 1292-1297: The test currently calls
run_document_transfer_that_does_not_yet_exist_at_protocol_version using
PlatformVersion::latest().protocol_version which will drift; change the protocol
argument to the hard-coded value 12 so the wrapper test
test_document_transfer_that_does_not_yet_exist pins to the intended post-fork
boundary. Locate the call in test_document_transfer_that_does_not_yet_exist and
replace PlatformVersion::latest().protocol_version with 12 while leaving the
other numeric parameter (517400) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e3cacdc-8e04-456c-a717-53d86af4f035
📒 Files selected for processing (13)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/nft.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/replacement.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/transfer.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/v0/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v1.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v2.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v3.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v4.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v5.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v6.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v7.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v8.rs
✅ Files skipped from review due to trivial changes (1)
- packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v7.rs
shumkov
changed the title
…verage Address @thepastaclaw's review on PR #3616: the purchase-failure regressions were only pinned to PlatformVersion::latest(), so the v11 chain-history branch was not covered. Pair them following the same pattern as the ownership-mismatch / transfer / replace tests: test_document_set_price_and_try_purchase_at_different_amount + ..._protocol_version_11 test_document_set_price_and_purchase_then_try_buy_back + ..._protocol_version_11 Both versions land as PaidConsensusError (legacy v0 aggregator wraps the empty per-tx action as Some(empty_vec) on v11; v1 aggregator + bump emission produces a non-empty action on v12). The error-string assertion is identical across versions; the helper takes platform_version and runs the full scenario under that protocol.
thepastaclaw
suggested changes
May 9, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified the Codex blocking finding against the worktree at 964df5b: the pre-PR DocumentTransition::Replace branch (commit d448005) explicitly emitted a BumpIdentityDataContractNonceAction on find_replaced_document_v0 failure, but after the refactor that path delegates to failed_per_transition_action, whose v0 arm returns errors-only — so v11 no longer applies the bump that the historical chain applied for replace-of-missing-document. This is a real consensus-history regression and the new regression test only runs at the latest protocol version. The remaining findings are doc, architecture, and test-coverage suggestions, with one significant API-surface concern in rs-dpp.
Reviewed commit: 964df5b
🔴 1 blocking | 🟡 5 suggestion(s) | 💬 3 nitpick(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-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/v0/mod.rs`:
- [BLOCKING] lines 699-710: v11 Replace-on-missing-document loses the legacy bump action — historical chain regression
Verified against the pre-fix-series commit `d448005f9` (the parent of this PR's branch): the `DocumentTransition::Replace` arm previously had a dedicated special case for `find_replaced_document_v0` failure that synthesized `BumpIdentityDataContractNonceAction` and returned `new_with_data_and_errors(...)`. Under PROTOCOL_VERSION_11 the per-tx aggregator (`flatten_v0`/`merge_many_v0`) lifts this to `Some(vec![BumpAction])` and the bump action advances the contract nonce in storage as part of the paid execution.
After this PR (transformer/v0/mod.rs:703-709) that branch delegates unconditionally to `Self::failed_per_transition_action(...)`, whose v0 arm at line 1066 returns `ConsensusValidationResult::new_with_errors(errors)` — i.e. `data: None`, no bump action. Under v11 the legacy `flatten_v0` then lifts to `Some(vec![])`. The downstream batch action is therefore an empty `BatchTransitionAction` with no transitions, and no `BumpIdentityDataContractNonce` is applied. The contract nonce that the historical v11 chain advanced is no longer advanced under the new code. This is a bit-for-bit divergence from PROTOCOL_VERSION_11 history (changes both the state root and any replay-protection guarantee for that path) — exactly the property the PR description claims is preserved.
The regression test `test_document_replace_that_does_not_yet_exist` only runs at `PlatformVersion::latest()` (replacement.rs:1117) so it does not exercise the v11 path; the only v11-paired sibling in replacement.rs is the non-mutable-document-type test at line 683, which goes through a different failure site.
Fix: gate the bump emission inside the Replace branch on the `failed_per_transition_action` version field so v0 keeps the historical inline `BumpIdentityDataContractNonceAction` and v1+ uses the unified helper. Add a paired `_protocol_version_11` sibling for the missing-document case to lock the historical fee/nonce-advance behavior in.
- [SUGGESTION] lines 290-520: `*_v0` transformer functions now contain internal version dispatch
By codebase convention a `_v0` function implements v0 behavior only; protocol-bumped behavior should live in `_v1`. After this PR `try_into_action_v0`, `transform_document_transitions_within_contract_v0`, `transform_document_transitions_within_document_type_v0`, and `transform_document_transition_v0` keep the `_v0` suffix while internally dispatching on `dpp.validation.validation_result.{flatten, merge_many}` and on `failed_per_transition_action`. Functionally fine because the sub-version fields gate the behavior, but the next protocol bump touching this path will accumulate more `match platform_version.…` arms inside the `_v0` body or have to retrofit a v0/v1 split. Rename to `_v1` (or add a doc comment explaining why the v0 suffix is being retained despite the internal dispatch) so future contributors don't replicate the pattern by accident.
In `packages/rs-platform-version/src/version/system_limits/v1.rs`:
- [SUGGESTION] lines 7-19: TODO references PR #3608 which is unrelated to this fix series
`git log --grep=#3608` returns nothing relevant; the fix-series commits in this branch reference issue #2867 only (e.g. `251124817`, `5cdc306ab`, `99cc0ab45`, `be4a53837`). The TODO ends with "see issue #2867 and PR #3608", which will lead reviewers chasing the comment to an unrelated change. Either drop the PR reference or replace it with the correct ticket / PR for the broader batch-pipeline atomicity work.
In `packages/rs-dpp/src/validation/validation_result/mod.rs`:
- [SUGGESTION] lines 54-98: Public ValidationResult::flatten/merge_many signature change is breaking for downstream rs-dpp consumers
Both `ValidationResult::flatten` and `ValidationResult::merge_many` changed from infallible `pub fn ... -> ValidationResult<...>` to fallible `pub fn ... -> Result<ValidationResult<...>, ProtocolError>` with a required `&PlatformVersion`. They are exported `pub` on a type that is part of `rs-dpp`'s public API and consumed by `wasm-dpp` and external SDKs. All in-tree call sites are migrated, but external consumers will fail to compile until they thread a `PlatformVersion` and `?`. Either gate the new behavior behind a separate method name (e.g. `flatten_versioned`) to keep the existing infallible API, or call out the breakage explicitly in the rs-dpp changelog / release notes.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/replacement.rs`:
- [SUGGESTION] lines 712-723: Replay-rejection regression test only covers Replace's revision-mismatch path
`replayed_failed_replace_with_consumed_nonce_must_be_rejected_at_check_tx` locks in the "failed Replace consumes the contract nonce" invariant for `check_revision_is_bumped_by_one_during_replace_v0`, but the PR adds bump emission to 13 distinct per-transition failure sites (Replace ×3, Transfer ×3, UpdatePrice ×3, Purchase ×4). The other 12 sites are exercised only via fee-baseline assertions (e.g. 345700, 571240, 517400) that confirm a non-zero fee was charged but do not confirm the nonce actually advanced and a replay would be rejected. A lightweight extension that runs the same nonce-replay invariant against a Transfer-of-missing-doc and a UpdatePrice-on-not-owned-doc would prevent a future refactor from silently skipping the bump at one of those sites.
In `packages/rs-dpp/src/validation/validation_result/flatten/v1/mod.rs`:
- [SUGGESTION] lines 21-41: `flatten_v1`/`merge_many_v1` collapse `Some(empty_vec)` inputs to `data: None`
v1 keys on `aggregate_data.is_empty()` to decide between `data: None` and `data: Some(...)`, which collapses two distinct caller-side intents: (a) every input had `data: None` (truly no work) and (b) every input had `data: Some(empty_vec)` (caller signaled validated-but-empty). For the documents-batch path this is currently safe — every per-transition handler under PROTOCOL_VERSION_12 emits at least one action on success and bump on failure — but the invariant the public docstring asserts ("data is None when no input contributed any data") is ambiguous about case (b). If a future caller adds a path that returns `Some(empty_vec)` to mean "matched but produced no actions", `flatten_v1`/`merge_many_v1` will silently collapse it to `None` and route to UnpaidConsensusError downstream. Strengthen the docstring on the dispatcher and v1 modules to state explicitly: "if at least one input has `data: Some(_)`, the aggregate has `data: Some(_)`; v1 cannot distinguish `Some(empty)` inputs from `None` inputs at the aggregate level."
Comment on lines
699
to
710
| DocumentTransition::Replace(document_replace_transition) => { | ||
| let mut result = ConsensusValidationResult::<BatchedTransitionAction>::new(); | ||
|
|
||
| let validation_result = | ||
| Self::find_replaced_document_v0(transition, replaced_documents); | ||
|
|
||
| if !validation_result.is_valid_with_data() { | ||
| // We can set the user fee increase to 0 here because it is decided by the Documents Batch instead | ||
| let bump_action = | ||
| BumpIdentityDataContractNonceAction::from_borrowed_document_base_transition( | ||
| document_replace_transition.base(), | ||
| owner_id, | ||
| 0, | ||
| ); | ||
| let batched_action = | ||
| BatchedTransitionAction::BumpIdentityDataContractNonce(bump_action); | ||
|
|
||
| return Ok(ConsensusValidationResult::new_with_data_and_errors( | ||
| batched_action, | ||
| return Self::failed_per_transition_action( | ||
| document_replace_transition.base(), | ||
| owner_id, | ||
| validation_result.errors, | ||
| )); | ||
| platform_version, | ||
| ); | ||
| } |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: v11 Replace-on-missing-document loses the legacy bump action — historical chain regression
Verified against the pre-fix-series commit d448005f9 (the parent of this PR's branch): the DocumentTransition::Replace arm previously had a dedicated special case for find_replaced_document_v0 failure that synthesized BumpIdentityDataContractNonceAction and returned new_with_data_and_errors(...). Under PROTOCOL_VERSION_11 the per-tx aggregator (flatten_v0/merge_many_v0) lifts this to Some(vec![BumpAction]) and the bump action advances the contract nonce in storage as part of the paid execution.
After this PR (transformer/v0/mod.rs:703-709) that branch delegates unconditionally to Self::failed_per_transition_action(...), whose v0 arm at line 1066 returns ConsensusValidationResult::new_with_errors(errors) — i.e. data: None, no bump action. Under v11 the legacy flatten_v0 then lifts to Some(vec![]). The downstream batch action is therefore an empty BatchTransitionAction with no transitions, and no BumpIdentityDataContractNonce is applied. The contract nonce that the historical v11 chain advanced is no longer advanced under the new code. This is a bit-for-bit divergence from PROTOCOL_VERSION_11 history (changes both the state root and any replay-protection guarantee for that path) — exactly the property the PR description claims is preserved.
The regression test test_document_replace_that_does_not_yet_exist only runs at PlatformVersion::latest() (replacement.rs:1117) so it does not exercise the v11 path; the only v11-paired sibling in replacement.rs is the non-mutable-document-type test at line 683, which goes through a different failure site.
Fix: gate the bump emission inside the Replace branch on the failed_per_transition_action version field so v0 keeps the historical inline BumpIdentityDataContractNonceAction and v1+ uses the unified helper. Add a paired _protocol_version_11 sibling for the missing-document case to lock the historical fee/nonce-advance behavior in.
💡 Suggested change
Suggested change
| DocumentTransition::Replace(document_replace_transition) => { | |
| let mut result = ConsensusValidationResult::<BatchedTransitionAction>::new(); | |
| let validation_result = | |
| Self::find_replaced_document_v0(transition, replaced_documents); | |
| if !validation_result.is_valid_with_data() { | |
| // We can set the user fee increase to 0 here because it is decided by the Documents Batch instead | |
| let bump_action = | |
| BumpIdentityDataContractNonceAction::from_borrowed_document_base_transition( | |
| document_replace_transition.base(), | |
| owner_id, | |
| 0, | |
| ); | |
| let batched_action = | |
| BatchedTransitionAction::BumpIdentityDataContractNonce(bump_action); | |
| return Ok(ConsensusValidationResult::new_with_data_and_errors( | |
| batched_action, | |
| return Self::failed_per_transition_action( | |
| document_replace_transition.base(), | |
| owner_id, | |
| validation_result.errors, | |
| )); | |
| platform_version, | |
| ); | |
| } | |
| DocumentTransition::Replace(document_replace_transition) => { | |
| let validation_result = | |
| Self::find_replaced_document_v0(transition, replaced_documents); | |
| if !validation_result.is_valid_with_data() { | |
| return match platform_version | |
| .drive_abci | |
| .validation_and_processing | |
| .state_transitions | |
| .batch_state_transition | |
| .failed_per_transition_action | |
| { | |
| 0 => { | |
| let bump_action = | |
| BumpIdentityDataContractNonceAction::from_borrowed_document_base_transition( | |
| document_replace_transition.base(), | |
| owner_id, | |
| 0, | |
| ); | |
| Ok(ConsensusValidationResult::new_with_data_and_errors( | |
| BatchedTransitionAction::BumpIdentityDataContractNonce(bump_action), | |
| validation_result.errors, | |
| )) | |
| } | |
| _ => Self::failed_per_transition_action( | |
| document_replace_transition.base(), | |
| owner_id, | |
| validation_result.errors, | |
| platform_version, | |
| ), | |
| }; | |
| } |
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/v0/mod.rs`:
- [BLOCKING] lines 699-710: v11 Replace-on-missing-document loses the legacy bump action — historical chain regression
Verified against the pre-fix-series commit `d448005f9` (the parent of this PR's branch): the `DocumentTransition::Replace` arm previously had a dedicated special case for `find_replaced_document_v0` failure that synthesized `BumpIdentityDataContractNonceAction` and returned `new_with_data_and_errors(...)`. Under PROTOCOL_VERSION_11 the per-tx aggregator (`flatten_v0`/`merge_many_v0`) lifts this to `Some(vec![BumpAction])` and the bump action advances the contract nonce in storage as part of the paid execution.
After this PR (transformer/v0/mod.rs:703-709) that branch delegates unconditionally to `Self::failed_per_transition_action(...)`, whose v0 arm at line 1066 returns `ConsensusValidationResult::new_with_errors(errors)` — i.e. `data: None`, no bump action. Under v11 the legacy `flatten_v0` then lifts to `Some(vec![])`. The downstream batch action is therefore an empty `BatchTransitionAction` with no transitions, and no `BumpIdentityDataContractNonce` is applied. The contract nonce that the historical v11 chain advanced is no longer advanced under the new code. This is a bit-for-bit divergence from PROTOCOL_VERSION_11 history (changes both the state root and any replay-protection guarantee for that path) — exactly the property the PR description claims is preserved.
The regression test `test_document_replace_that_does_not_yet_exist` only runs at `PlatformVersion::latest()` (replacement.rs:1117) so it does not exercise the v11 path; the only v11-paired sibling in replacement.rs is the non-mutable-document-type test at line 683, which goes through a different failure site.
Fix: gate the bump emission inside the Replace branch on the `failed_per_transition_action` version field so v0 keeps the historical inline `BumpIdentityDataContractNonceAction` and v1+ uses the unified helper. Add a paired `_protocol_version_11` sibling for the missing-document case to lock the historical fee/nonce-advance behavior in.
Comment on lines
+2716
to
+2724
| /// Same scenario at PROTOCOL_VERSION_11 (legacy bump-only fee) and | ||
| /// PROTOCOL_VERSION_12 (fee covers the fetch + ownership-mismatch | ||
| /// validation work that ran before the failure). Both versions land as | ||
| /// PaidConsensusError because the bump-emission patch makes the per-tx | ||
| /// failure produce a non-empty action on every version. | ||
| async fn run_document_set_price_on_not_owned_document_at_protocol_version( | ||
| protocol_version: dpp::version::ProtocolVersion, | ||
| expected_processing_fee: dpp::fee::Credits, | ||
| ) { |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Helper doc comment misattributes why v11 lands as PaidConsensusError
The doc comment on run_document_set_price_on_not_owned_document_at_protocol_version claims: "Both versions land as PaidConsensusError because the bump-emission patch makes the per-tx failure produce a non-empty action on every version." Verified at transformer/v0/mod.rs:1058-1066: failed_per_transition_action only emits a bump action when the new failed_per_transition_action version field is 1 (DRIVE_ABCI_VALIDATION_VERSIONS_V8 / PROTOCOL_VERSION_12). Under v11 the helper returns new_with_errors(errors) with data: None, so the v11 case lands as paid via flatten_v0/merge_many_v0's legacy Some(empty_vec) lifting — the bug this PR is preserving for chain reproducibility — not via bump emission. Future maintainers reading this comment will get the wrong mental model; consider rewording to "v11 lands as paid via legacy aggregator lifting; v12 lands as paid because the per-tx failure now emits a BumpIdentityDataContractNonce action."
source: ['claude']
Comment on lines
+7
to
+19
| // TODO: this is currently capped at 1 because the batch state-transition | ||
| // pipeline has known correctness issues with multi-transition batches: | ||
| // - It is not atomic: when one transition errors, earlier successful | ||
| // transitions inside the same batch are still applied to state. | ||
| // - Nonce-bump semantics for mixed success/failure batches are not | ||
| // well-defined: it is unclear whether to bump the nonce for the | ||
| // 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 and PR #3608). | ||
| // 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 | ||
| // those bugs to mainnet traffic. |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: TODO references PR #3608 which is unrelated to this fix series
git log --grep=#3608 returns nothing relevant; the fix-series commits in this branch reference issue #2867 only (e.g. 251124817, 5cdc306ab, 99cc0ab45, be4a53837). The TODO ends with "see issue #2867 and PR #3608", which will lead reviewers chasing the comment to an unrelated change. Either drop the PR reference or replace it with the correct ticket / PR for the broader batch-pipeline atomicity work.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-version/src/version/system_limits/v1.rs`:
- [SUGGESTION] lines 7-19: TODO references PR #3608 which is unrelated to this fix series
`git log --grep=#3608` returns nothing relevant; the fix-series commits in this branch reference issue #2867 only (e.g. `251124817`, `5cdc306ab`, `99cc0ab45`, `be4a53837`). The TODO ends with "see issue #2867 and PR #3608", which will lead reviewers chasing the comment to an unrelated change. Either drop the PR reference or replace it with the correct ticket / PR for the broader batch-pipeline atomicity work.
Comment on lines
54
to
98
| pub fn flatten<I: IntoIterator<Item = ValidationResult<Vec<TData>, E>>>( | ||
| items: I, | ||
| ) -> ValidationResult<Vec<TData>, E> { | ||
| let mut aggregate_errors = vec![]; | ||
| let mut aggregate_data = vec![]; | ||
| items.into_iter().for_each(|single_validation_result| { | ||
| let ValidationResult { mut errors, data } = single_validation_result; | ||
| aggregate_errors.append(&mut errors); | ||
| if let Some(mut data) = data { | ||
| aggregate_data.append(&mut data); | ||
| } | ||
| }); | ||
| ValidationResult::new_with_data_and_errors(aggregate_data, aggregate_errors) | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<ValidationResult<Vec<TData>, E>, ProtocolError> { | ||
| match platform_version.dpp.validation.validation_result.flatten { | ||
| 0 => Ok(flatten::v0::flatten_v0(items)), | ||
| 1 => Ok(flatten::v1::flatten_v1(items)), | ||
| version => Err(ProtocolError::UnknownVersionMismatch { | ||
| method: "ValidationResult::flatten".to_string(), | ||
| known_versions: vec![0, 1], | ||
| received: version, | ||
| }), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl<TData: Clone, E: Debug> ValidationResult<TData, E> { | ||
| /// Aggregate a list of `ValidationResult<TData, E>` into a | ||
| /// `ValidationResult<Vec<TData>, E>`. Dispatches to the version selected | ||
| /// by `platform_version`: | ||
| /// | ||
| /// - **v0** (`PROTOCOL_VERSION_11` and below): always returns | ||
| /// `data: Some(Vec<...>)`, including `Some(empty_vec)` when no input | ||
| /// contributed any data. Preserved for chain reproducibility. | ||
| /// - **v1** (`PROTOCOL_VERSION_12`+): returns `data: None` when no input | ||
| /// contributed any data. See [`flatten`] for the invariant this | ||
| /// restores. | ||
| /// | ||
| /// See issue #2867 for context on the v0 → v1 change. | ||
| /// | ||
| /// [`flatten`]: ValidationResult::flatten | ||
| pub fn merge_many<I: IntoIterator<Item = ValidationResult<TData, E>>>( | ||
| items: I, | ||
| ) -> ValidationResult<Vec<TData>, E> { | ||
| let mut aggregate_errors = vec![]; | ||
| let mut aggregate_data = vec![]; | ||
| items.into_iter().for_each(|single_validation_result| { | ||
| let ValidationResult { mut errors, data } = single_validation_result; | ||
| aggregate_errors.append(&mut errors); | ||
| if let Some(data) = data { | ||
| aggregate_data.push(data); | ||
| } | ||
| }); | ||
| ValidationResult::new_with_data_and_errors(aggregate_data, aggregate_errors) | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<ValidationResult<Vec<TData>, E>, ProtocolError> { | ||
| match platform_version.dpp.validation.validation_result.merge_many { | ||
| 0 => Ok(merge_many::v0::merge_many_v0(items)), | ||
| 1 => Ok(merge_many::v1::merge_many_v1(items)), | ||
| version => Err(ProtocolError::UnknownVersionMismatch { | ||
| method: "ValidationResult::merge_many".to_string(), | ||
| known_versions: vec![0, 1], | ||
| received: version, | ||
| }), | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Public ValidationResult::flatten/merge_many signature change is breaking for downstream rs-dpp consumers
Both ValidationResult::flatten and ValidationResult::merge_many changed from infallible pub fn ... -> ValidationResult<...> to fallible pub fn ... -> Result<ValidationResult<...>, ProtocolError> with a required &PlatformVersion. They are exported pub on a type that is part of rs-dpp's public API and consumed by wasm-dpp and external SDKs. All in-tree call sites are migrated, but external consumers will fail to compile until they thread a PlatformVersion and ?. Either gate the new behavior behind a separate method name (e.g. flatten_versioned) to keep the existing infallible API, or call out the breakage explicitly in the rs-dpp changelog / release notes.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/validation/validation_result/mod.rs`:
- [SUGGESTION] lines 54-98: Public ValidationResult::flatten/merge_many signature change is breaking for downstream rs-dpp consumers
Both `ValidationResult::flatten` and `ValidationResult::merge_many` changed from infallible `pub fn ... -> ValidationResult<...>` to fallible `pub fn ... -> Result<ValidationResult<...>, ProtocolError>` with a required `&PlatformVersion`. They are exported `pub` on a type that is part of `rs-dpp`'s public API and consumed by `wasm-dpp` and external SDKs. All in-tree call sites are migrated, but external consumers will fail to compile until they thread a `PlatformVersion` and `?`. Either gate the new behavior behind a separate method name (e.g. `flatten_versioned`) to keep the existing infallible API, or call out the breakage explicitly in the rs-dpp changelog / release notes.
Comment on lines
290
to
520
| @@ -345,7 +359,8 @@ impl BatchTransitionInternalTransformerV0 for BatchTransition { | |||
| ) | |||
| }) | |||
| .collect::<Result<Vec<ConsensusValidationResult<BatchedTransitionAction>>, Error>>()?; | |||
| let validation_result = ConsensusValidationResult::merge_many(validation_result); | |||
| let validation_result = | |||
| ConsensusValidationResult::merge_many(validation_result, platform_version)?; | |||
| Ok(validation_result) | |||
| } | |||
| fn transform_document_transitions_within_contract_v0( | |||
| @@ -399,7 +414,10 @@ impl BatchTransitionInternalTransformerV0 for BatchTransition { | |||
| }) | |||
| .collect::<Result<Vec<ConsensusValidationResult<Vec<BatchedTransitionAction>>>, Error>>( | |||
| )?; | |||
| Ok(ConsensusValidationResult::flatten(validation_result)) | |||
| Ok(ConsensusValidationResult::flatten( | |||
| validation_result, | |||
| platform_version, | |||
| )?) | |||
| } | |||
|
|
|||
| fn transform_document_transitions_within_document_type_v0( | |||
| @@ -495,7 +513,8 @@ impl BatchTransitionInternalTransformerV0 for BatchTransition { | |||
|
|
|||
| let result = ConsensusValidationResult::merge_many( | |||
| document_transition_actions_validation_result, | |||
| ); | |||
| platform_version, | |||
| )?; | |||
|
|
|||
| if !result.is_valid() { | |||
| return Ok(result); | |||
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: *_v0 transformer functions now contain internal version dispatch
By codebase convention a _v0 function implements v0 behavior only; protocol-bumped behavior should live in _v1. After this PR try_into_action_v0, transform_document_transitions_within_contract_v0, transform_document_transitions_within_document_type_v0, and transform_document_transition_v0 keep the _v0 suffix while internally dispatching on dpp.validation.validation_result.{flatten, merge_many} and on failed_per_transition_action. Functionally fine because the sub-version fields gate the behavior, but the next protocol bump touching this path will accumulate more match platform_version.… arms inside the _v0 body or have to retrofit a v0/v1 split. Rename to _v1 (or add a doc comment explaining why the v0 suffix is being retained despite the internal dispatch) so future contributors don't replicate the pattern by accident.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/v0/mod.rs`:
- [SUGGESTION] lines 290-520: `*_v0` transformer functions now contain internal version dispatch
By codebase convention a `_v0` function implements v0 behavior only; protocol-bumped behavior should live in `_v1`. After this PR `try_into_action_v0`, `transform_document_transitions_within_contract_v0`, `transform_document_transitions_within_document_type_v0`, and `transform_document_transition_v0` keep the `_v0` suffix while internally dispatching on `dpp.validation.validation_result.{flatten, merge_many}` and on `failed_per_transition_action`. Functionally fine because the sub-version fields gate the behavior, but the next protocol bump touching this path will accumulate more `match platform_version.…` arms inside the `_v0` body or have to retrofit a v0/v1 split. Rename to `_v1` (or add a doc comment explaining why the v0 suffix is being retained despite the internal dispatch) so future contributors don't replicate the pattern by accident.
Comment on lines
69
to
76
| }) | ||
| .collect::<Result<Vec<ConsensusValidationResult<Vec<Document>>>, Error>>()?; | ||
|
|
||
| let validation_result = ConsensusValidationResult::flatten(validation_results_of_documents); | ||
| let validation_result = | ||
| ConsensusValidationResult::flatten(validation_results_of_documents, platform_version)?; | ||
|
|
||
| Ok(validation_result) | ||
| } |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Deprecated fetch_documents_for_transitions should be deleted, not retrofitted
fetch_documents_for_transitions is #[deprecated] + #[allow(dead_code)] and has no live callers (fetch_documents_for_transitions_knowing_contract_id_and_document_type_name is also only called from this dead helper). The PR threads platform_version through it and through flatten solely to keep it compiling against the new aggregator signature. Carrying dead consensus-pipeline code with version-dispatching internals invites future confusion — a contributor may reintroduce it without realising it now silently changes behavior across the v11/v12 boundary. Cleaner to delete both helpers and the flatten import from this file.
source: ['claude']
Comment on lines
+712
to
+723
| .set_genesis_state(); | ||
|
|
||
| let mut rng = StdRng::seed_from_u64(437); | ||
|
|
||
| let platform_state = platform.state.load(); | ||
|
|
||
| let (identity, signer, key) = setup_identity(&mut platform, 958, dash_to_credits!(0.5)); | ||
|
|
||
| let dashpay = platform.drive.cache.system_data_contracts.load_dashpay(); | ||
| let dashpay_contract = dashpay.clone(); | ||
|
|
||
| // Use the mutable `profile` doc type — same contract-and-doc-type that |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Replay-rejection regression test only covers Replace's revision-mismatch path
replayed_failed_replace_with_consumed_nonce_must_be_rejected_at_check_tx locks in the "failed Replace consumes the contract nonce" invariant for check_revision_is_bumped_by_one_during_replace_v0, but the PR adds bump emission to 13 distinct per-transition failure sites (Replace ×3, Transfer ×3, UpdatePrice ×3, Purchase ×4). The other 12 sites are exercised only via fee-baseline assertions (e.g. 345700, 571240, 517400) that confirm a non-zero fee was charged but do not confirm the nonce actually advanced and a replay would be rejected. A lightweight extension that runs the same nonce-replay invariant against a Transfer-of-missing-doc and a UpdatePrice-on-not-owned-doc would prevent a future refactor from silently skipping the bump at one of those sites.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/replacement.rs`:
- [SUGGESTION] lines 712-723: Replay-rejection regression test only covers Replace's revision-mismatch path
`replayed_failed_replace_with_consumed_nonce_must_be_rejected_at_check_tx` locks in the "failed Replace consumes the contract nonce" invariant for `check_revision_is_bumped_by_one_during_replace_v0`, but the PR adds bump emission to 13 distinct per-transition failure sites (Replace ×3, Transfer ×3, UpdatePrice ×3, Purchase ×4). The other 12 sites are exercised only via fee-baseline assertions (e.g. 345700, 571240, 517400) that confirm a non-zero fee was charged but do not confirm the nonce actually advanced and a replay would be rejected. A lightweight extension that runs the same nonce-replay invariant against a Transfer-of-missing-doc and a UpdatePrice-on-not-owned-doc would prevent a future refactor from silently skipping the bump at one of those sites.
Comment on lines
+21
to
+41
| ) -> ValidationResult<Vec<TData>, E> | ||
| where | ||
| TData: Clone, | ||
| E: Debug, | ||
| I: IntoIterator<Item = ValidationResult<Vec<TData>, E>>, | ||
| { | ||
| let mut aggregate_errors = vec![]; | ||
| let mut aggregate_data = vec![]; | ||
| items.into_iter().for_each(|single_validation_result| { | ||
| let ValidationResult { mut errors, data } = single_validation_result; | ||
| aggregate_errors.append(&mut errors); | ||
| if let Some(mut data) = data { | ||
| aggregate_data.append(&mut data); | ||
| } | ||
| }); | ||
| if aggregate_data.is_empty() { | ||
| ValidationResult::new_with_errors(aggregate_errors) | ||
| } else { | ||
| ValidationResult::new_with_data_and_errors(aggregate_data, aggregate_errors) | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: flatten_v1/merge_many_v1 collapse Some(empty_vec) inputs to data: None
v1 keys on aggregate_data.is_empty() to decide between data: None and data: Some(...), which collapses two distinct caller-side intents: (a) every input had data: None (truly no work) and (b) every input had data: Some(empty_vec) (caller signaled validated-but-empty). For the documents-batch path this is currently safe — every per-transition handler under PROTOCOL_VERSION_12 emits at least one action on success and bump on failure — but the invariant the public docstring asserts ("data is None when no input contributed any data") is ambiguous about case (b). If a future caller adds a path that returns Some(empty_vec) to mean "matched but produced no actions", flatten_v1/merge_many_v1 will silently collapse it to None and route to UnpaidConsensusError downstream. Strengthen the docstring on the dispatcher and v1 modules to state explicitly: "if at least one input has data: Some(_), the aggregate has data: Some(_); v1 cannot distinguish Some(empty) inputs from None inputs at the aggregate level."
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/validation/validation_result/flatten/v1/mod.rs`:
- [SUGGESTION] lines 21-41: `flatten_v1`/`merge_many_v1` collapse `Some(empty_vec)` inputs to `data: None`
v1 keys on `aggregate_data.is_empty()` to decide between `data: None` and `data: Some(...)`, which collapses two distinct caller-side intents: (a) every input had `data: None` (truly no work) and (b) every input had `data: Some(empty_vec)` (caller signaled validated-but-empty). For the documents-batch path this is currently safe — every per-transition handler under PROTOCOL_VERSION_12 emits at least one action on success and bump on failure — but the invariant the public docstring asserts ("data is None when no input contributed any data") is ambiguous about case (b). If a future caller adds a path that returns `Some(empty_vec)` to mean "matched but produced no actions", `flatten_v1`/`merge_many_v1` will silently collapse it to `None` and route to UnpaidConsensusError downstream. Strengthen the docstring on the dispatcher and v1 modules to state explicitly: "if at least one input has `data: Some(_)`, the aggregate has `data: Some(_)`; v1 cannot distinguish `Some(empty)` inputs from `None` inputs at the aggregate level."
Comment on lines
359
to
365
| ) | ||
| }) | ||
| .collect::<Result<Vec<ConsensusValidationResult<BatchedTransitionAction>>, Error>>()?; | ||
| let validation_result = ConsensusValidationResult::merge_many(validation_result); | ||
| let validation_result = | ||
| ConsensusValidationResult::merge_many(validation_result, platform_version)?; | ||
| Ok(validation_result) | ||
| } |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Token batch path silently affected by the merge_many aggregator bump
transform_token_transitions_within_contract_v0 calls ConsensusValidationResult::merge_many(...) on token transitions, so the v0→v1 aggregator change at PROTOCOL_VERSION_12 also alters how token-only batches with all-failed sub-transitions classify. In practice this is currently safe: every token sub-transformer (e.g. try_from_borrowed_token_burn_transition_with_contract_lookup v0 lines 105–111) emits a BumpIdentityDataContractNonce on base-validation failure, so per-token results always have data: Some(...) and the v1 aggregator never collapses to None. But the PR description and module-level docs frame this as a documents-batch fix without acknowledging the token path is affected, and there is no test exercising an all-failed token batch under v12 to lock in the "tokens always pay" property. A doc note (or a small token-side regression test) would prevent a future change to a token transformer from silently routing failures to UnpaidConsensusError.
source: ['claude']
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Closes #2867 — Duplicated transactions in Tenderdash chain (MAINNET / TESTNET). Mainnet ST hash
35C08D574302D32D7160E603D8159042C8606BE12FD97D952CF5FD40DB57313C(block 361774, idx 1, FAIL) reappeared in a later block past the platform-explorer indexer'sUNIQUE(hash)panic point. The same shape was reported on testnet on 2026-02-05 (90AC…) and earlier (cf28…at blocks 206243 + 216145). The pattern: same exact serialized Documents Batch Replace bytes accepted into multiple blocks.The root cause is two layered bugs in the Documents Batch transformer; this PR fixes both, gated to PROTOCOL_VERSION_12. PROTOCOL_VERSION_11 chain history is preserved bit-for-bit.
Bug 1 — paid empty action: invalid batches synthesised as paid empty
BatchTransitionActionConsensusValidationResult::flattenandmerge_manyaggregators wrapped per-item results asSome(Vec)even when no input contributed any data —Some(empty_vec). Downstream code (process_validation_result_v0:241) keys ondata.is_some()to choose betweenPaidConsensusErrorandUnpaidConsensusError. With the legacy aggregator, an all-failed Documents Batch becamePaidConsensusErrorwith an emptyBatchTransitionActionand no actual drive ops —prepare_proposalthen kept the tx in the block.Bug 2 — free advanced-structure validation: per-transition failure paths emit no action
transform_document_transition_v0's per-transition handler emitted aBumpIdentityDataContractNonceActiononly for one path (find_replaced_document_v0failure onReplace). Every other failure (check_ownership,check_revision,DocumentNotForSaleError,DocumentIncorrectPurchasePriceError, etc.) returned errors-only with no action data. Combined with Bug 1, this meant validators did real work (document fetch + ownership/revision check) but charged 0 for it, and the contract nonce never advanced — the same exact bytes could be re-broadcast and would passCheckTx FirstTimeCheckagain.What was done?
Two complementary fixes, both gated to PROTOCOL_VERSION_12:
Aggregator versioning (
packages/rs-dpp/src/validation/validation_result/):flattenandmerge_manyare dispatched onplatform_version.dpp.validation.validation_result.{flatten, merge_many}. v0 keeps the legacySome(empty_vec)semantics for chain reproducibility; v1 returnsdata: Nonewhen no input contributed, restoring the invariantdata.is_none() ⇔ no work doneon which the paid/unpaid routing depends. Module layout follows the codebase's<method>/v{N}/mod.rsconvention. Per-version tests live next to each implementation;mod.rs::testsonly retains facade dispatch tests.Bump emission on per-transition failure (
packages/rs-drive-abci/.../batch/transformer/v0/mod.rs): all 13 per-transition failure paths in the four mutating-document arms (Replace ×3, Transfer ×3, UpdatePrice ×3, Purchase ×4) now emit aBumpIdentityDataContractNonceActionso the user pays for the validation work that ran (fetch + ownership/revision check) and the contract nonce advances. The emission itself is gated by a newfailed_per_transition_action: FeatureVersionfield onDriveAbciDocumentsStateTransitionValidationVersions; v0 (PROTOCOL_VERSION_11 and below) preserves the legacy errors-only path.Together these two layers split the failure landscape correctly:
PaidConsensusError, user pays for the validation work, nonce advancesdata: None→UnpaidConsensusError→ tx removed from block by prepare_proposaldata: None→UnpaidConsensusError→ tx removedAlso in this PR
max_transitions_in_documents_batch = 1— added a TODO atsystem_limits/v1.rsdocumenting the broader batch-pipeline issues (non-atomic application of partial-success batches, unclear nonce-bump semantics for mixed success/failure batches) that must be reviewed before lifting the cap above 1.let result = ConsensusValidationResult::new()declarations andif result.is_valid() { Ok(action) } else { Ok(result) }tails removed in the four mutating-document arms (the result/merge pattern they implemented is gone now that each per-tx failure returns early via the bump helper).transform_document_transition_v0explaining that the0user_fee_increase passed into eachBumpIdentityDataContractNonceAction::from_borrowed_document_base_transitioncall is overridden by the outer Documents Batch'suser_fee_increasewhen the per-transition action rolls up.How Has This Been Tested?
New regression test at
batch/tests/document/replacement.rs::replayed_failed_replace_with_consumed_nonce_must_be_rejected_at_check_tx:check_revision_is_bumped_by_one_during_replace_v0fails.state_transition_to_execution_event_for_check_txwithFirstTimeCheck.InvalidIdentityNonceError.Paired-version sibling tests — three sibling tests now run on both PROTOCOL_VERSION_11 (legacy bump-only fee) and PROTOCOL_VERSION_12 (fee covers fetch + structural validation) so the v11 chain history stays bit-for-bit reproducible:
test_document_replace_on_document_type_that_is_not_mutable+_protocol_version_11(445700 / 41880)test_document_transfer_that_does_not_yet_exist+_protocol_version_11(517400 / 36200)test_document_set_price_on_not_owned_document+_protocol_version_11(571240 / 36200)Unit tests for the new aggregator submodules (
flatten/v0,flatten/v1,merge_many/v0,merge_many/v1) plus facade dispatch tests invalidation_result/mod.rs.Breaking Changes
Consensus-affecting for
PROTOCOL_VERSION_12(currentv3.1-dev, not yet active on mainnet — mainnet runsPROTOCOL_VERSION_11):identity_contract_nonceadvances on every per-transition failure path now (previously only onfind_replaced_document_v0failure forReplace).UnpaidConsensusErrorand are removed from the block byprepare_proposal, instead of being recorded as a paid emptyBatchTransitionAction.Activation should coincide with the v3.1 hard fork. No action needed before then.
Out of Scope
pshenmic/platform-explorer'sUNIQUE(hash)constraint onstate_transitionsis too strict regardless of this fix. Tenderdash never guaranteed cross-block tx-hash uniqueness — failed-but-paid txs are committed by protocol design. RecommendUNIQUE(block_height, hash)upstream so the indexer no longer panics on legitimate Tenderdash RPC output.Cargo.toml/Cargo.lockand rs-platform-wallet. That gap let an asset-lock fixture regression slip past PR review (since fixed in fix(dpp): use DIP-0002 version 3 in asset-lock tx fixtures #3621). Consider always running Rust workspace tests whenCargo.tomlorCargo.lockis touched.Checklist
Summary by CodeRabbit
Bug Fixes
Tests
Documentation