Merge AccumulatorEvents in temporary_store so that transaction effects are accurate #24176

alex-mysten · 2025-11-04T15:27:27Z

Description

Merge AccumulatorWriteV1 objects in temporary_store. This allows authenticated events to be indexed correctly in rpc_index and provides accurate transaction_effects.

Test plan

Address Balance and Authenticated Events tests added; they fail without this change.

Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

vercel · 2025-11-04T15:27:34Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
sui-docs	Ready	Preview	Comment	Nov 15, 2025 11:50am

2 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
multisig-toolkit	Ignored	Preview		Nov 15, 2025 11:50am
sui-kiosk	Ignored	Preview		Nov 15, 2025 11:50am

crates/sui-types/src/effects/object_change.rs

mystenmark · 2025-11-04T17:00:57Z

crates/sui-types/src/effects/object_change.rs

+                };
+                let amount_u64 = amount
+                    .try_into()
+                    .expect("accumulator value overflow: merged amount exceeds u64::MAX");


hmm, i think this expect could be triggered by malicious code. You can create balance amounts of u64::max (not on a "real" currency, but nevertheless), and if you just do that twice you will overflow.

so we either have to use u128s in effects, or else detect the overflow during execution (at emit_accumulator_event in object_runtime.rs) and abort the transaction.

I added validation in emit_accumulator_event. I didn't do the merging here because it would have required changing the types in MoveAccumulatorValue and it seemed cleaner this way, happy to revisit this if you have a different opinion.

crates/sui-types/src/effects/object_change.rs

mystenmark · 2025-11-12T16:56:46Z

sui-execution/latest/sui-move-natives/src/object_runtime/mod.rs

