Support for multiple settlement transactions per commit #23733
Conversation
mystenmark
commented
Sep 24, 2025
mystenmark
commented
- Changes to support non-exclusive mutable shared object access.
- Restrict user transactions from using SharedObjectV2 (for now)
- Changes to allow &mut inputs that do not get an automatic version bump
- Build multiple settlement transactions
- Scheduler support for non-exclusive-write + barrier transactions
- A new ObjectArg variant is added which has an enum to express mutability rather than a boolean. - Initially this will only be allowed for system transactions, specifically settlement transactions. Settlement transactions are guaranteed not to have overlapping DF writes. - In the future, we could expose this to users, but we would have to abort transactions that do overlapping writes, or transactions that directly mutate the root. The next steps for this feature are: - Changes to execution to avoid bumping versions of non-exclusive write inputs. - Consensus handler will schedule a system transaction to advance the object's version after all non-exclusive writes have completed. This ensures that all DF writes that happen at a given owner version become visible atomically.
mystenmark
requested review from
a team,
ronny-mysten and
jessiemongeon1
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
mystenmark
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
| mutable: match mutability { | ||
| SharedObjectMutability::Mutable => true, | ||
| SharedObjectMutability::Immutable => false, | ||
| SharedObjectMutability::NonExclusiveWrite => todo!(), |
There was a problem hiding this comment.
For consistency, we shall have // TODO(address-balances)
| UnchangedConsensusKind::ReadConsensusStreamEnded(version), | ||
| )) | ||
| )), | ||
| SharedObjectMutability::NonExclusiveWrite => Some(( |
There was a problem hiding this comment.
Is this line reachable at all? Might be better to annotate unreachable if this is for the future.
There was a problem hiding this comment.
it is unreachable for now but it seems easy enough to do the correct thing here right now? Added a comment
| } => { | ||
| message.object_id = Some(id.to_canonical_string(true)); | ||
| message.version = Some(initial_shared_version.value()); | ||
| // TODO: add enum to schema |
| mutable: match mutability { | ||
| crate::transaction::SharedObjectMutability::Mutable => true, | ||
| crate::transaction::SharedObjectMutability::Immutable => false, | ||
| crate::transaction::SharedObjectMutability::NonExclusiveWrite => todo!(), |
| Self::SharedMoveObject { mutability, .. } => match mutability { | ||
| SharedObjectMutability::Mutable => true, | ||
| SharedObjectMutability::Immutable => false, | ||
| SharedObjectMutability::NonExclusiveWrite => todo!(), |
There was a problem hiding this comment.
I am not sure if it's safe to make this a todo. Are we certain this function can never be called before we verify a transaction? Might need find a better way to mark this as todo
There was a problem hiding this comment.
This method does not even appear to be used anywhere, removing
crates/sui-types/src/transaction.rs
Outdated
| (InputObjectKind::SharedMoveObject { mutability, .. }, _) => match mutability { | ||
| SharedObjectMutability::Mutable => true, | ||
| SharedObjectMutability::Immutable => false, | ||
| SharedObjectMutability::NonExclusiveWrite => todo!(), |
crates/sui-types/src/transaction.rs
Outdated
| } | ||
| } | ||
| SharedObjectMutability::Immutable => None, | ||
| SharedObjectMutability::NonExclusiveWrite => todo!(), |
|
|
||
| /// All inputs that will be directly mutated by the transaction. This does | ||
| /// not include SharedObjectMutability::NonExclusiveWrite inputs. | ||
| pub fn mutated_inputs(&self) -> BTreeMap<ObjectID, (VersionDigest, Owner)> { |
There was a problem hiding this comment.
I think the two names look too similar to use correctly. It's also confusing to call it "mutated" on a transaction instead of effects. I would certainly mis-use if I didn't review this PR. Maybe explicitly name with exclusive_mutable_inputs and all_mutable_inputs? This would then be consistent with the following function non_exclusive_mutable_inputs
| self.objects.iter().filter_map(|o| o.as_object()) | ||
| } | ||
|
|
||
| pub fn non_exclusive_writes(&self) -> impl Iterator<Item = (ObjectID, SequenceNumber)> + '_ { |
There was a problem hiding this comment.
Should be mutable_inputs instead of writes
| input_metadata_opt, | ||
| Some(InputObjectMetadata::InputObject { | ||
| is_mutable_input: false, | ||
| mutability: Mutability::Immutable | Mutability::NonExclusiveWrite, |
There was a problem hiding this comment.
We should not allow taking NonExclusiveWrite by value?
There was a problem hiding this comment.
that's what this is doing - if its immutable or non-exclusive, it returns an error
| Some(InputObjectMetadata::InputObject { | ||
| // We are only interested in mutable inputs. | ||
| is_mutable_input: true, | ||
| mutability: Mutability::Mutable, |
There was a problem hiding this comment.
I don't fully understand whether this should include NonExlusiveMutable
There was a problem hiding this comment.
i don't think it should - this causes mutable inputs to be written even if the transaction did not mutate them
| settlements.push(build_one_settlement_txn( | ||
| settlements.len() as u64, | ||
| // pending_updates will be drained and can be re-used | ||
| &mut pending_updates, |
There was a problem hiding this comment.
Feels cleaner to drain the updates here and pass it by value
| input_sui: u64, | ||
| output_sui: u64, |
There was a problem hiding this comment.
Please add some comments on why these belong to the Updates
| _ => false, | ||
| }) | ||
| .expect( | ||
| "barrier transaction must have a version for SUI_ACCUMULATOR_ROOT_OBJECT_ID", |
| ) | ||
| .filter_map( | ||
| |(txn, tx_digest, expected_fx_digest, effects, executed_fx_digest)| { | ||
| let mut barrier_counts = BTreeMap::new(); |
There was a problem hiding this comment.
I think this can be problematic since it is still possible that we split a large checkpoint into multiple checkpoints.
There was a problem hiding this comment.
this is a good point. this problem will go away once i move checkpoint splitting into consensus handler
| } | ||
|
|
||
| impl Default for ObjectWriteBarrier { | ||
| fn default() -> Self { |
There was a problem hiding this comment.
Why do we introduce the default constructor? Seems non-intuitive to transition from 0 count to a non-zero count
There was a problem hiding this comment.
its so that we can use or_default() when inserting into the map. We can't start out with the correct count because we may receive a notification before we receive a wait(u32) call.
| }; | ||
| } | ||
|
|
||
| async fn wait_barrier(&self, key: (ObjectID, SequenceNumber), count: u32) { |
There was a problem hiding this comment.
Can you explain how this deals with races between checkpoint builder and checkpoint executor?
There was a problem hiding this comment.
ah, i suppose that the same transaction can be enqueued twice at the same time. will have to support multiple waiters :/
