Skip to content
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
27 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
2511248
fix(drive-abci): close paid-with-empty-action gap in batch transformer
shumkov May 7, 2026
46cc656
refactor(dpp,drive-abci): rename strict aggregators to canonical; leg…
shumkov May 7, 2026
ce5c375
chore: deprecate _or_empty_vec aggregators, simplify None construction
shumkov May 7, 2026
fdecf17
fix(drive-abci): bump nonce of failed transitions in best-effort batches
shumkov May 7, 2026
4c7aa62
Revert "fix(drive-abci): bump nonce of failed transitions in best-eff…
shumkov May 7, 2026
99cc0ab
refactor(dpp,drive-abci): version flatten/merge_many instead of the b…
shumkov May 7, 2026
9a7d350
refactor(dpp): organise validation_result aggregator versions per cod…
shumkov May 7, 2026
c37622f
test(dpp): co-locate validation_result aggregator tests with their ve…
shumkov May 7, 2026
1d0a02b
refactor(drive-abci): tighten bump-emission framing in batch transfor…
shumkov May 7, 2026
f9d0ee1
style(drive-abci): cargo fmt fetch_documents.rs flatten call
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
c402712
Merge branch 'v3.1-dev' into fix/issue-2867-validating-state-transiti…
QuantumExplorer May 8, 2026
24437ef
Merge remote-tracking branch 'origin/v3.1-dev' into fix/issue-2867-bu…
shumkov May 8, 2026
583bd6d
test(drive-abci): expect UnpaidConsensusError for single-tx purchase …
shumkov May 8, 2026
d353bc9
Merge remote-tracking branch 'origin/fix/issue-2867-bump-nonce-on-fai…
shumkov May 8, 2026
964df5b
test(drive-abci): pair purchase-failure tests for v11/v12 protocol co…
shumkov May 8, 2026
8ab48a0
fix(drive-abci): preserve v11 bump on Replace's missing-target-docume…
shumkov May 11, 2026
869d257
docs(drive-abci): correct nft.rs helper doc — v11 lands Paid via lega…
shumkov May 11, 2026
50addd7
docs(platform-version): drop stale PR #3608 reference from max_transi…
shumkov May 11, 2026
ed0c66a
docs(drive-abci): explain why batch transformer keeps the _v0 suffix …
shumkov May 11, 2026
7fb8495
docs(dpp): document Some(empty_vec) collapse hazard in flatten_v1
shumkov May 11, 2026
046ccef
test(drive-abci): pin tokens-always-pay invariant for batch transform…
shumkov May 11, 2026
11c87bc
docs(drive-abci): address QE review comments on transformer/v0 doc ac…
shumkov May 11, 2026
8d7cad6
docs(drive-abci): trim Replace inline-bump comment per QE review
shumkov May 12, 2026
fd92ebd
docs(drive-abci): fix mislabeled inline comment in set_price_on_not_o…
shumkov May 12, 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
pub(super) mod v0;
pub(super) mod v1;
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
//! v0 of [`ConsensusValidationResult::flatten`].
//!
//! Legacy semantics: always returns `data: Some(Vec<...>)`, including
//! `Some(empty_vec)` when no input contributed any data.
//!
//! Preserved for `PROTOCOL_VERSION_11` and below — the
//! `Some(empty_vec)`-on-no-data behavior is part of the existing chain
//! history, and changing it would be a consensus-breaking change for
//! already-finalized blocks. New code should let the facade dispatch to v1.
//!
//! See issue #2867 for context.
//!
//! [`ConsensusValidationResult::flatten`]: crate::validation::ConsensusValidationResult::flatten

use crate::validation::ValidationResult;
use std::fmt::Debug;

pub(in crate::validation::validation_result) fn flatten_v0<TData, E, I>(
items: I,
) -> 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);
}
});
ValidationResult::new_with_data_and_errors(aggregate_data, aggregate_errors)
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn merges_data_and_errors() {
let r1: ValidationResult<Vec<i32>, String> = ValidationResult::new_with_data(vec![1, 2]);
let r2: ValidationResult<Vec<i32>, String> =
ValidationResult::new_with_data_and_errors(vec![3], vec!["e".to_string()]);
let r3: ValidationResult<Vec<i32>, String> =
ValidationResult::new_with_error("e2".to_string());

let flat = flatten_v0(vec![r1, r2, r3]);
assert_eq!(flat.data, Some(vec![1, 2, 3]));
assert_eq!(flat.errors, vec!["e".to_string(), "e2".to_string()]);
}

