Add gas validation fuzz and edge-case tests for dry-run and dev-inspect#25541
Add gas validation fuzz and edge-case tests for dry-run and dev-inspect#25541akmysti merged 1 commit intoMystenLabs:bella-ciaofrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
tzakian
left a comment
There was a problem hiding this comment.
This looks good to me. A couple suggestions for future testing directions, but nothing needed for this PR. @dariorussi wants to take another look over as well though too.
| .map(|_| ()) | ||
| } | ||
|
|
||
| pub fn dev_inspect_transaction( |
There was a problem hiding this comment.
This is great. Could you also add a similar method for calling into simulate_transaction as well (https://github.com/MystenLabs/sui/blob/main/crates/sui-core/src/authority.rs#L2390).
Not needed for this PR, but would be good to have in a followup PR.
| .map(|o| match o { | ||
| Owner::ObjectOwner(_) | Owner::AddressOwner(_) if owned_by_sender => { | ||
| .map(|o| { | ||
| if force_all_sender_owned { |
There was a problem hiding this comment.
Not for this PR (but for a followup PR): we should add the ability to generate transactions with address balance gas payments as well.
| let kind = make_transfer_sui_pt(); | ||
| let tx_data = TransactionData::new_with_gas_data(kind, sender, gas_data); | ||
|
|
||
| let result = executor.dry_run_transaction(tx_data); |
There was a problem hiding this comment.
Question: Is this test purely testing for panic freedom? Or should we be doing something with the result here?
There was a problem hiding this comment.
Yes, for fuzz testing this is what I had in mind. The results may or maynot be valid as long as we dont panic somewhere in between.
| let kind = make_transfer_sui_pt(); | ||
| let gas_refs: Vec<ObjectRef> = gas_data.payment.clone(); | ||
|
|
||
| let result = executor.dev_inspect_transaction( |
| /// Valid gas data succeeds in all execution modes. | ||
| #[test] | ||
| #[cfg_attr(msim, ignore)] | ||
| fn test_valid_gas_data_all_modes() { |
There was a problem hiding this comment.
Not needed for this PR, but extending these to be proptests, and using prop_oneof! to make sure the specific edge cases are covered would be great.
dariorussi
left a comment
There was a problem hiding this comment.
this is looking good. I took another look after looking at that with Tim and no issue.
We can clean this up and get it in and do the other work that was described in the comment (simulate_transaction and account balance)
Description
Extend the transaction-fuzzer crate to test gas data handling across all
four execution modes: normal (validator), dry-run (fullnode), dev-inspect
with skip_checks=true, and dev-inspect with skip_checks=false. This
exercises the gas validation paths fixed in #25361 where (1) gas price
was accessed unchecked causing panics and (2) non-gas-coin objects as
gas in dev-inspect caused crashes.
Changes to executor.rs:
new_fullnode()constructor that uses a keypair not in thecommittee, enabling dry-run and dev-inspect APIs
dry_run_transaction()wrappingAuthorityState::dry_exec_transactiondev_inspect_transaction()wrappingAuthorityState::dev_inspect_transaction_blockChanges to lib.rs:
GasDataGenConfig::sender_owned_only()which forces all gas coinsto
AddressOwner(sender), needed for dev-inspect with skip_checks=truewhere ownership validation is bypassed and non-sender-owned gas objects
would panic during execution
run_proptest_with_fullnode()helper and refactor existingrun_proptest()to share logic viarun_proptest_with_executor()Changes to gas_data_tests.rs:
and dev-inspect without skip_checks
gas_price=0, gas_price < RGP, gas_budget=0, gas_budget > max, and
insufficient balance — each verified across all four execution modes
modes in a single test
Test plan
The gas tests cover 4 execution modes across 2 testing strategies:
Proptest Fuzz Tests (5 total)
Randomly generate gas data (price, budget, balance, number of gas
objects, ownership) and verify no panics across execution modes:
test_gas_data_owned_or_immut: sender-owned or immutable gas, normal executiontest_gas_data_any_owner: any owner type, normal executiontest_gas_data_dry_run_fuzz: sender-owned or immutable gas, dry-runtest_gas_data_dev_inspect_skip_checks_fuzz: sender-owned only, dev-inspect (skip_checks=true)test_gas_data_dev_inspect_no_skip_fuzz: sender-owned or immutable gas, dev-inspect (skip_checks=false)The skip_checks fuzz uses
sender_owned_only()because when ownershipvalidation is bypassed, immutable/shared gas objects panic in
temporary_store.rswhen the system tries to modify them.Deterministic Edge Case Tests (6 total)
Each test runs through all 4 modes (normal, dry-run, dev-inspect with
and without skip_checks) and asserts the expected error:
test_valid_gas_data_all_modes: valid price/budget/balance, all modes succeedtest_gas_price_zero: gas_price=0, expects "Gas price 0 under min"test_gas_price_below_rgp: gas_price=rgp-1, expects "Gas price ... under min"test_gas_budget_zero: gas_budget=0, expects "Gas budget 0 is below min"test_gas_budget_exceeds_max: gas_budget > max_tx_gas, expects "Gas budget ... is above max"test_gas_balance_insufficient: balance=1, expects "balance of ... is lower than the needed"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.