Skip to content

fix(drive-abci): free advanced structure validation for invalid batch transitions #3608

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shumkov wants to merge 5 commits into from
Closed

fix(drive-abci): free advanced structure validation for invalid batch transitions #3608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5cdc306
fix(drive-abci): bump nonce on failed batch document transitions
shumkov May 7, 2026
1d0a02b
refactor(drive-abci): tighten bump-emission framing in batch transfor…
shumkov May 7, 2026
aa47a82
style(drive-abci): cargo fmt for #2867 bump-emission cleanup
shumkov May 7, 2026
be4a538
fix(drive-abci): version-gate per-transition bump emission, add v11 p…
shumkov May 7, 2026
24437ef
Merge remote-tracking branch 'origin/v3.1-dev' into fix/issue-2867-bu…
shumkov May 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 34 additions & 4 deletions ...i/src/execution/validation/state_transition/state_transitions/batch/tests/document/nft.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2652,10 +2652,17 @@ mod nft_tests {
assert_eq!(processing_result.aggregated_fees().processing_fee, 0);
}

#[tokio::test]
async fn test_document_set_price_on_not_owned_document() {
let platform_version = PlatformVersion::latest();
/// Helper for the paired set-price-on-not-owned-document test. Same
/// scenario at PROTOCOL_VERSION_11 (legacy bump-only fee) and
/// PROTOCOL_VERSION_12 (fee covers fetch + validation work).
async fn run_document_set_price_on_not_owned_document_at_protocol_version(
protocol_version: dpp::version::ProtocolVersion,
expected_processing_fee: dpp::fee::Credits,
) {
let platform_version = PlatformVersion::get(protocol_version)
.expect("expected platform version for the requested protocol_version");
let (mut platform, contract) = TestPlatformBuilder::new()
.with_initial_protocol_version(protocol_version)
.build_with_mock_rpc()
.set_initial_state_structure()
.with_crypto_card_game_nft(TradeMode::DirectPurchase);
Expand Down Expand Up @@ -2788,7 +2795,12 @@ mod nft_tests {

assert_eq!(processing_result.valid_count(), 0);

assert_eq!(processing_result.aggregated_fees().processing_fee, 36200);
assert_eq!(
processing_result.aggregated_fees().processing_fee,
expected_processing_fee,
"PROTOCOL_VERSION_{}: processing fee must match the version-specific baseline",
protocol_version,
);

let sender_documents_sql_string =
format!("select * from card where $ownerId == '{}'", identity.id());
Expand Down Expand Up @@ -2818,6 +2830,24 @@ mod nft_tests {
);
}

/// PROTOCOL_VERSION_12+: bump emission charges the user for the fetch +
/// ownership check that ran before the failure.
#[tokio::test]
async fn test_document_set_price_on_not_owned_document() {
run_document_set_price_on_not_owned_document_at_protocol_version(
PlatformVersion::latest().protocol_version,
571240,
)
.await;
}

/// PROTOCOL_VERSION_11: pre-fix bump-only fee. Pinned so v11 chain
/// history stays bit-for-bit reproducible.
#[tokio::test]
async fn test_document_set_price_on_not_owned_document_protocol_version_11() {
run_document_set_price_on_not_owned_document_at_protocol_version(11, 36200).await;
}

#[tokio::test]
async fn test_document_set_price_and_purchase_with_token_costs() {
let platform_version = PlatformVersion::latest();
Expand Down
271 changes: 266 additions & 5 deletions ...ecution/validation/state_transition/state_transitions/batch/tests/document/replacement.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,11 +509,17 @@ mod replacement_tests {
.await;
}

#[tokio::test]
async fn test_document_replace_on_document_type_that_is_not_mutable() {
let platform_version = PlatformVersion::latest();
/// Helper for the paired Replace-on-immutable-doc test. The same scenario
/// is exercised at PROTOCOL_VERSION_11 (legacy bump-only fee) and at
/// PROTOCOL_VERSION_12 (fee covers fetch + validation).
async fn run_document_replace_on_document_type_that_is_not_mutable_at_protocol_version(
protocol_version: dpp::version::ProtocolVersion,
expected_processing_fee: dpp::fee::Credits,
) {
let platform_version = PlatformVersion::get(protocol_version)
.expect("expected platform version for the requested protocol_version");
let mut platform = TestPlatformBuilder::new()
.with_latest_protocol_version()
.with_initial_protocol_version(protocol_version)
.build_with_mock_rpc()
.set_genesis_state();

Expand Down Expand Up @@ -651,7 +657,262 @@ mod replacement_tests {

assert_eq!(processing_result.valid_count(), 0);

assert_eq!(processing_result.aggregated_fees().processing_fee, 41880);
assert_eq!(
processing_result.aggregated_fees().processing_fee,
expected_processing_fee,
"PROTOCOL_VERSION_{}: processing fee must match the version-specific baseline",
protocol_version,
);
}

/// PROTOCOL_VERSION_12+: bump emission charges the user for the fetch +
/// structure validation that ran before the failure.
#[tokio::test]
async fn test_document_replace_on_document_type_that_is_not_mutable() {
run_document_replace_on_document_type_that_is_not_mutable_at_protocol_version(
PlatformVersion::latest().protocol_version,
445700,
)
.await;
}

/// PROTOCOL_VERSION_11: pre-fix bump-only fee (no charge for the fetch
/// + validation work). Pinned so v11 chain history stays bit-for-bit
/// reproducible.
#[tokio::test]
async fn test_document_replace_on_document_type_that_is_not_mutable_protocol_version_11() {
run_document_replace_on_document_type_that_is_not_mutable_at_protocol_version(11, 41880)
.await;
}

/// Pins the bump-emission contract on Replace's revision-mismatch path.
///
/// Without the bump, a failed Replace returns errors-only with no action.
/// Fee accounting then charges the user (PaidConsensusError) but the
/// identity_contract_nonce in state never advances — the same exact bytes
/// can be re-broadcast indefinitely.
///
/// The test asserts:
/// 1. After a Replace that fails `check_revision_is_bumped_by_one`, the
/// stored contract nonce MUST advance past the submitted nonce.
/// 2. Re-submitting the same bytes through CheckTx FirstTimeCheck MUST
/// be rejected with `InvalidIdentityNonceError`.
#[tokio::test]
async fn replayed_failed_replace_with_consumed_nonce_must_be_rejected_at_check_tx() {
use crate::execution::check_tx::CheckTxLevel;
use crate::execution::validation::state_transition::check_tx_verification::state_transition_to_execution_event_for_check_tx;
use crate::platform_types::platform::PlatformRef;
use dpp::serialization::PlatformDeserializable;
use dpp::state_transition::StateTransition;

let platform_version = PlatformVersion::latest();
let mut platform = TestPlatformBuilder::new()
.with_latest_protocol_version()
.build_with_mock_rpc()
.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
// mainnet 35C0 was operating on (DPNS-like profile-replace flow).
let profile = dashpay_contract
.document_type_for_name("profile")
.expect("expected a profile document type");
assert!(profile.documents_mutable());

let entropy = Bytes32::random_with_rng(&mut rng);
let mut document = profile
.random_document_with_identifier_and_entropy(
&mut rng,
identity.id(),
entropy,
DocumentFieldFillType::FillIfNotRequired,
DocumentFieldFillSize::AnyDocumentFillSize,
platform_version,
)
.expect("expected a random document");
// Random fillers can produce a non-URI avatarUrl that fails JSON-schema
// validation on Create. Pin it to a valid URI like the sibling tests do.
document.set("avatarUrl", "http://test.com/bob.jpg".into());
document.set("displayName", "Original".into());

// 1) Create at nonce 2 — consumes nonce 2; doc lands at revision 1.
let create_transition = BatchTransition::new_document_creation_transition_from_document(
document.clone(),
profile,
entropy.0,
&key,
2,
0,
None,
&signer,
platform_version,
None,
)
.await
.expect("expected to build create transition");

let create_serialized = create_transition
.serialize_to_bytes()
.expect("expected to serialize create");

let transaction = platform.drive.grove.start_transaction();
let create_result = platform
.platform
.process_raw_state_transitions(
&vec![create_serialized],
&platform_state,
&BlockInfo::default(),
&transaction,
platform_version,
false,
None,
)
.expect("expected to process create");
assert_eq!(create_result.valid_count(), 1);
platform
.drive
.grove
.commit_transaction(transaction)
.unwrap()
.expect("expected to commit create");

let (post_create_nonce_raw, _) = platform
.drive
.fetch_identity_contract_nonce_with_fees(
identity.id().to_buffer(),
dashpay_contract.id().to_buffer(),
&BlockInfo::default(),
true,
None,
platform_version,
)
.expect("expected to fetch contract nonce after create");
let post_create_nonce =
post_create_nonce_raw.expect("contract nonce must be present after create");

// 2) Build a Replace at nonce 3 with revision 3. Doc is at revision
// 1, so check_revision_is_bumped_by_one_during_replace_v0 returns
// InvalidDocumentRevisionError(Some(1), 3) and we hit the
// failure-with-bump path in the transformer.
let mut altered_document = document.clone();
altered_document.set_revision(Some(3));
altered_document.set("displayName", "Out of order".into());

let replace_transition =
BatchTransition::new_document_replacement_transition_from_document(
altered_document,
profile,
&key,
3,
0,
None,
&signer,
platform_version,
None,
)
.await
.expect("expected to build replace transition");

let replace_serialized = replace_transition
.serialize_to_bytes()
.expect("expected to serialize replace");

let transaction = platform.drive.grove.start_transaction();
let replace_result = platform
.platform
.process_raw_state_transitions(
&vec![replace_serialized.clone()],
&platform_state,
&BlockInfo::default(),
&transaction,
platform_version,
false,
None,
)
.expect("expected to process replace");
platform
.drive
.grove
.commit_transaction(transaction)
.unwrap()
.expect("expected to commit failed replace");

assert_eq!(
replace_result.invalid_paid_count(),
1,
"Replace must commit as invalid_paid (PaidConsensusError); execution_results={:?}",
replace_result.execution_results()
);
assert_eq!(replace_result.valid_count(), 0);

// 3) Direct invariant: the bump must have advanced the contract nonce
// in state. If the stored nonce is still post-create, the bump
// silently dropped — that is the bug.
let (post_replace_nonce_raw, _) = platform
.drive
.fetch_identity_contract_nonce_with_fees(
identity.id().to_buffer(),
dashpay_contract.id().to_buffer(),
&BlockInfo::default(),
true,
None,
platform_version,
)
.expect("expected to fetch contract nonce after failed replace");
let post_replace_nonce =
post_replace_nonce_raw.expect("contract nonce must be present after failed replace");

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
);
Comment on lines +873 to +879
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.


// 4) Re-submitting identical bytes through CheckTx FirstTimeCheck must
// hit the nonce check first and reject.
let replayed_state_transition =
StateTransition::deserialize_from_bytes(&replace_serialized)
.expect("expected to deserialize replayed transition");

let platform_state = platform.state.load();
let platform_ref = PlatformRef {
drive: &platform.drive,
state: &platform_state,
config: &platform.config,
core_rpc: &platform.core_rpc,
};

let check_tx_result = state_transition_to_execution_event_for_check_tx(
&platform_ref,
replayed_state_transition,
CheckTxLevel::FirstTimeCheck,
platform_version,
)
.expect("expected check_tx to not return an Err");

assert!(
!check_tx_result.is_valid(),
"CheckTx FirstTimeCheck must reject identical bytes after the \
failed-Replace bump consumed the nonce"
);
assert!(
check_tx_result.errors.iter().any(|e| matches!(
e,
ConsensusError::StateError(StateError::InvalidIdentityNonceError(_))
)),
"expected InvalidIdentityNonceError on replay; got {:?}",
check_tx_result.errors
);
}

#[tokio::test]
Expand Down
Loading
Loading