#[test]
fn empty_input_returns_some_empty() {
// Legacy v11 behavior: Some(empty_vec), not None.
let flat: ValidationResult<Vec<i32>, String> =
flatten_v0(std::iter::empty::<ValidationResult<Vec<i32>, String>>());
assert_eq!(flat.data, Some(vec![]));
assert!(flat.errors.is_empty());
}

#[test]
fn all_inputs_no_data_returns_some_empty() {
let r1: ValidationResult<Vec<i32>, String> =
ValidationResult::new_with_error("e1".to_string());
let r2: ValidationResult<Vec<i32>, String> =
ValidationResult::new_with_error("e2".to_string());

let flat = flatten_v0(vec![r1, r2]);
assert_eq!(flat.data, Some(vec![]));
assert_eq!(flat.errors, vec!["e1".to_string(), "e2".to_string()]);
}
}
121 changes: 121 additions & 0 deletions packages/rs-dpp/src/validation/validation_result/flatten/v1/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
//! v1 of [`ConsensusValidationResult::flatten`].
//!
//! Canonical semantics: returns `data: None` when no input contributed any
//! data (i.e. every input was either `data: None` or `data: Some(empty_vec)`),
//! and `data: Some(merged_vec)` when at least one input contributed
//! non-empty data.
//!
//! This honors the invariant `data.is_none() ⇔ no work done`, which
//! downstream code (e.g. `process_validation_result_v0:241`) relies on to
//! choose between `PaidConsensusError` and `UnpaidConsensusError`.
//!
//! # Caller-intent ambiguity
//!
//! `flatten_v1` keys on `aggregate_data.is_empty()` to decide between
//! `data: None` and `data: Some(_)`. This collapses two distinct
//! caller-side intents into the same output:
//!
//! * **Truly no work**: every input had `data: None`.
//! * **Validated but produced no output**: every input had
//! `data: Some(empty_vec)`.
//!
//! v1 cannot distinguish those two cases at the aggregate level — both
//! end up as `data: None` and are routed to `UnpaidConsensusError`
//! downstream. For the documents-batch path under PROTOCOL_VERSION_12 this
//! is safe: every per-transition handler emits at least one action on
//! success and a bump action on failure, so no caller produces
//! `Some(empty_vec)`. A future caller that needs "validated, but no
//! actions to apply" must signal that with at least one non-empty entry,
//! not with `Some(empty_vec)`.
//!
//! See issue #2867 for context.
//!
//! [`ConsensusValidationResult::flatten`]: crate::validation::ConsensusValidationResult::flatten

use crate::validation::ValidationResult;
Comment on lines +13 to +35
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 Suggestion: flatten_v1 collapses Some(empty_vec) into data:None — same footgun class this PR is closing

flatten_v1 keys on aggregate_data.is_empty() to decide between data:None (→ UnpaidConsensusError) and data:Some(_) (→ PaidConsensusError). This collapses two distinct caller intents into one output: (1) every input had data:None (truly no work) and (2) every input had Some(empty_vec) (work performed, no actions emitted). Today's batch transformer never produces Some(empty_vec) under v12 because every per-transition handler emits at least one action or a bump action — but that invariant is maintained by convention across many call sites. A future per-transition path (new TokenOperationType, new DocumentTransition variant, or a refactor) that returns ConsensusValidationResult::new_with_data(vec![]) silently slides back into data:None aggregate → tx erased from block, free validation work, nonce never advances — exactly the #2867 shape this PR is closing.

A stronger formulation would track any_some_input: bool separately from aggregate_data so Some(empty_vec) inputs surface as Some(empty_vec) (or as an internal error), while pure None inputs continue to collapse to None. The PR's own all_some_empty_returns_none test demonstrates the collapse — at minimum, add a defensive debug_assert! or doc-level invariant on per-item shape at each call site.

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 13-35: flatten_v1 collapses Some(empty_vec) into data:None — same footgun class this PR is closing
  `flatten_v1` keys on `aggregate_data.is_empty()` to decide between `data:None` (→ UnpaidConsensusError) and `data:Some(_)` (→ PaidConsensusError). This collapses two distinct caller intents into one output: (1) every input had `data:None` (truly no work) and (2) every input had `Some(empty_vec)` (work performed, no actions emitted). Today's batch transformer never produces `Some(empty_vec)` under v12 because every per-transition handler emits at least one action or a bump action — but that invariant is maintained by convention across many call sites. A future per-transition path (new TokenOperationType, new DocumentTransition variant, or a refactor) that returns `ConsensusValidationResult::new_with_data(vec![])` silently slides back into `data:None` aggregate → tx erased from block, free validation work, nonce never advances — exactly the #2867 shape this PR is closing.

