Skip to content

Consolidate tx checking in signing and simulate#25624

Merged
lxfind merged 2 commits intomainfrom
xun/consolidate-tx-check-signing-simulate
Feb 27, 2026
Merged

Consolidate tx checking in signing and simulate#25624
lxfind merged 2 commits intomainfrom
xun/consolidate-tx-check-signing-simulate

Conversation

@lxfind
Copy link
Copy Markdown
Contributor

@lxfind lxfind commented Feb 26, 2026

Description

simulate_transaction duplicated validation logic from the signing path but had diverged: it called process_funds_withdrawals_for_execution (which uses .unwrap()) instead of the safe process_funds_withdrawals_for_signing (which returns Result), meaning a malformed transaction could panic a fullnode during simulation. This extracts the shared pre-object-load validation — deny checks, funds withdrawal parsing, and balance availability checks — into pre_object_load_checks, called by both handle_transaction_deny_checks and simulate_transaction, fixing the panic and ensuring the two paths stay in sync.

Test plan

  • cargo check -p sui-core — compiles cleanly
  • SUI_SKIP_SIMTESTS=1 cargo nextest run -p sui-core — all tests pass
  • cargo xclippy — no warnings

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.

  • Protocol:
  • Nodes (Validators and Full nodes): Fixes a potential fullnode panic when simulating a malformed transaction with invalid funds withdrawals.
  • gRPC:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:
  • Indexing Framework:

@vercel
Copy link
Copy Markdown

vercel bot commented Feb 26, 2026

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

Project Deployment Actions Updated (UTC)
sui-docs Ready Ready Preview, Comment Feb 27, 2026 6:46am
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
multisig-toolkit Ignored Ignored Preview Feb 27, 2026 6:46am
sui-kiosk Ignored Ignored Preview Feb 27, 2026 6:46am

Request Review

@lxfind lxfind temporarily deployed to sui-typescript-aws-kms-test-env February 26, 2026 17:16 — with GitHub Actions Inactive
@lxfind lxfind force-pushed the xun/consolidate-tx-check-signing-simulate branch from 3f3492a to de2e0ae Compare February 26, 2026 17:49
@lxfind lxfind temporarily deployed to sui-typescript-aws-kms-test-env February 26, 2026 17:50 — with GitHub Actions Inactive
@lxfind lxfind force-pushed the xun/consolidate-tx-check-signing-simulate branch from de2e0ae to b722f84 Compare February 26, 2026 18:04
@lxfind lxfind temporarily deployed to sui-typescript-aws-kms-test-env February 26, 2026 18:04 — with GitHub Actions Inactive
@lxfind lxfind requested a review from mystenmark February 26, 2026 18:04
@lxfind lxfind marked this pull request as ready for review February 26, 2026 18:04
@lxfind lxfind temporarily deployed to sui-typescript-aws-kms-test-env February 26, 2026 18:04 — with GitHub Actions Inactive
@mystenmark
Copy link
Copy Markdown
Contributor

should we add a test?

@lxfind lxfind temporarily deployed to sui-typescript-aws-kms-test-env February 26, 2026 20:12 — with GitHub Actions Inactive
@lxfind
Copy link
Copy Markdown
Contributor Author

lxfind commented Feb 26, 2026

should we add a test?

done

`simulate_transaction` duplicated validation logic from the signing path but had diverged: it called `process_funds_withdrawals_for_execution` (which uses `.unwrap()`) instead of the safe `process_funds_withdrawals_for_signing` (which returns `Result`), meaning a malformed transaction could panic a fullnode during simulation. This extracts the shared pre-object-load validation — deny checks, funds withdrawal parsing, and balance availability checks — into `pre_object_load_checks`, called by both `handle_transaction_deny_checks` and `simulate_transaction`, fixing the panic and ensuring the two paths stay in sync.
@lxfind lxfind force-pushed the xun/consolidate-tx-check-signing-simulate branch from dc393bf to c1ccf4d Compare February 27, 2026 06:43
@lxfind lxfind temporarily deployed to sui-typescript-aws-kms-test-env February 27, 2026 06:43 — with GitHub Actions Inactive
@lxfind lxfind merged commit 48d0a9b into main Feb 27, 2026
58 of 59 checks passed
@lxfind lxfind deleted the xun/consolidate-tx-check-signing-simulate branch February 27, 2026 07:05
jessiemongeon1 pushed a commit to jessiemongeon1/sui that referenced this pull request Mar 5, 2026
## Description 

`simulate_transaction` duplicated validation logic from the signing path
but had diverged: it called `process_funds_withdrawals_for_execution`
(which uses `.unwrap()`) instead of the safe
`process_funds_withdrawals_for_signing` (which returns `Result`),
meaning a malformed transaction could panic a fullnode during
simulation. This extracts the shared pre-object-load validation — deny
checks, funds withdrawal parsing, and balance availability checks — into
`pre_object_load_checks`, called by both
`handle_transaction_deny_checks` and `simulate_transaction`, fixing the
panic and ensuring the two paths stay in sync.

## Test plan 

- `cargo check -p sui-core` — compiles cleanly
- `SUI_SKIP_SIMTESTS=1 cargo nextest run -p sui-core` — all tests pass
- `cargo xclippy` — no warnings

---

## 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.

- [ ] Protocol: 
- [x] Nodes (Validators and Full nodes): Fixes a potential fullnode
panic when simulating a malformed transaction with invalid funds
withdrawals.
- [ ] gRPC:
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
- [ ] Indexing Framework:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants