[core] dry run should use dev inspect executor

[jordanjennings-mysten]

Description

Necessary for follow up work to improve PTB errors on dry run.

Test plan

How did you test the new or updated feature?

---

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):
• Indexer:
• JSON-RPC:
• GraphQL:
• CLI:
• Rust SDK:
• REST API:

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 22, 2024 8:35pm

3 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview		Nov 22, 2024 8:35pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview		Nov 22, 2024 8:35pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview		Nov 22, 2024 8:35pm

[amnn]

The change itself makes sense, but we should be careful about making sure this change does what we think it does (i.e. nothing, hopefully). Could you flesh out the test plan with various scenarios? If we don't have them as unit tests already, then we should add them. I would want to check that the following cases behave identically before and after this change,

• Gas estimation (calls dry run with empty gas object refs).
• Dry-running a transaction that fails
• Dry-running a transaction that succeeds
• Dry-running an exotic transaction, like a publish or upgrade.

The reason to be careful is that most transactions that go through the network go through a dry-run call first (to do gas estimation) so we want to make sure we don't break that path, or people won't be able to run transactions anymore.

Also good to get some reviews from folks on the Move side for this change.

[dariorussi]

this feels a very intrusive change to me.
Are we sure this leads to the same results for dry run in all cases?
I assume would make dry run more expensive and it seems a departure from where we were?
thoughts?
@tnowacki @tzakian @cgswords

[jordanjennings-mysten]

I see a handful of tests on sui-core

authority::authority_tests::test_dry_run_dev_inspect_dynamic_field_too_new: test
authority::authority_tests::test_dry_run_dev_inspect_max_gas_version: test
authority::authority_tests::test_dry_run_no_gas_big_transfer: test
authority::authority_tests::test_dry_run_on_validator: test
authority::authority_tests::test_dry_run_transaction_block: test
authority::authority_tests::test_for_inc_201_dry_run: test

I'll look at expanding these. Appreciate the feedback I've been comparing the two code paths (dry vs dev inspect) and the only notable difference I found so far was the return types but more tests will be good as well.

[dariorussi]

the issue I have and would love to see discussed (thus the tagging I added) is that - as I remember - devinspect performs operations that we do not do in dryrun, for instance saving results per command. Also I am not clear how dev inspect is going to evolve in the face of transaction replay and the ability to get more insights in a transaction as opposed to normal execution.
I think of dry run as an execution that has got high fidelity with normal execution and the idea of conflating that with dev inspect seems a stretch to me.
And I believe that is deeper than just the name and it goes into the fact I could make change to dev inspect not realizing I am now affecting dry run.
In other words I would love to understand how other people (particularly those that have thought about that space for much longer than me) see the relationship between dev inspect and dry run, now and moving forward.
I may be also looking at this the wrong way so please correct me if so

[jordanjennings-mysten]

Also this PR would bring JSON-RPC in line with how GraphQL works.
https://github.com/MystenLabs/sui/blob/main/crates/sui-graphql-rpc/src/types/query.rs#L179
there appears to be an DevInspect.into() -> DryRunResponse so it might be good to do the swap to devinspect call in the api layer?

[amnn]

I think the performance concerns are interesting and worth digging into. Re: dryRun vs devInspect, the main thing for me is what Jordan mentioned above -- GraphQL implements dryRunTransactionBlock via a devInspect call that has the option to skip checks or not (i.e. it combines the two endpoints, but introduces a parameter), so in GraphQL, this conflation already exists.

The reason to do this was to give us a way to expose richer errors during dry-run (for PTBs in particular), by building on top of ExecutionMode -- the thinking was that most transactions go through a dry-run before they execute, so this is a good way to expose people to higher quality errors, but curious to hear thoughts from the cc'ed folks.

[tnowacki]

Could you speak a bit more to the motivation here? This is potentially a really big regression for folks, since it will increase the space for transactions that will pass "dry run" but then fail at execution.
For example, someone could do something they feel is harmless, like construct some user defined struct or enum from CallArg::Pure. Dev inspect will gladly let you do this, but dry run (and in turn normal execution), will give you an error. There is a very long list of this sort of differing behavior.

[tnowacki]

I forgot to refresh before making this comment and now see all the other comments above. Reading.

[tnowacki]

I think the performance concerns are interesting and worth digging into. Re: dryRun vs devInspect, the main thing for me is what Jordan mentioned above -- GraphQL implements dryRunTransactionBlock via a devInspect call that has the option to skip checks or not (i.e. it combines the two endpoints, but introduces a parameter), so in GraphQL, this conflation already exists.

Could you explain more what "the option to skip checks" entails?

[tnowacki]

Also this PR would bring JSON-RPC in line with how GraphQL works. https://github.com/MystenLabs/sui/blob/main/crates/sui-graphql-rpc/src/types/query.rs#L179 there appears to be an DevInspect.into() -> DryRunResponse so it might be good to do the swap to devinspect call in the api layer?