A stronger formulation would track `any_some_input: bool` separately from `aggregate_data` so `Some(empty_vec)` inputs surface as `Some(empty_vec)` (or as an internal error), while pure `None` inputs continue to collapse to `None`. The PR's own `all_some_empty_returns_none` test demonstrates the collapse — at minimum, add a defensive `debug_assert!` or doc-level invariant on per-item shape at each call site.

use std::fmt::Debug;

pub(in crate::validation::validation_result) fn flatten_v1<TData, E, I>(
items: I,
) -> 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)
}
}
Comment thread
shumkov marked this conversation as resolved.

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn merges_non_empty_data() {
let r1: ValidationResult<Vec<i32>, String> = ValidationResult::new_with_data(vec![1, 2]);
let r2: ValidationResult<Vec<i32>, String> =
ValidationResult::new_with_data_and_errors(vec![3], vec!["e".to_string()]);
let r3: ValidationResult<Vec<i32>, String> =
ValidationResult::new_with_error("e2".to_string());

let flat = flatten_v1(vec![r1, r2, r3]);
assert_eq!(flat.data, Some(vec![1, 2, 3]));
assert_eq!(flat.errors, vec!["e".to_string(), "e2".to_string()]);
}

#[test]
fn empty_input_returns_none() {
let flat: ValidationResult<Vec<i32>, String> =
flatten_v1(std::iter::empty::<ValidationResult<Vec<i32>, String>>());
assert_eq!(flat.data, None);
assert!(flat.errors.is_empty());
}

#[test]
fn all_inputs_no_data_returns_none() {
// Downstream code (process_validation_result_v0:241) keys on
// data.is_none() to route to UnpaidConsensusError.
let r1: ValidationResult<Vec<i32>, String> =
ValidationResult::new_with_error("e1".to_string());
let r2: ValidationResult<Vec<i32>, String> =
ValidationResult::new_with_error("e2".to_string());

let flat = flatten_v1(vec![r1, r2]);
assert!(flat.data.is_none());
assert_eq!(flat.errors, vec!["e1".to_string(), "e2".to_string()]);
}

#[test]
fn some_empty_some_non_empty_returns_some() {
let r1: ValidationResult<Vec<i32>, String> = ValidationResult::new_with_data(vec![]);
let r2: ValidationResult<Vec<i32>, String> = ValidationResult::new_with_data(vec![42]);

let flat = flatten_v1(vec![r1, r2]);
assert_eq!(flat.data, Some(vec![42]));
assert!(flat.errors.is_empty());
}

#[test]
fn all_some_empty_returns_none() {
// All inputs had data:Some(empty_vec). The aggregate Vec is empty → data:None.
let r1: ValidationResult<Vec<i32>, String> = ValidationResult::new_with_data(vec![]);
let r2: ValidationResult<Vec<i32>, String> = ValidationResult::new_with_data(vec![]);

let flat = flatten_v1(vec![r1, r2]);
assert!(flat.data.is_none());
assert!(flat.errors.is_empty());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
pub(super) mod v0;
pub(super) mod v1;
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
//! v0 of [`ValidationResult::merge_many`].
//!
//! Legacy semantics: always returns `data: Some(Vec<...>)`, including
//! `Some(empty_vec)` when no input had `data: Some(_)`.
//!
//! Preserved for `PROTOCOL_VERSION_11` and below — the
//! `Some(empty_vec)`-on-no-data behavior is part of the existing chain
//! history, and changing it would be a consensus-breaking change for
//! already-finalized blocks. New code should let the facade dispatch to v1.
//!
//! See issue #2867 for context.
//!
//! [`ValidationResult::merge_many`]: crate::validation::ValidationResult::merge_many

use crate::validation::ValidationResult;
use std::fmt::Debug;

pub(in crate::validation::validation_result) fn merge_many_v0<TData, E, I>(
items: I,
) -> ValidationResult<Vec<TData>, E>
where
TData: Clone,
E: Debug,
I: IntoIterator<Item = ValidationResult<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)
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn collects_data_into_vec() {
let r1: ValidationResult<i32, String> = ValidationResult::new_with_data(1);
let r2: ValidationResult<i32, String> = ValidationResult::new_with_data(2);
let r3: ValidationResult<i32, String> = ValidationResult::new_with_error("e".to_string());

let merged = merge_many_v0(vec![r1, r2, r3]);
assert_eq!(merged.data, Some(vec![1, 2]));
assert_eq!(merged.errors, vec!["e".to_string()]);
}

