fix(drive-abci): free advanced structure validation for invalid batch transitions#3608
Closed
fix(drive-abci): free advanced structure validation for invalid batch transitions#3608
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>
Contributor
📝 WalkthroughWalkthroughThis PR adds a new ChangesBatch Document Transition Fee Bump Actions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3608 +/- ##
============================================
- Coverage 88.29% 88.29% -0.01%
============================================
Files 2479 2479
Lines 301659 301710 +51
============================================
+ Hits 266347 266381 +34
- Misses 35312 35329 +17
🚀 New features to boost your workflow:
|
Collaborator
|
✅ Review complete (commit 24437ef) |
![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/transformer/v0/mod.rs (1)
667-758: ⚡ Quick winFactor out the nonce-bump failure return and drop the inert
result.
resultis never mutated here, soif result.is_valid()is always true and theelsebranch is dead. The repeatedBumpIdentityDataContractNonceconstruction across these arms is also the exact pattern that drifted and caused this bug class in the first place. A small helper that builds the bump+errors result would make the control flow clearer and reduce the chance of missing another failure path later.🤖 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/transformer/v0/mod.rs` around lines 667 - 758, The code creates identical BumpIdentityDataContractNonce actions and returns ConsensusValidationResult with errors in several places while keeping an inert local result (ConsensusValidationResult::<BatchedTransitionAction> result) that is never mutated; remove that dead variable and factor the repeated failure-return into a small helper (e.g. build_bump_failure_result or bump_with_errors) that takes the borrowed DocumentBaseTransition (document_replace_transition.base()), owner_id and the validation errors and returns ConsensusValidationResult::new_with_data_and_errors(BatchedTransitionAction::BumpIdentityDataContractNonce(<...>), errors) using BumpIdentityDataContractNonceAction::from_borrowed_document_base_transition; replace each repeated block that constructs the bump action and returns with calls to this helper, and after creating execution_context.add_operation(...) simply return the successful DocumentReplaceTransitionAction instead of checking the unused result.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/replacement.rs (1)
868-880: ⚡ Quick winAssert the exact consumed nonce here.
assert_ne!proves “something changed,” but this regression is really about consuming exactly one contract nonce on the failed replace. If this ever starts double-bumping, this test would still pass while consensus behavior changed.Suggested tightening
- assert_ne!( - post_replace_nonce, post_create_nonce, - "BUG: failed Replace's bump action did not advance the contract \ - nonce. Stored nonce is still {:`#x`} (= post-create value), so the \ - same exact serialized bytes can be replayed forever. \ - Root cause: batch/transformer/v0/mod.rs:712-715 (and 697-700) \ - return Ok(result) with errors-only and no BumpIdentityDataContractNonce \ - action when check_revision_is_bumped_by_one_during_replace_v0 (or \ - check_ownership_of_old_replaced_document_v0) fails — unlike the \ - find_replaced_document_v0 failure path on the same arm (lines \ - 672-686) which does emit the bump.", - post_create_nonce - ); + assert_eq!( + post_replace_nonce, + post_create_nonce + 1, + "failed Replace should consume exactly one identity_contract_nonce" + );🤖 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 868 - 880, The assertion currently only checks that post_replace_nonce differs from post_create_nonce (assert_ne!), but we must assert it was advanced by exactly one; update the test to assert that post_replace_nonce equals post_create_nonce + 1 (use the appropriate increment/ wrapping_add for the nonce type) so the test fails if nonce is not consumed exactly once (refer to the variables post_replace_nonce and post_create_nonce in the replacement test).
🤖 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/replacement.rs`:
- Around line 868-880: The assertion currently only checks that
post_replace_nonce differs from post_create_nonce (assert_ne!), but we must
assert it was advanced by exactly one; update the test to assert that
post_replace_nonce equals post_create_nonce + 1 (use the appropriate increment/
wrapping_add for the nonce type) so the test fails if nonce is not consumed
exactly once (refer to the variables post_replace_nonce and post_create_nonce in
the replacement test).
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/v0/mod.rs`:
- Around line 667-758: The code creates identical BumpIdentityDataContractNonce
actions and returns ConsensusValidationResult with errors in several places
while keeping an inert local result
(ConsensusValidationResult::<BatchedTransitionAction> result) that is never
mutated; remove that dead variable and factor the repeated failure-return into a
small helper (e.g. build_bump_failure_result or bump_with_errors) that takes the
borrowed DocumentBaseTransition (document_replace_transition.base()), owner_id
and the validation errors and returns
ConsensusValidationResult::new_with_data_and_errors(BatchedTransitionAction::BumpIdentityDataContractNonce(<...>),
errors) using
BumpIdentityDataContractNonceAction::from_borrowed_document_base_transition;
replace each repeated block that constructs the bump action and returns with
calls to this helper, and after creating execution_context.add_operation(...)
simply return the successful DocumentReplaceTransitionAction instead of checking
the unused result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71594617-1229-483e-aab0-52132a18b224
📒 Files selected for processing (5)
packages/rs-dpp/src/state_transition/serialization.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/v0/mod.rs
QuantumExplorer
requested changes
May 7, 2026
Member
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
We need to add a test that the state transition that causes a dedup continues to do the same (bad) thing in version 11 and below. Also the fix you did is not versioned which would lead to a chain stall.
| BumpIdentityDataContractNonceAction::from_borrowed_document_base_transition( | ||
| document_replace_transition.base(), | ||
| owner_id, | ||
| 0, |
Member
There was a problem hiding this comment.
This should be the user_fee_increase, not 0
Member
There was a problem hiding this comment.
Nvm, it can be anything, but we should leave a comment saying that it's okay that it's 0 because it's in a batch_action, and the batch_action will override the user fee increase.
| // Transfer find-replaced-document failure path now emits a bump action | ||
| // (previously dropped, leaking nonce-replay risk). Cost covers the | ||
| // fetch+validation work that was already happening. | ||
| assert_eq!(processing_result.aggregated_fees().processing_fee, 517400); |
Member
There was a problem hiding this comment.
We need to add a test to verify that in version 11 the fees stay constant.
| // PROTOCOL_VERSION_12. Issue #2867 fix: the failure path now emits the | ||
| // bump after running the document fetch + validation work, so the fee | ||
| // covers that work — same drive ops, just charged correctly. | ||
| assert_eq!(processing_result.aggregated_fees().processing_fee, 445700); |
Member
There was a problem hiding this comment.
We need to add a test that in version 11 the fees stay the same.
| // UpdatePrice ownership-mismatch failure path now emits a bump action | ||
| // (previously dropped, leaking nonce-replay risk). Cost covers the | ||
| // fetch+validation work that was already happening. | ||
| assert_eq!(processing_result.aggregated_fees().processing_fee, 571240); |
Member
There was a problem hiding this comment.
We need to add a test that in version 11 the fees stay the same.
5 tasks
shumkov
added a commit
that referenced
this pull request
May 7, 2026
…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).
shumkov
changed the title
…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.
…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)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v7.rs (1)
2836-2840:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPin this fee test to protocol 12 instead of
latest().Line 2838 currently binds an exact fee (
571240) toPlatformVersion::latest(). That can become flaky when a newer protocol updates fee accounting. Pinning explicitly to protocol 12 keeps this regression stable and scoped to the intended activation boundary.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, 571240, ) .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-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v7.rs` around lines 2836 - 2840, The test pins an exact fee (571240) to PlatformVersion::latest(), which can break as newer protocols change fees; update the test to explicitly target protocol 12 by replacing PlatformVersion::latest() with the protocol-12 variant (e.g., PlatformVersion::V12 or the equivalent constructor that yields protocol 12) where the fee 571240 is asserted so the fee expectation remains stable and scoped to the intended activation boundary.
🤖 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/replacement.rs`:
- Around line 873-879: The current assertion uses assert_ne!(post_replace_nonce,
post_create_nonce) which allows the nonce to decrease; change it to assert that
post_replace_nonce is strictly greater than post_create_nonce (e.g., use
assert!(post_replace_nonce > post_create_nonce, "...")) so the test enforces
monotonic nonce advancement after the Replace bump action; update the failure
message to reflect that we expect a larger (advanced) nonce and reference the
variables post_replace_nonce and post_create_nonce in the message.
---
Outside diff comments:
In
`@packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v7.rs`:
- Around line 2836-2840: The test pins an exact fee (571240) to
PlatformVersion::latest(), which can break as newer protocols change fees;
update the test to explicitly target protocol 12 by replacing
PlatformVersion::latest() with the protocol-12 variant (e.g.,
PlatformVersion::V12 or the equivalent constructor that yields protocol 12)
where the fee 571240 is asserted so the fee expectation remains stable and
scoped to the intended activation boundary.
🪄 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: 4fdabee4-8c7b-41c0-9bec-dcfede804d50
📒 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/v3.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/v0/mod.rs
Comment on lines
+873
to
+879
| assert_ne!( | ||
| post_replace_nonce, post_create_nonce, | ||
| "failed Replace's bump action did not advance the contract \ | ||
| nonce — stored nonce is still {:#x} (= post-create value), so \ | ||
| the same serialized bytes can be replayed", | ||
| post_create_nonce | ||
| ); |
Contributor
There was a problem hiding this comment.
Strengthen nonce assertion to enforce monotonic advancement.
assert_ne! can pass even if nonce changes in the wrong direction. This test should assert that post-failure nonce is strictly greater than post-create nonce.
Suggested assertion hardening
- assert_ne!(
- post_replace_nonce, post_create_nonce,
- "failed Replace's bump action did not advance the contract \
- nonce — stored nonce is still {:`#x`} (= post-create value), so \
- the same serialized bytes can be replayed",
- post_create_nonce
- );
+ assert!(
+ post_replace_nonce > post_create_nonce,
+ "failed Replace must strictly advance contract nonce: post_create={:`#x`}, post_replace={:`#x`}",
+ post_create_nonce,
+ post_replace_nonce
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert_ne!( | |
| post_replace_nonce, post_create_nonce, | |
| "failed Replace's bump action did not advance the contract \ | |
| nonce — stored nonce is still {:#x} (= post-create value), so \ | |
| the same serialized bytes can be replayed", | |
| post_create_nonce | |
| ); | |
| assert!( | |
| post_replace_nonce > post_create_nonce, | |
| "failed Replace must strictly advance contract nonce: post_create={:`#x`}, post_replace={:`#x`}", | |
| post_create_nonce, | |
| post_replace_nonce | |
| ); |
🤖 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 873 - 879, The current assertion uses
assert_ne!(post_replace_nonce, post_create_nonce) which allows the nonce to
decrease; change it to assert that post_replace_nonce is strictly greater than
post_create_nonce (e.g., use assert!(post_replace_nonce > post_create_nonce,
"...")) so the test enforces monotonic nonce advancement after the Replace bump
action; update the failure message to reflect that we expect a larger (advanced)
nonce and reference the variables post_replace_nonce and post_create_nonce in
the message.
thepastaclaw
reviewed
May 7, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the PR at be4a538372563e2ed75cefbe7f4129675b0b03ad and did not find a correctness or consensus-safety defect in the implementation. The only issue I could confirm is a regression-coverage gap: purchase failures now use the same consensus-visible version gate as other per-transition document failures, but the tests do not pin that behavior across protocol versions.
Reviewed commit: be4a538
🟡 1 suggestion(s)
1 additional finding
🟡 suggestion: Purchase failure paths are not pinned with the same version-specific assertions as the other new failed-per-transition cases
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/nft.rs (lines 1666-1861)
DocumentTransition::Purchase now routes both DocumentNotForSaleError and DocumentIncorrectPurchasePriceError through failed_per_transition_action, so from protocol 12 onward these failures must emit a BumpIdentityDataContractNonce action, charge the higher processing fee, and consume the contract nonce. The existing purchase regression in this file only checks that the wrong-price path becomes a PaidConsensusError with the expected message, and there is no paired protocol-11/protocol-12 assertion for either wrong-price or not-for-sale purchase failures. That leaves the purchase arm less protected than the replace, transfer, and update-price arms, whose tests now pin the version-specific fee behavior and, for replace, nonce consumption semantics. Because both fee accounting and nonce advancement are consensus-visible, this is a real regression-coverage gap.
🤖 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 1666-1861: Purchase failure paths are not pinned with the same version-specific assertions as the other new failed-per-transition cases
`DocumentTransition::Purchase` now routes both `DocumentNotForSaleError` and `DocumentIncorrectPurchasePriceError` through `failed_per_transition_action`, so from protocol 12 onward these failures must emit a `BumpIdentityDataContractNonce` action, charge the higher processing fee, and consume the contract nonce. The existing purchase regression in this file only checks that the wrong-price path becomes a `PaidConsensusError` with the expected message, and there is no paired protocol-11/protocol-12 assertion for either wrong-price or not-for-sale purchase failures. That leaves the purchase arm less protected than the replace, transfer, and update-price arms, whose tests now pin the version-specific fee behavior and, for replace, nonce consumption semantics. Because both fee accounting and nonce advancement are consensus-visible, this is a real regression-coverage gap.
…mp-nonce-on-failed-batch-document-transitions
shumkov
added a commit
that referenced
this pull request
May 8, 2026
…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.
thepastaclaw
reviewed
May 8, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The implementation at 24437ef8b3279d856a39437ccf62f94748b4f7c9 is internally consistent: DocumentTransition::Purchase now goes through the same failed_per_transition_action version gate as the other per-transition document failures, and protocol 12 correctly enables nonce-bumping actions while protocol 11 preserves legacy behavior. The remaining issue is a real regression-coverage gap: the purchase-specific failure tests still do not pin the protocol-11 versus protocol-12 fee and nonce semantics that this PR made consensus-visible.
Reviewed commit: 24437ef
🟡 1 suggestion(s)
1 additional finding
🟡 suggestion: Purchase failure paths are still not pinned across the protocol 11/12 gate
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/nft.rs (lines 1666-2358)
DocumentTransition::Purchase now routes both wrong-price and not-for-sale failures through failed_per_transition_action, and that helper is explicitly versioned: protocol 11 returns errors only, while protocol 12 emits BumpIdentityDataContractNonce, charges for the fetch/validation work that already ran, and consumes the contract nonce. In nft.rs, test_document_set_price_and_try_purchase_at_different_amount and test_document_set_price_and_purchase_then_try_buy_back only run at PlatformVersion::latest() and only assert the PaidConsensusError message. Unlike the replace, transfer, and update-price failure tests added elsewhere in this PR, these purchase-specific branches do not assert the version-specific processing-fee baseline or that replay is prevented once the nonce is consumed. Because fee accounting and nonce advancement are consensus-visible, leaving these purchase paths unpinned makes it easy for a later refactor to silently regress protocol 12 back to the old protocol 11 semantics.
🤖 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 1666-2358: Purchase failure paths are still not pinned across the protocol 11/12 gate
`DocumentTransition::Purchase` now routes both wrong-price and not-for-sale failures through `failed_per_transition_action`, and that helper is explicitly versioned: protocol 11 returns errors only, while protocol 12 emits `BumpIdentityDataContractNonce`, charges for the fetch/validation work that already ran, and consumes the contract nonce. In `nft.rs`, `test_document_set_price_and_try_purchase_at_different_amount` and `test_document_set_price_and_purchase_then_try_buy_back` only run at `PlatformVersion::latest()` and only assert the `PaidConsensusError` message. Unlike the replace, transfer, and update-price failure tests added elsewhere in this PR, these purchase-specific branches do not assert the version-specific processing-fee baseline or that replay is prevented once the nonce is consumed. Because fee accounting and nonce advancement are consensus-visible, leaving these purchase paths unpinned makes it easy for a later refactor to silently regress protocol 12 back to the old protocol 11 semantics.
Collaborator
Author
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
Validators currently do advanced-structure validation work for free on invalid batch document transitions. When a per-transition check fails —
find_replaced_document_v0,check_ownership_of_old_replaced_document_v0,check_revision_is_bumped_by_one_during_replace_v0,DocumentNotForSaleError,DocumentIncorrectPurchasePriceError— the failure path returns errors-only with no action data. The emptyBatchTransitionActionflows up throughflatten/merge_manyand the transition is recorded asPaidConsensusError, but because there's noBumpIdentityDataContractNonceaction, noUpdateIdentityContractNoncedrive op is created. Two consequences:CheckTx FirstTimeCheckagain because the nonce slot is still free.The mainnet duplicate-transaction symptom (issue #2867 —
35C08D…313Creappearing in multiple blocks) is the most visible consequence. The architectural mainnet fix is in #3616 (returndata: Nonefrom the aggregators so all-failed batches becomeUnpaidConsensusErrorand are removed from the block). This PR is the complementary layer: when a transition does land in a block but its per-transition check fails, the user pays for the work that ran.What was done?
Patched all 13 per-transition failure paths in
packages/rs-drive-abci/.../batch/transformer/v0/mod.rs::transform_document_transition_v0to emit aBumpIdentityDataContractNonceAction:find_replaced,check_ownership,check_revisionfind_replaced,check_ownership,check_revisionfind_replaced,check_ownership,check_revisionfind_replaced,DocumentNotForSale,DocumentIncorrectPurchasePrice,check_revisionCleaned up the now-dead
let result = ConsensusValidationResult::new();andif result.is_valid() { ... } else { Ok(result) }tails in those four arms.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_onefails.state_transition_to_execution_event_for_check_txwithFirstTimeCheck.InvalidIdentityNonceError.Three sibling tests had hard-coded fee assertions matching the old (under-charged) numbers; updated:
test_document_replace_on_document_type_that_is_not_mutable:41880→445700test_document_transfer_that_does_not_yet_exist:36200→517400test_document_set_price_on_not_owned_document:36200→571240The new fee covers the fetch + structural check that ran before the failure (same drive ops, just charged correctly).
cargo test -p drive-abci --lib batch::tests::document: 72 passed, 0 failed.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, not just onfind_replaced_document_v0for Replace.Activation should coincide with the
v3.1hard-fork.Checklist
Summary by CodeRabbit
Bug Fixes
Tests