My first reaction to this news is that we have done a very mean thing to our GraphQL users, but I would be interested in learning more (especially with this "the option to skip checks" thing I don't understand)

[amnn]

My first reaction to this news is that we have done a very mean thing to our GraphQL users, but I would be interested in learning more (especially with this "the option to skip checks" thing I don't understand)

In GraphQL, there's a single combined dryRunTransactionBlock query, that combines the features that we used to offer via dry-run and dev-inspect:

• It can accept a TransactionData or a TransactionKind.
• It has a skipChecks flag which allows for running the transaction without the normal ownership checks etc (like how dev inspect works).
• Owing to how GraphQL works, users can opt in to receiving the intermediate results of commands or not.

There are some more details in the RPC 2.0 RFC: #13700, particularly this part.

We did this because people were regularly confused by the difference between dev-inspect and dry-run, and this was made worse by the fact that there were configurations that didn't work, that we did not have great reasons for, like:

• Why do we not allow people to dev inspect a transaction if we provide it as TransactionData?
• Why can't we inspect intermediate results of commands with checks enabled?
• Why can't we simulate a transaction kind, but again, with checks enabled?

The commonality is that none of these transactions' effects are committed to the chain, so we ended up retaining the dryRun nomenclature. In the end, this will probably evolve one more time into resolve/simulate, which addresses another issue of needing to use multiple round trips before a transaction is ready, and I think the naming is more standard as well, so hopefully less confusing.

[tnowacki]

I should probably learn to shut up a bit more when I haven't looked at code that I wrote more than a year ago.
The skip_checks flag seems to doing the right thing in AuthorityState::dev_inspect_transaction_block, and so I trust y'all if you think this will do the exact same behavior as dry-run today. (even though skip_checks is a bit lacking as a name, I've never really thought of Move's type system as a "check" 😝)

I was about to approve this but... it looks like we don't have any tests for the behavior of dry-run in sui-adapter-transactional-tests. We probably want to add them before landing this to make sure we don't break anything. I would probably test

• All sorts of Gas craziness
• Object owner checks
• arbitrary args via bytes
• reference return values
• entry taint checks (if you want to lose your mind)
• function visibility/entry check bypassing
• conservation checks

I wouldn't be surprised if some of these tests exist in the integration test land, but I would be surprised if it was this exhaustive.

Do such tests exist for Graph QL?

[jordanjennings-mysten]

there appear to be a few related to graphql.

Running tests/cli_tests.rs (target/debug/deps/cli_tests-50a62d76257781da)
test_dry_run: test

Running unittests src/lib.rs (target/debug/deps/sui_core-c73fbdd212dad4b5)
authority::authority_tests::test_dry_run_dev_inspect_dynamic_field_too_new: test
authority::authority_tests::test_dry_run_dev_inspect_max_gas_version: test
authority::authority_tests::test_dry_run_no_gas_big_transfer: test
authority::authority_tests::test_dry_run_on_validator: test
authority::authority_tests::test_dry_run_transaction_block: test
authority::authority_tests::test_for_inc_201_dry_run: test

Running tests/traffic_control_tests.rs (target/debug/deps/traffic_control_tests-894638d11d538e15)
test_fullnode_traffic_control_dry_run: test
test_validator_traffic_control_dry_run: test

**Running unittests src/lib.rs (target/debug/deps/sui_graphql_rpc-04aacd5897d6fa6e)**
server::builder::tests::test_payload_dry_run_exceeded: test
server::builder::tests::test_payload_inline_fragment_dry_run_exceeded: test
server::builder::tests::test_payload_multiple_dry_run_exceeded: test
server::builder::tests::test_payload_named_fragment_dry_run_exceeded: test
server::builder::tests::test_payload_reusing_vars_dry_run: test
server::builder::tests::test_payload_using_vars_dry_run_exceeded: test
server::builder::tests::test_payload_using_vars_dry_run_read_exceeded: test

Running tests/e2e_tests.rs (target/debug/deps/e2e_tests-74e3ae0c9e805fc2)
test_dry_run_failed_execution: test
test_transaction_dry_run: test
test_transaction_dry_run_with_kind: test

Running tests/balance_changes_tests.rs (target/debug/deps/balance_changes_tests-e4dd84a09e4c3d45)
test_dry_run_publish_with_mocked_coin: test

[tnowacki]

there appear to be a few related to graphql.

I don't think those tests are stressing the semantic difference between dry-run and dev-inspect though, which is my concern with the change in this PR.

[amnn]

Yeah, I don't think we have tests that would expose exactly this kind of dry-run vs dev-inspect strangeness in GraphQL (Today it's a little tough to write GraphQL transactional tests that involve building transactions to execute through GraphQL's dryRunTransactionBlock/executeTransactionBlock, so we are light there)

It would probably be better to have those tests closer to the authority codebase where those differences are maybe more apparent? (In GraphQL we are already living a post-dryRun world too).

[tnowacki]

It would probably be better to have those tests closer to the authority codebase where those differences are maybe more apparent? (In GraphQL we are already living a post-dryRun world too).

I was imagining having the tests in sui-adapter-transactional-tests, yeah.

[jordanjennings-mysten]

in sui-core/src/unit_tests/authority_tests.rs

existing

• test_dry_run_no_gas_big_transfer : gas testing which I believe is to ensure you can dry run to check gas requirements
• test_dry_run_dev_inspect_max_gas_version: is an example of a successful TX against dry

I added

• test_dry_run_no_arbitrary_functions: covers both a failed dry run and one which fails from calling a private function
• test_dry_run_invalid_object_ownership: checks you cannot violate ownership during dry run, I think this covers arbitrary args via bytes? it definitely covers
• test_dry_run_publish

I'm going to sift through sui-adapter-transactional-tests some of these tests might make more sense there.

My main questions now are:
other than breaking ownership what is the "via bytes" part of arbitrary args? is this related to arbitrary values flag on mode?
https://github.com/MystenLabs/sui/blob/main/sui-execution/latest/sui-adapter/src/execution_mode.rs#L209
I was able to test this flag with test_dry_run_invalid_object_ownership.

reference return values I think are pretty straight forward specifically checking for this I think:

public fun create_and_return_ref(): &u64 {
    let s = 0;
    &s
}

while I do see the flag for conservation checks and understand what it does, how you are suppose to violate it in a test I'm not sure of. I figure you need to break a few things in core parts of the code before that check comes into play since that would mean you were able to write a tx that inflates sui right? Can't find any examples of this in the code.