+            let mut merge_total = 0u128;
+            let mut split_total = 0u128;
+
+            for event in &self.state.accumulator_events {


This gives contracts the ability to cause quadratic execution time.

I'd prefer to track the running total in self.state, which should be simple enough, but if for some reason we can't do that, we should make sure there is a reasonable cap on max accumulator events and enforce it here.

Addressed, moved to self.state

…s are accurate

mystenmark · 2025-11-14T16:17:30Z

crates/sui-adapter-transactional-tests/tests/address_balances/large_balance_transfer.move

+//> 1: test::large_balance::create_holder();
+//> TransferObjects([Result(0), Result(1)], Input(0))
+
+//# run test::large_balance::send_large_balance --args object(2,0) @A 18446744073709551614 --sender A


before this, we should have a case that sends two large transfers in a single PTB, to test that it causes a move abort

mystenmark · 2025-11-14T16:18:02Z

crates/sui-adapter-transactional-tests/tests/address_balances/large_balance_transfer.move

+//> 0: sui::balance::redeem_funds<test::large_balance::MARKER>(Input(0));
+//> 1: sui::balance::redeem_funds<test::large_balance::MARKER>(Input(1));
+//> 2: sui::balance::join<test::large_balance::MARKER>(Result(0), Result(1));
+//> 3: sui::balance::send_funds<test::large_balance::MARKER>(Result(0), Input(2));


can we follow this up with a check that we can successfully withdraw the large amounts one at a time? Or do we already have that?

mystenmark · 2025-11-14T16:22:08Z

crates/sui-types/src/effects/object_change.rs

+                            match w.operation {
+                                //we validate that these do not overflow in object_runtime
+                                AccumulatorOperation::Merge => (merge + v, split),
+                                AccumulatorOperation::Split => (merge, split + v),


lets do merge.checked_add(v).expect("validated in object runtime")

(Or you can wrap the code with the checked_arithmetic! { ... } macro to rewrite it automatically)

Used checked_add :)

mystenmark

approved with some small nits. thanks!

alex-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 4, 2025 15:27 — with GitHub Actions Inactive

alex-mysten changed the title ~~Merge AccumulatorEvents in temporary_store so that transaction effect…~~ Merge AccumulatorEvents in temporary_store so that transaction effects are accurate Nov 4, 2025

alex-mysten requested review from lxfind and mystenmark November 4, 2025 15:29

alex-mysten force-pushed the steka-aes-event-indexing-fix branch from 3fa273f to 8175d6b Compare November 4, 2025 15:53

alex-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 4, 2025 15:53 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 4, 2025 15:55 View deployment

alex-mysten force-pushed the steka-aes-event-indexing-fix branch from 8175d6b to 242bbd6 Compare November 4, 2025 16:03

alex-mysten marked this pull request as ready for review November 4, 2025 16:04

alex-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 4, 2025 16:04 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 4, 2025 16:05 View deployment

mystenmark reviewed Nov 4, 2025

View reviewed changes

crates/sui-types/src/effects/object_change.rs Outdated Show resolved Hide resolved

mystenmark reviewed Nov 4, 2025

View reviewed changes

crates/sui-types/src/effects/object_change.rs Outdated Show resolved Hide resolved

mystenmark reviewed Nov 4, 2025

View reviewed changes

crates/sui-types/src/effects/object_change.rs Outdated Show resolved Hide resolved

alex-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 7, 2025 01:26 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 7, 2025 01:28 View deployment

alex-mysten force-pushed the steka-aes-event-indexing-fix branch from d46664d to da06499 Compare November 7, 2025 01:32

alex-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 7, 2025 01:32 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 7, 2025 01:34 View deployment

alex-mysten force-pushed the steka-aes-event-indexing-fix branch from da06499 to 10ea44d Compare November 7, 2025 13:58

alex-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 7, 2025 13:58 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 7, 2025 14:00 View deployment

alex-mysten requested a review from mystenmark November 7, 2025 16:06

mystenmark reviewed Nov 12, 2025

View reviewed changes

alex-mysten force-pushed the steka-aes-event-indexing-fix branch from 10ea44d to b3eba0e Compare November 12, 2025 18:47

alex-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 12, 2025 18:47 — with GitHub Actions Inactive

Merge AccumulatorEvents in temporary_store so that transaction effect…

ee1280f

…s are accurate

alex-mysten force-pushed the steka-aes-event-indexing-fix branch from b3eba0e to 9a15fa9 Compare November 12, 2025 18:48

alex-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 12, 2025 18:48 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 12, 2025 18:50 View deployment

alex-mysten force-pushed the steka-aes-event-indexing-fix branch from 9a15fa9 to 82f372c Compare November 12, 2025 18:56

alex-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 12, 2025 18:56 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 12, 2025 18:58 View deployment

alex-mysten requested a review from mystenmark November 13, 2025 13:39

alex-mysten force-pushed the steka-aes-event-indexing-fix branch from 82f372c to 068e6e2 Compare November 13, 2025 22:13

alex-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 13, 2025 22:13 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 13, 2025 22:15 View deployment

mystenmark reviewed Nov 14, 2025

View reviewed changes

mystenmark approved these changes Nov 14, 2025

View reviewed changes

add u64 overflow prevention

bfb71dd

alex-mysten force-pushed the steka-aes-event-indexing-fix branch from 068e6e2 to bfb71dd Compare November 15, 2025 11:48

alex-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 15, 2025 11:48 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 15, 2025 11:50 View deployment

alex-mysten merged commit 76f4ce9 into main Nov 17, 2025
56 checks passed

alex-mysten deleted the steka-aes-event-indexing-fix branch November 17, 2025 17:36

Merge AccumulatorEvents in temporary_store so that transaction effects are accurate #24176

Merge AccumulatorEvents in temporary_store so that transaction effects are accurate #24176

Conversation

alex-mysten commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test plan

Release notes

Uh oh!

vercel bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mystenmark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alex-mysten commented Nov 4, 2025 •

edited

Loading

vercel bot commented Nov 4, 2025 •

edited

Loading