#[test]
fn empty_input_returns_some_empty() {
// Legacy v11 behavior: Some(empty_vec), not None.
let merged: ValidationResult<Vec<i32>, String> =
merge_many_v0(std::iter::empty::<ValidationResult<i32, String>>());
assert_eq!(merged.data, Some(vec![]));
assert!(merged.errors.is_empty());
}

#[test]
fn all_inputs_no_data_returns_some_empty() {
let r1: ValidationResult<i32, String> = ValidationResult::new_with_error("e1".to_string());
let r2: ValidationResult<i32, String> = ValidationResult::new_with_error("e2".to_string());

let merged = merge_many_v0(vec![r1, r2]);
assert_eq!(merged.data, Some(vec![]));
assert_eq!(merged.errors, vec!["e1".to_string(), "e2".to_string()]);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
//! v1 of [`ValidationResult::merge_many`].
//!
//! Canonical semantics: returns `data: None` when no input had
//! `data: Some(_)`, and `data: Some(Vec<TData>)` when at least one input
//! contributed data.
//!
//! This honors the invariant `data.is_none() ⇔ no work done`, which
//! downstream code (e.g. `process_validation_result_v0:241`) relies on to
//! choose between `PaidConsensusError` and `UnpaidConsensusError`.
//!
//! # Caller-intent ambiguity
//!
//! `merge_many_v1` keys on `aggregate_data.is_empty()` to decide between
//! `data: None` and `data: Some(_)`. Every `Some(_)` input contributes one
//! element to `aggregate_data`, so the only way to get `data: None` is to
//! have zero inputs with `data: Some(_)`. There is no `Some(empty_vec)`
//! input shape at this layer (the per-item `data` is `TData`, not
//! `Vec<TData>`), so the collapse hazard described for `flatten_v1`
//! doesn't apply here. The dispatcher facade ([`ValidationResult::merge_many`])
//! shares the limitation note for symmetry.
//!
//! See issue #2867 for context.
//!
//! [`ValidationResult::merge_many`]: crate::validation::ValidationResult::merge_many

use crate::validation::ValidationResult;
use std::fmt::Debug;

pub(in crate::validation::validation_result) fn merge_many_v1<TData, E, I>(
items: I,
) -> ValidationResult<Vec<TData>, E>
where
TData: Clone,
E: Debug,
I: IntoIterator<Item = ValidationResult<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);
}
});
if aggregate_data.is_empty() {
ValidationResult::new_with_errors(aggregate_errors)
} else {
ValidationResult::new_with_data_and_errors(aggregate_data, aggregate_errors)
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn collects_non_empty_data() {
let r1: ValidationResult<i32, String> = ValidationResult::new_with_data(1);
let r2: ValidationResult<i32, String> = ValidationResult::new_with_data(2);
let r3: ValidationResult<i32, String> = ValidationResult::new_with_error("e".to_string());

let merged = merge_many_v1(vec![r1, r2, r3]);
assert_eq!(merged.data, Some(vec![1, 2]));
assert_eq!(merged.errors, vec!["e".to_string()]);
}

#[test]
fn empty_input_returns_none() {
let merged: ValidationResult<Vec<i32>, String> =
merge_many_v1(std::iter::empty::<ValidationResult<i32, String>>());
assert!(merged.data.is_none());
assert!(merged.errors.is_empty());
}

#[test]
fn all_inputs_no_data_returns_none() {
let r1: ValidationResult<i32, String> = ValidationResult::new_with_error("e1".to_string());
let r2: ValidationResult<i32, String> = ValidationResult::new_with_error("e2".to_string());

let merged = merge_many_v1(vec![r1, r2]);
assert!(merged.data.is_none());
assert_eq!(merged.errors, vec!["e1".to_string(), "e2".to_string()]);
}

#[test]
fn some_data_returns_some() {
let r1: ValidationResult<i32, String> = ValidationResult::new_with_error("e1".to_string());
let r2: ValidationResult<i32, String> = ValidationResult::new_with_data(7);

let merged = merge_many_v1(vec![r1, r2]);
assert_eq!(merged.data, Some(vec![7]));
assert_eq!(merged.errors, vec!["e1".to_string()]);
}
}
Loading
Loading