add simulateTransaction endpoint #1610

tsachiherman · 2024-09-30T18:50:24Z

What's in this PR ?

This PR adds the simulateTransaction endpoint to the VM API.

What are the changes in this PR ?

The Recorder struct from the vmwithcontracts example was moved into the core of the hyper-sdk.
Added simulateTransaction endpoint to the API server/client

What is left and still need to be implemented in a subsequent PR ?

The existing support of Simulate in the vmwithcontracts remained as is. Removing it while taking advantage of the above would require some additional work that wasn't captured in this PR.

aaronbuchwald · 2024-09-30T22:04:02Z

chain/dependencies.go

+ // If any keys are touched during [Execute] while running in [OnchainTransactionExecution] mode that
+ // are not specified in [StateKeys], the transaction will revert and the max fee will be charged.


Hmm this comment was actually not true before. If a state key is read that was not specified, it's up to the action to decide whether to propagate it (usually should).

Could we revert this if we're able to remove OnchainTransactionExecution mode?

aaronbuchwald · 2024-10-01T01:46:21Z

examples/vmwithcontracts/vm/server.go

@@ -101,7 +101,7 @@ func (j *JSONRPCServer) simulate(ctx context.Context, t actions.Call, actor code
 if err != nil {
 return nil, 0, err
 }
- recorder := storage.NewRecorder(currentState)
+ recorder := state.NewRecorder(currentState)


Can we remove this from vmwithcontracts/ since it can use the general purpose API after this PR?

aaronbuchwald · 2024-10-01T01:53:10Z

api/jsonrpc/server.go

+func (j *JSONRPCServer) SimulateTransaction(
+ req *http.Request,
+ args *SimulateTransactionArgs,
+ reply *SimulateTransactionReply,
+) error {


Could we start with simulating a single action and providing the additional arguments as needed (actor) ?

In our case, transactions include additional data that we don't need to require from the user. For example, with this API you can only simulate an action from a given address if you can generate a signature from that address.

aaronbuchwald · 2024-10-01T01:54:17Z

genesis/rules.go

@@ -152,6 +152,10 @@ func (*Rules) FetchCustom(string) (any, bool) {
 return nil, false
 }

+func (*Rules) GetTransactionExecutionMode() chain.TransactionExecutionMode {


Is there a reason that we need to add this mode to Rules rather than just tracking what state keys were touched by executing an action?

If we just wrap the current view produced by the VM and run Execute against it, I don't think that we should need to add key tracking within vmwithcontracts at all and can remove this extra function on Rules.

aaronbuchwald · 2024-10-01T01:56:01Z

api/jsonrpc/server.go

+type SimulateTransactionArgs struct {
+ Tx []byte `json:"tx"`
+}
+
+type SimulatedAction struct {
+ Output []byte `json:"output_b64"`
+ Error *string `json:"error"`
+}


Could we use codec.Bytes in API responses when passing []byte values to provide a reasonable JSON output?

aaronbuchwald · 2024-10-01T01:57:14Z

api/jsonrpc/server.go

+type SimulateTransactionReply struct {
+ SimulatedActions []SimulatedAction `json:"simulatedActions"`
+ ReadKeys []string `json:"readKeys"`
+ WriteKeys []string `json:"writeKeys"`
+ AllocateKeys []string `json:"allocKeys"`
+ BlockHeight uint64 `json:"blockheight"`
+ Error string `json:"error"`
+}


Could we reduce the state key fields here down to just state.Keys and add support for JSON marshal/unmarshal directly to this type?

I'd prefer to remove BlockHeight and Error (in favor of returning the error directly and pushing to the client to handle it) unless there's a clear need to include them in the response.

aaronbuchwald · 2024-10-01T02:03:52Z

api/jsonrpc/server.go

+ if reply == nil {
+ return errors.New("SimulateTransaction was called with a nil reply object")
+ }


This should be provided by the server, so we can remove this check and follow the style in the rest of the APIs of not checking if the reply value is nil.

aaronbuchwald · 2024-10-01T02:05:02Z

api/jsonrpc/client.go

+type SimulatedActionResponse struct {
+ Output []byte
+ Error error
+}
+
+type SimulateTransactionResponse struct {
+ SimulatedActions []SimulatedActionResponse
+ ReadKeys []string
+ WriteKeys []string
+ AllocateKeys []string
+ BlockHeight uint64
+}


Can we re-use the types specified in server.go as we do for the existing server/client implementations?

…ersdk into tsachi/simulate_transaction

aaronbuchwald · 2024-10-03T12:41:21Z

state/recorder.go

@@ -50,10 +48,10 @@ func (r *Recorder) GetValue(ctx context.Context, key []byte) (value []byte, err
 return r.State.GetValue(ctx, key)
 }

-func (r *Recorder) GetStateKeys() state.Keys {
- result := state.Keys{}
+func (r *Recorder) GetStateKeys() Keys {


Ik this is just moving code, but we should make sure it's bug free since we're moving it out of examples.

During onchain execution, we use tstate and want the recorder to exactly replicate the permissions required to execute an action on top of tstate.TStateView.

tstate.TStateView will return an error if we EVER try to perform a read/write/allocate where we do not have permission.

The current Recorder overwrites the changedValue for a given key between insert/remove operations and then records the summation of these events at the end. This means that if the action inserts and then removes a key, it will not record it as a write operation even though during onchain execution, the first write will fail here:

hypersdk/state/tstate/tstate_view.go

Line 199 in 47849d8

if !ts.checkScope(ctx, key, state.Write) {

.

Could we update the recorder to make sure that it correctly handles this case and add unit tests for the happy path of tracking read/write/allocate + regression test for operations that may overwrite each other?

the above isn't the correct example, but you're correct in saying that it won't perform correctly in the way it's currently implemented.

aaronbuchwald · 2024-10-03T12:49:01Z

examples/vmwithcontracts/cmd/vmwithcontracts-cli/cmd/action.go

+ return err
+ }
+
+ rtx := codec.NewReader(encodedActionResults, actions.MaxResultSizeLimit) // will likely be much smaller than this


nit: we can set the max size to the size of the buffer when reading since we'll never read more than the length of the bytes we're reading

aaronbuchwald · 2024-10-03T12:51:30Z

examples/vmwithcontracts/actions/call.go

+
+// Marshal encodes a type as bytes.
+func (r *Result) Marshal(p *codec.Packer) {
+ p.PackBytes(r.Value)
+ p.PackUint64(r.ConsumedFuel)
+}


We should use the output parser

hypersdk/examples/vmwithcontracts/vm/vm.go

Line 51 in 47849d8

OutputParser.Register(&actions.Result{}, nil),

instead of adding explicit marshal functions here

aaronbuchwald · 2024-10-03T12:53:22Z

api/jsonrpc/client.go

+ stateKeys := state.Keys{}
+ for _, entry := range resp.StateKeys {
+ hexBytes, err := hex.DecodeString(entry.HexKey)
+ if err != nil {
+ return nil, nil, err
+ }
+
+ stateKeys.Add(string(hexBytes), state.Permissions(entry.Permissions))
+ }


Rather than adding special handling here - could we implement MarshalJSON and UnmarshalJSON on state.Keys so that we can use the type directly in the response?

aaronbuchwald · 2024-10-03T12:54:43Z

api/jsonrpc/server.go

+ if reply == nil {
+ return errors.New("SimulateAction was called with a nil reply object")
+ }


Could we remove this check? This is populated by the server, so there should be no need to check for a nil value here

aaronbuchwald · 2024-10-03T12:55:22Z

api/jsonrpc/server.go

+ }
+
+ actionRegistry := j.vm.ActionRegistry()
+ rtx := codec.NewReader(args.Action, consts.NetworkSizeLimit) // will likely be much smaller than this


nit: let's use len(args.Action) since we'll never read longer than the actual bytes provided

aaronbuchwald · 2024-10-03T12:55:47Z

api/jsonrpc/server.go

+ }
+
+ actionRegistry := j.vm.ActionRegistry()
+ rtx := codec.NewReader(args.Action, consts.NetworkSizeLimit) // will likely be much smaller than this


Is rtx for read transaction? Could we rename this since it's reading in action bytes now?

aaronbuchwald · 2024-10-03T12:56:42Z

api/jsonrpc/server.go

+ if len(actions) != 1 {
+ return fmt.Errorf("simulateAction expects a single action, %d found", len(actions))
+ }


Could we remove this check and support handling multiple actions?

aaronbuchwald · 2024-10-03T12:57:27Z

api/jsonrpc/server.go

+ for key, permission := range recorder.GetStateKeys() {
+ reply.StateKeys = append(reply.StateKeys, SimulateStateKey{HexKey: hex.EncodeToString([]byte(key)), Permissions: byte(permission)})
+ }


To support multiple actions, could we return a StateKeys output per action?

aaronbuchwald · 2024-10-03T12:57:57Z

api/jsonrpc/server.go

+ return err
+ }
+ }
+ for key, permission := range recorder.GetStateKeys() {


Can we also remove the special reply handling by implementing JSON marshalling on the state.Keys type directly?

tsachiherman added 2 commits September 30, 2024 14:40

initial version

45749b1

Merge branch 'main' into tsachi/simulate_transaction

23c8b1d

tsachiherman self-assigned this Sep 30, 2024

tsachiherman linked an issue Sep 30, 2024 that may be closed by this pull request

Create an API that enables you to simulate execution of an action and see what state keys it touches. #1552

Open

update

65df47b

tsachiherman marked this pull request as ready for review September 30, 2024 20:33

tsachiherman requested review from aaronbuchwald and joshua-kim as code owners September 30, 2024 20:33

linter

6fc64c9

aaronbuchwald requested changes Oct 1, 2024

View reviewed changes

update

693b7c0

tsachiherman marked this pull request as draft October 1, 2024 19:57

tsachiherman added 6 commits October 1, 2024 16:00

rollback unwanted changes

0da1d84

correct typo

1a2e340

Merge branch 'main' into tsachi/simulate_transaction

8253c81

add encoding to state keys.

7d26831

update

75c07e6

Merge branch 'main' into tsachi/simulate_transaction

a306b46

tsachiherman marked this pull request as ready for review October 2, 2024 13:30

tsachiherman added 2 commits October 2, 2024 09:34

remove comments

5379398

Merge branch 'tsachi/simulate_transaction' of github.com:ava-labs/hyp…

ff86ecf

…ersdk into tsachi/simulate_transaction

aaronbuchwald requested changes Oct 3, 2024

View reviewed changes

tsachiherman added 2 commits October 3, 2024 10:21

Merge branch 'main' into tsachi/simulate_transaction

f42b284

add keys marshaler

c08b8cf

tsachiherman mentioned this pull request Oct 3, 2024

refactor marshalActions implementation #1631

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add simulateTransaction endpoint #1610

add simulateTransaction endpoint #1610

tsachiherman commented Sep 30, 2024 •

edited

Loading

aaronbuchwald Sep 30, 2024

aaronbuchwald Oct 1, 2024

tsachiherman Oct 1, 2024

aaronbuchwald Oct 1, 2024

tsachiherman Oct 3, 2024

aaronbuchwald Oct 1, 2024

tsachiherman Oct 1, 2024

aaronbuchwald Oct 1, 2024

tsachiherman Oct 1, 2024

aaronbuchwald Oct 1, 2024

tsachiherman Oct 1, 2024

aaronbuchwald Oct 1, 2024

aaronbuchwald Oct 1, 2024

tsachiherman Oct 1, 2024

aaronbuchwald Oct 1, 2024

tsachiherman Oct 1, 2024

aaronbuchwald Oct 1, 2024

tsachiherman Oct 1, 2024

aaronbuchwald Oct 3, 2024

tsachiherman Oct 4, 2024

aaronbuchwald Oct 3, 2024

aaronbuchwald Oct 3, 2024

aaronbuchwald Oct 3, 2024

tsachiherman Oct 3, 2024

aaronbuchwald Oct 3, 2024

tsachiherman Oct 3, 2024

aaronbuchwald Oct 3, 2024

tsachiherman Oct 3, 2024

aaronbuchwald Oct 3, 2024

tsachiherman Oct 3, 2024

aaronbuchwald Oct 3, 2024

aaronbuchwald Oct 3, 2024

aaronbuchwald Oct 3, 2024

		// If any keys are touched during [Execute] while running in [OnchainTransactionExecution] mode that
		// are not specified in [StateKeys], the transaction will revert and the max fee will be charged.

add simulateTransaction endpoint #1610

Are you sure you want to change the base?

add simulateTransaction endpoint #1610

Conversation

tsachiherman commented Sep 30, 2024 • edited Loading

What's in this PR ?

What are the changes in this PR ?

What is left and still need to be implemented in a subsequent PR ?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsachiherman commented Sep 30, 2024 •

edited

Loading