Skip to content

Reduce state/tipset/msg execution/replay/compute APIs in /v2 #13028

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rvagg opened this issue Apr 11, 2025 · 9 comments
Open

Reduce state/tipset/msg execution/replay/compute APIs in /v2 #13028

rvagg opened this issue Apr 11, 2025 · 9 comments

Comments

@rvagg
Copy link
Member

rvagg commented Apr 11, 2025

Breaking this out from #13027 where I was starting to ramble a bit too much and departing from the issue at hand.


Given we are working on /v2, there's an opportunity to consolidate our msg execution/replay APIs.

We have StateReplay and StateCall and StateCompute today and they all do roughly the same thing but with minor variations, and the internals are becoming very messy to support the variations while keeping backward compatibility, made more complicated by the needs of the Ethereum APIs which want to do similar things.

@Stebalien and I were discussing these the other day trying to boil them down to a single method that we can overload with an options struct argument to support the variations we care about. Then we can push those options down into the core and clean things up a bit.

In #12822 I've replaced a use of StateReplay with StateCall and I'm having to introduce third argument to it just to get my new option in, but in that case I'm introducing an optional struct argument (optional so it's backward compatible as JSONRPC) so I can get the new option in.

callInternal already illustrates the problem if you look both at the number of args, and the number of branches in it:

func (sm *StateManager) callInternal(ctx context.Context, msg *types.Message, priorMsgs []types.ChainMsg, ts *types.TipSet, stateCid cid.Cid,
- that was most recently complicated by needing a new "strategy" for Ethereum APIs.

Then, if you look at how StateReplay works, you see that it's basically the same thing as StateCall, but with a "execute everything up to this msg" strategy: https://github.com/filecoin-project/lotus/blob/6f09aca67ac53f65004751f55e6aa3c86b349153/chain/stmgr/call.go#L295-L311

StateCompute is something like a StateCall but with an batch of messages and a "apply all messages, then apply these new ones" strategy but also return the new state root.

Right now we're considering at adding a state root to InvocResult as part of solving #13022, which would make ComputeStateOutput redundant.

So, I think we could introduce something like:

StateExecute({
  BaseTipSet, // mandatory
  []ImplicitMessages, // optional, could be used to solve some of our gas estimation bugs
  []Messages, // optional
  TipSetMessageSelectionStrategy, // optional, default to "all", could also be "none", "same sender"
  FlushType, // optional, defaults "none", could be "state", or "all"
})
@github-project-automation github-project-automation bot moved this to 📌 Triage in FilOz Apr 11, 2025
@rvagg
Copy link
Member Author

rvagg commented Apr 11, 2025

Here's another fun factoid: it looks like all of our current uses of StateCompute in lotus use the same height argument as the tipset they are calling with:

if _, err := api.StateCompute(ctx, startTs.Height(), nil, startTs.Key()); err != nil {

ts, err := api.ChainGetTipSetByHeight(ctx, height, endTs.Key())
if err != nil {
return err
}
if _, err := api.StateCompute(ctx, height, nil, ts.Key()); err != nil {
return err

if _, err := api.StateCompute(ctx, endTs.Height(), nil, endTs.Key()); err != nil {

compute, err := api.StateCompute(ctx, dlEndTs.Height(), nil, dlEndTs.Key())

cso, err := clientApi.StateCompute(ctx, currTs.Height(), nil, currTs.Key())

st, err := api.StateCompute(ctx, execTs.Height(), nil, execTsk)

In all of the above, it's only the last one where we care about the Root return value, so the others could probably be easily replaced with a variation of StateCall or StateReplay.

lotus state compute-state is the only place where you can differ the height from the tipset, but it exists as a CLI form of the API, so that makes sense.

@rvagg
Copy link
Member Author

rvagg commented Apr 11, 2025

Something else to consider here - StateCompute does lean on an execution trace cache, because the strategy involves re-executing a complete tipset (even if you're going to apply new msgs on top), it can cache old ones to make it more efficient. It only defaults to 16 because they can be large objects, but I think Glif might bump that up in production. But that could easily be part of some new internal form of this thing - "if we're doing all messages in this tipset, use a cache".

@masih
Copy link
Member

masih commented Apr 11, 2025

it looks like all of our current uses of StateCompute in lotus use the same height argument as the tipset they are calling with

This is consistent with what I observed in Zondax codebase and the rationale for removing height from StateCompute implementation in #13027

@rvagg
Copy link
Member Author

rvagg commented Apr 11, 2025

OK, so it just becomes the same as a full-messages StateReplay but gives you the root state CID in the return, which is nice, but it should just be in InvocResult rather than having a special ComputeStateOutput to contain it.

masih added a commit that referenced this issue Apr 11, 2025
To unblock progress in making incremental progress at v2 apis I am
removing the implementation of StateSimulate and StateCompute in light
of discussions at #13028.
masih added a commit that referenced this issue Apr 11, 2025
Implement the Lotus V2 APIs for :

* `StateGetActor` - enhanced to accept tipset selector.
* `StateGetID` - enhanced to accept tipset selector / renamed from
  `LookupID` in v1 for consistency.

Tests are added to exercise the end-to-end API call through JSON rpc
with the raw wire format of the JSON-RPC request. Additional factory
functions are added for `TipSetLimit`, a new concept to specify a limit
for tipsets either as absolute height or relative to some
tipset selector. The future State message search APIs in v2 aim to use
Limit in upcoming PRs.

To unblock progress in making incremental progress at v2 apis I am
removing the implementation of StateSimulate and StateCompute in light
of discussions at #13028.
@Kubuxu
Copy link
Contributor

Kubuxu commented Apr 11, 2025

StateCompute with explicit height, afaik came into existence for null-epoch and migration testing.

@Stebalien
Copy link
Member

Why split implicit messages from messages? IMO, we should just let you pass arbitrary messages from any sender (ideally by CID OR By explicitly specifying a message to execute).

TipSetMessageSelectionStrategy

We also need a way to say "and include all messages from this same sender in the message pool". Or even include all messages from these specific senders? All messages to the recipient?

It would be really nice to have a complex "message-inclusion-strategy" query here: {SameSender: bool, SameRecipient: bool, MessagePool: bool, TipSet: bool}.

(we can start with the simplest version, but I'd structure this as an object we can extend over time)

Also: does "all" mean "including implicit messages"? We probably need to distinguish between "all messages" and "all all".

debugging

I'd also add a "debug actors" option that enables FVM debugging and returns the logs (embedded in the trace).

@Kubuxu
Copy link
Contributor

Kubuxu commented Apr 20, 2025

I don't think "from mpool" is a good idea, it is inherently racy and non-reproducible.

@masih
Copy link
Member

masih commented Apr 21, 2025

Note to implementer: Also see #13046

@Stebalien
Copy link
Member

I don't think "from mpool" is a good idea, it is inherently racy and non-reproducible.

I was thinking about things like gas estimation. But maybe that'll use a separate API? I'd prefer to have one API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📌 Triage
Development

No branches or pull requests

4 participants