From 939cb4899392e111de07c618d9447e1eafa7c11c Mon Sep 17 00:00:00 2001 From: Junha Park <0xjunha@gmail.com> Date: Thu, 5 Feb 2026 14:15:25 +0900 Subject: [PATCH 1/2] Distinguish new/removed accounts in the same accpar round and those from previous rounds --- pvm/pvm-host/src/context/mod.rs | 4 -- .../src/host_functions/accumulate/mod.rs | 1 - .../src/host_functions/general/tests.rs | 1 - pvm/pvm-invocation/src/accumulate/mod.rs | 6 -- pvm/pvm-invocation/src/accumulate/pipeline.rs | 69 ++++++++++++++++--- pvm/pvm-invocation/src/error.rs | 4 +- 6 files changed, 60 insertions(+), 25 deletions(-) diff --git a/pvm/pvm-host/src/context/mod.rs b/pvm/pvm-host/src/context/mod.rs index 35c0df4c..4318b070 100644 --- a/pvm/pvm-host/src/context/mod.rs +++ b/pvm/pvm-host/src/context/mod.rs @@ -188,8 +188,6 @@ pub struct AccumulateHostContext { pub yielded_accumulate_hash: Option, /// **`p`**: Provided preimage data pub provided_preimages: HashSet<(ServiceId, Octets)>, - /// Service ids created during this accumulation - pub created_service_ids: HashSet, /// Accumulate entry-point function invocation args (read-only) pub invoke_args: AccumulateInvokeArgs, /// Current entropy value (`η0′`) @@ -205,7 +203,6 @@ impl Clone for AccumulateHostContext { deferred_transfers: self.deferred_transfers.clone(), yielded_accumulate_hash: self.yielded_accumulate_hash.clone(), provided_preimages: self.provided_preimages.clone(), - created_service_ids: self.created_service_ids.clone(), invoke_args: self.invoke_args.clone(), curr_entropy: self.curr_entropy.clone(), } @@ -235,7 +232,6 @@ impl AccumulateHostContext { deferred_transfers: Vec::new(), yielded_accumulate_hash: None, provided_preimages: HashSet::new(), - created_service_ids: HashSet::new(), invoke_args, curr_entropy, }) diff --git a/pvm/pvm-host/src/host_functions/accumulate/mod.rs b/pvm/pvm-host/src/host_functions/accumulate/mod.rs index ebf7ff15..3f31f72d 100644 --- a/pvm/pvm-host/src/host_functions/accumulate/mod.rs +++ b/pvm/pvm-host/src/host_functions/accumulate/mod.rs @@ -449,7 +449,6 @@ impl AccumulateHostFunction { x.rotate_new_account_id(state_provider).await?; new_service_id }; - x.created_service_ids.insert(new_service_id); tracing::debug!( "NEW service_id={new_service_id} parent={}", diff --git a/pvm/pvm-host/src/host_functions/general/tests.rs b/pvm/pvm-host/src/host_functions/general/tests.rs index a5cf323a..4a3881e2 100644 --- a/pvm/pvm-host/src/host_functions/general/tests.rs +++ b/pvm/pvm-host/src/host_functions/general/tests.rs @@ -297,7 +297,6 @@ mod fetch_tests { deferred_transfers: vec![], yielded_accumulate_hash: None, provided_preimages: HashSet::new(), - created_service_ids: HashSet::new(), invoke_args: AccumulateInvokeArgs { inputs: self.accumulate_inputs.clone(), ..Default::default() diff --git a/pvm/pvm-invocation/src/accumulate/mod.rs b/pvm/pvm-invocation/src/accumulate/mod.rs index 9a3b2d2c..a73c7c7f 100644 --- a/pvm/pvm-invocation/src/accumulate/mod.rs +++ b/pvm/pvm-invocation/src/accumulate/mod.rs @@ -48,8 +48,6 @@ pub struct AccumulateResult { pub gas_used: UnsignedGas, /// **`p`**: Provided preimage entries during accumulation pub provided_preimages: HashSet<(ServiceId, Octets)>, - /// Service ids created during accumulation - pub created_service_ids: HashSet, pub accumulate_host: ServiceId, } @@ -61,7 +59,6 @@ impl Default for AccumulateResult { yielded_accumulate_hash: None, gas_used: UnsignedGas::default(), provided_preimages: HashSet::new(), - created_service_ids: HashSet::new(), accumulate_host: ServiceId::default(), } } @@ -239,7 +236,6 @@ impl AccumulateInvocation { gas_used: result.gas_used, accumulate_host: x.accumulate_host, provided_preimages: x.provided_preimages, - created_service_ids: x.created_service_ids, })) } PVMInvocationOutput::OutputUnavailable => Ok(Some(AccumulateResult { @@ -249,7 +245,6 @@ impl AccumulateInvocation { gas_used: result.gas_used, accumulate_host: x.accumulate_host, provided_preimages: x.provided_preimages, - created_service_ids: x.created_service_ids, })), PVMInvocationOutput::OutOfGas(_) | PVMInvocationOutput::Panic(_) => { Ok(Some(AccumulateResult { @@ -259,7 +254,6 @@ impl AccumulateInvocation { gas_used: result.gas_used, // Note: taking gas usage from the `x` context accumulate_host: x.accumulate_host, provided_preimages: y.provided_preimages, - created_service_ids: y.created_service_ids, })) } } diff --git a/pvm/pvm-invocation/src/accumulate/pipeline.rs b/pvm/pvm-invocation/src/accumulate/pipeline.rs index d7cd426e..083e4ff0 100644 --- a/pvm/pvm-invocation/src/accumulate/pipeline.rs +++ b/pvm/pvm-invocation/src/accumulate/pipeline.rs @@ -188,7 +188,8 @@ async fn merge_partial_state_change( prev_assigns: &AssignServices, prev_designate: ServiceId, prev_registrar: ServiceId, - created_service_ids: &HashSet, + new_service_ids_this_round: &mut HashSet, + removed_service_ids_this_round: &mut HashSet, ) -> Result<(), PVMInvokeError> { // Accumulating service sandbox let accumulate_host_sandbox = partial_state_union @@ -316,29 +317,67 @@ async fn merge_partial_state_change( SandboxEntryStatus::Added => { match partial_state_union.accounts_sandbox.get(&service_id) { None => { + if new_service_ids_this_round.contains(&service_id) { + return Err(PVMInvokeError::DuplicateServiceIdWithinAccumulateRound( + service_id, + )); + } partial_state_union .accounts_sandbox .insert(service_id, sandbox.clone()); + new_service_ids_this_round.insert(service_id); } Some(existing) => { + // Allow re-creating removed account from previous rounds. if matches!(existing.metadata.status(), SandboxEntryStatus::Removed) { - // Allow re-creating a previously removed account. + if new_service_ids_this_round.contains(&service_id) + || removed_service_ids_this_round.contains(&service_id) + { + return Err( + PVMInvokeError::DuplicateServiceIdWithinAccumulateRound( + service_id, + ), + ); + } partial_state_union .accounts_sandbox .insert(service_id, sandbox.clone()); - } else if created_service_ids.contains(&service_id) { - // If NEW service ids collide, block should be considered invalid. - return Err(PVMInvokeError::DuplicateNewServiceId(service_id)); - } else { - // Inherited Added entry from a prior round; skip to avoid overwriting. + new_service_ids_this_round.insert(service_id); } } } } SandboxEntryStatus::Removed => { - partial_state_union - .accounts_sandbox - .insert(service_id, sandbox.clone()); + match partial_state_union.accounts_sandbox.get(&service_id) { + Some(existing) => { + if !matches!(existing.metadata.status(), SandboxEntryStatus::Removed) { + if removed_service_ids_this_round.contains(&service_id) + || new_service_ids_this_round.contains(&service_id) + { + return Err( + PVMInvokeError::DuplicateServiceIdWithinAccumulateRound( + service_id, + ), + ); + } + partial_state_union + .accounts_sandbox + .insert(service_id, sandbox.clone()); + removed_service_ids_this_round.insert(service_id); + } + } + None => { + if removed_service_ids_this_round.contains(&service_id) { + return Err(PVMInvokeError::DuplicateServiceIdWithinAccumulateRound( + service_id, + )); + } + partial_state_union + .accounts_sandbox + .insert(service_id, sandbox.clone()); + removed_service_ids_this_round.insert(service_id); + } + } } _ => {} } @@ -419,6 +458,13 @@ async fn accumulate_parallel( let prev_designate = partial_state_union.designate_service.last_confirmed; let prev_registrar = partial_state_union.registrar_service.last_confirmed; + // It is not allowed to reuse service IDs (Create/Remove) that were modified + // by other accumulate hosts in the current parallel accumulation round. + // Modifications from previous rounds do not cause such conflicts. + // We track the set of IDs modified in the current round to enforce this rule. + let mut new_service_ids_this_round = HashSet::new(); + let mut removed_service_ids_this_round = HashSet::new(); + // Accumulating service groups: 1) With Digests 2) Transfer Receivers 3) Always-accumulates 4) Privileged Services let services_with_digests: BTreeSet = reports .iter() @@ -497,7 +543,8 @@ async fn accumulate_parallel( &prev_assigns, prev_designate, prev_registrar, - &accumulate_result.created_service_ids, + &mut new_service_ids_this_round, + &mut removed_service_ids_this_round, ) .await?; diff --git a/pvm/pvm-invocation/src/error.rs b/pvm/pvm-invocation/src/error.rs index 50537536..a7f55465 100644 --- a/pvm/pvm-invocation/src/error.rs +++ b/pvm/pvm-invocation/src/error.rs @@ -23,14 +23,14 @@ pub enum PVMInvokeError { WorkReportBlobTooLarge, #[error("Account sandbox for accumulate host (s={0}) is missing")] MissingAccumulateHostSandbox(ServiceId), - #[error("Duplicate new service id (s={0})")] - DuplicateNewServiceId(ServiceId), #[error("Number of work digests exceeds maximum")] WorkDigestsOverflow, #[error("Gas usage or gas limit overflowed")] GasOverflow, #[error("Zero padded justification data size exceeds SEGMENT_SIZE")] PagedProofJustificationBlobTooLarge, + #[error("Attempted to add or remove service (s={0}) that was removed or added in the same parallel accumulation round")] + DuplicateServiceIdWithinAccumulateRound(ServiceId), #[error("JamCodecError: {0}")] JamCodecError(#[from] JamCodecError), #[error("CryptoError: {0}")] From 198888f3d65f4e2aba29a7d50998569c16a46a41 Mon Sep 17 00:00:00 2001 From: Junha Park <0xjunha@gmail.com> Date: Thu, 5 Feb 2026 14:58:21 +0900 Subject: [PATCH 2/2] Fix `merge_partial_state_change` --- pvm/pvm-invocation/src/accumulate/pipeline.rs | 123 +++++++++--------- 1 file changed, 62 insertions(+), 61 deletions(-) diff --git a/pvm/pvm-invocation/src/accumulate/pipeline.rs b/pvm/pvm-invocation/src/accumulate/pipeline.rs index 083e4ff0..8bc5fd1c 100644 --- a/pvm/pvm-invocation/src/accumulate/pipeline.rs +++ b/pvm/pvm-invocation/src/accumulate/pipeline.rs @@ -188,6 +188,8 @@ async fn merge_partial_state_change( prev_assigns: &AssignServices, prev_designate: ServiceId, prev_registrar: ServiceId, + base_present_service_ids: &HashSet, + base_removed_service_ids: &HashSet, new_service_ids_this_round: &mut HashSet, removed_service_ids_this_round: &mut HashSet, ) -> Result<(), PVMInvokeError> { @@ -298,7 +300,7 @@ async fn merge_partial_state_change( // Accumulate host state change *accumulate_host_sandbox = accumulate_result_partial_state .accounts_sandbox - .get_account_sandbox(state_manager, accumulate_host) + .get_account_sandbox(state_manager.clone(), accumulate_host) .await? .cloned() .ok_or(PVMInvokeError::MissingAccumulateHostSandbox( @@ -313,71 +315,52 @@ async fn merge_partial_state_change( .iter() .filter(|(&service_id, _)| service_id != accumulate_host) { + // Check whether the service ID existed in the base state at the start of the current + // parallel accumulation round. + let base_exists = if base_removed_service_ids.contains(&service_id) { + false + } else if base_present_service_ids.contains(&service_id) { + true + } else { + state_manager.account_exists(service_id).await? + }; + match sandbox.metadata.status() { SandboxEntryStatus::Added => { - match partial_state_union.accounts_sandbox.get(&service_id) { - None => { - if new_service_ids_this_round.contains(&service_id) { - return Err(PVMInvokeError::DuplicateServiceIdWithinAccumulateRound( - service_id, - )); - } - partial_state_union - .accounts_sandbox - .insert(service_id, sandbox.clone()); - new_service_ids_this_round.insert(service_id); - } - Some(existing) => { - // Allow re-creating removed account from previous rounds. - if matches!(existing.metadata.status(), SandboxEntryStatus::Removed) { - if new_service_ids_this_round.contains(&service_id) - || removed_service_ids_this_round.contains(&service_id) - { - return Err( - PVMInvokeError::DuplicateServiceIdWithinAccumulateRound( - service_id, - ), - ); - } - partial_state_union - .accounts_sandbox - .insert(service_id, sandbox.clone()); - new_service_ids_this_round.insert(service_id); - } - } + // Not a new ID for this round + if base_exists { + continue; + } + // Reject block if the same service ID is created or removed within the same round + if new_service_ids_this_round.contains(&service_id) + || removed_service_ids_this_round.contains(&service_id) + { + return Err(PVMInvokeError::DuplicateServiceIdWithinAccumulateRound( + service_id, + )); } + partial_state_union + .accounts_sandbox + .insert(service_id, sandbox.clone()); + new_service_ids_this_round.insert(service_id); } SandboxEntryStatus::Removed => { - match partial_state_union.accounts_sandbox.get(&service_id) { - Some(existing) => { - if !matches!(existing.metadata.status(), SandboxEntryStatus::Removed) { - if removed_service_ids_this_round.contains(&service_id) - || new_service_ids_this_round.contains(&service_id) - { - return Err( - PVMInvokeError::DuplicateServiceIdWithinAccumulateRound( - service_id, - ), - ); - } - partial_state_union - .accounts_sandbox - .insert(service_id, sandbox.clone()); - removed_service_ids_this_round.insert(service_id); - } - } - None => { - if removed_service_ids_this_round.contains(&service_id) { - return Err(PVMInvokeError::DuplicateServiceIdWithinAccumulateRound( - service_id, - )); - } - partial_state_union - .accounts_sandbox - .insert(service_id, sandbox.clone()); - removed_service_ids_this_round.insert(service_id); - } + // Ignore removals of accounts that didn't exist at round start + if !base_exists { + continue; } + // Reject block if the same service ID is created or removed within the same round + if new_service_ids_this_round.contains(&service_id) + || removed_service_ids_this_round.contains(&service_id) + { + return Err(PVMInvokeError::DuplicateServiceIdWithinAccumulateRound( + service_id, + )); + } + partial_state_union + .accounts_sandbox + .insert(service_id, sandbox.clone()); + removed_service_ids_this_round.insert(service_id); } _ => {} } @@ -461,7 +444,23 @@ async fn accumulate_parallel( // It is not allowed to reuse service IDs (Create/Remove) that were modified // by other accumulate hosts in the current parallel accumulation round. // Modifications from previous rounds do not cause such conflicts. - // We track the set of IDs modified in the current round to enforce this rule. + // We track the set of IDs that are present/removed from the base state. + // Also, we track service IDs that are modified in the current round to enforce this rule. + let base_present_service_ids: HashSet = partial_state_union + .accounts_sandbox + .iter() + .filter_map(|(&service_id, sandbox)| { + (!matches!(sandbox.metadata.status(), SandboxEntryStatus::Removed)) + .then_some(service_id) + }) + .collect(); + let base_removed_service_ids: HashSet = partial_state_union + .accounts_sandbox + .iter() + .filter_map(|(&service_id, sandbox)| { + matches!(sandbox.metadata.status(), SandboxEntryStatus::Removed).then_some(service_id) + }) + .collect(); let mut new_service_ids_this_round = HashSet::new(); let mut removed_service_ids_this_round = HashSet::new(); @@ -543,6 +542,8 @@ async fn accumulate_parallel( &prev_assigns, prev_designate, prev_registrar, + &base_present_service_ids, + &base_removed_service_ids, &mut new_service_ids_this_round, &mut removed_service_ids_this_round, )