filecoin-project / builtin-actors

The Filecoin built-in actors
Other
83 stars 79 forks source link

Client State Access #194

Closed Stebalien closed 1 year ago

Stebalien commented 2 years ago

Clients need to be able to "read" actor state for:

Currently, clients "decode" the underlying state by importing a struct definition.

Options

  1. Continue importing language-specific state definitions. Possibly extracting the rust state definitions to a new repo? Possibly defined externally via IPLD schemas?
  2. Add state access methods to actors.
anorth commented 2 years ago

I've talked about this a bit with @frrist and @ZenGround0: Sentinel (and chain explorers generally) is another party that reads chain state. I basically think we should do both.

  1. Is going to be necessary for deep state inspection of the type that Sentinel uses. They won't be able to tolerate waiting for abstractions, so will end up with these definitions anyway. IMO we should lean into this and elevate state access libraries to first-class maintained things (but expect a lot of the maintenance efforts to come from the users of such libraries: node/client impls, Sentinel, chain explorers etc). I think this should be a separate Go repo (not specs-actors) and similarly separate Rust repo. These libraries do not need to follow the same versioning approaches that the actors do: e.g. one "release" might contain packages that can read all the actors versions up to that point, and maybe the abstraction layer over them.
  2. We should do for other clients like end users. This is just a change to our patterns of writing contracts: now that we don't know who all the callers are, authors should endeavour to expose all useful read-only information abstractly. But don't expect this to be 100% comprehensive.
raulk commented 2 years ago

I agree with your assessment here, @anorth. However, I'd expand the scope beyond state access (architecturally equivalent to DAOs for old dogs like me), to cover also method inputs and outputs (architecturally equivalent to DTOs).

There is a whole set of transaction submitters (e.g. wallets, miner management applications, deal-making apps, DataCap/notary management apps, etc.) for whom having parameter and return objects available in their languages to serde to/from is important.

@rllola has been doing some early prototyping, and is feeling the pain of rust serde.

anorth commented 2 years ago

@Stebalien I'm not sure there's any action to take here. The go-state-types repo is where the Go solution goes. Rust users can import this repo (though we should incrementally add better state access functions). Shall we close this?

Stebalien commented 2 years ago

Yeah. I don't think we're really getting anything from this issue at the moment.

raulk commented 2 years ago

Since we are not releasing the actors themselves, just the bundle, Forest no longer has access to the latest state and DTO objects, so this becomes relevant and necessary again @lemmih

anorth commented 1 year ago

Rust users can import this repo

This won't cut it as we have multiple versions of built-in actor states going forward #835. So in that case I think we should replicate the go-state-types pattern here, copy (not move) the state and param/response definitions out to a new rust repo, and maintain multiple versions in different crates in that repo.

lemmih commented 1 year ago

Summary of the discussion in #835:

  1. The crates in the builtin-actors repository only exists to create WASM bundles and should not be directly used by anyone.
  2. The Forest team will fork the builtin-actors and provide slimmed-down versions that do not contain any of the smart-contract code. The forked actors will still contain the state structures and all business logic required for validating consensus.
  3. The forked code will live in the Forest repository but may be moved at a later date.

My comments:

  1. Duplicating the business logic of the actors is unwise. It will not contribute to network robustness. There may be some value in duplicating the logic in Go but copy&pasting the code in Rust is, in my opinion, a mistake.
  2. I don't think it's reasonable to expect the developers of the builtin-actors to propagate their changes into the slimmed-down actors. When push comes to shove, Forest will be responsible for keeping the duplicated code in sync.
  3. While duplicating code is a bad solution, it's better than no solution and Forest would like to move forward with this.

As a side note, for me, this discussion is similar to our discussion about semantic versioning. I assured you that violating semantic versioning would cause trouble (which it did) and now again I'm assuring you that copy&pasting the actor logic will cause trouble. Probably not for Lotus but for everyone else in the ecosystem. We managed to get on the same page with respect to semantic versioning so hopefully we'll end up on the same page regarding having a single source of truth.

This summary is my understanding of a discussion that borders on the philosophical. Any misrepresentation is accidental and I genuinely want to understand the full picture.

anorth commented 1 year ago

Thanks @lemmih.

Duplicating the business logic of the actors is unwise

The example of miner_nominal_power_meets_consensus_minimum is actually just about to be exported as an actor method. I don't know if it's going to be reasonable for you to invoke the VM where you need this value, but duplication at this point will be an optimisation rather than necessity.

Where there is other business logic that is written in actors but invoked via direct state inspection elsewhere, we might also be able to export those as actor methods. Please do let us know what they are as you discover them.

I don't think it's reasonable to expect the developers of the builtin-actors to propagate their changes into the slimmed-down actors

Assuming they're slimmed down to basically just state objects and params/returns, please at least ask us to!

anorth commented 1 year ago

@lemmih is https://github.com/ChainSafe/fil-actor-states sufficient for us to close this issue?

lemmih commented 1 year ago

@lemmih is https://github.com/ChainSafe/fil-actor-states sufficient for us to close this issue?

Yes, our fork satisfies our needs. I wouldn't mind if you closed this issue. Might be appropriate to close it as wont-fix.