Skip to content
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

Client State Access #194

Closed
Stebalien opened this issue Apr 6, 2022 · 10 comments
Closed

Client State Access #194

Stebalien opened this issue Apr 6, 2022 · 10 comments
Labels
wontfix This will not be worked on

Comments

@Stebalien
Copy link
Member

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

  • Chain validation.
  • Submitting proofs.
  • Window post dispute.

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

  • Lotus/Venus import each version of specs-actors and abstract over all versions. Going forward, this we'd need to replicate state changes from here to specs-actors to make this work.
  • Forest imports this package. Going forward, this is going to be tricky as it means backporting changes to, e.g., fvm_shared to prior actor releases. We can technically do this as long as we don't change the version used on the network... but we'd still rather not "maintain" prior releases like this.
  • The FVM re-defines the state types for the account, init, market, power, reward, and system actors.

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.
  • The first one is the most flexible but still requires maintaining abstractions over multiple actor versions.
  • The second option is the "cleanest" as it pushes the abstraction into the actors themselves. However, it's the least flexible as adding new functions requires a chain upgrade.
@anorth
Copy link
Member

anorth commented Apr 7, 2022

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
Copy link
Member

raulk commented Apr 7, 2022

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
Copy link
Member

anorth commented Aug 11, 2022

@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
Copy link
Member Author

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

@raulk
Copy link
Member

raulk commented Oct 20, 2022

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
Copy link
Member

anorth commented Nov 14, 2022

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
Copy link
Contributor

lemmih commented Nov 16, 2022

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
Copy link
Member

anorth commented Nov 16, 2022

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
Copy link
Member

anorth commented Mar 3, 2023

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

@lemmih
Copy link
Contributor

lemmih commented Mar 9, 2023

@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.

@anorth anorth added the wontfix This will not be worked on label Mar 9, 2023
@anorth anorth closed this as completed Mar 9, 2023
tchataigner added a commit to polyphene/kythera that referenced this issue Apr 5, 2023
* Removed dependencies to builtin actors to rely on Chainsafe repo instead as per filecoin-project/builtin-actors#194 (comment)
tchataigner added a commit to polyphene/kythera that referenced this issue Apr 7, 2023
## Description

The main goal of this PR is to properly deploy all necessary builtins to
our local execution environment, at the address they are expected to be
based on the [builtin actors
repository](https://github.com/filecoin-project/builtin-actors/blob/master/runtime/src/builtin/singletons.rs).

Aside from this, we also removed direct references to the builtins
crates as per [this
comment](filecoin-project/builtin-actors#194 (comment)),
only relying on it for the WASM bundle. Instead, to instantiate builtins
and their state we use crates from [Chainsafe
repository](https://github.com/ChainSafe/fil-actor-states).

Moreover, we now use `frc42_dispatch` crate in test actors. Closes #42 

Finally, we removed dependencies on the `ref-fvm` repository to instead
be based on crates.io released version.

## Notes

We should write some simple tests to check some flows that leverages
builtin actors in the future (e.g.: Market actor, miner actor ...).

Added issue in backlog of the project:
https://github.com/orgs/polyphene/projects/1/views/3?pane=issue&itemId=24909149

## Links to any relevant issues or pull requests

- #42

## Open Questions

<!-- Unresolved questions, if any. -->

## Change checklist

- [x] I have performed a self-review of my own code
- [ ] I have made corresponding changes to the documentation
- [x] I have added tests that prove my fix is effective or that my
feature works

---------

Co-authored-by: PhilippeMts <[email protected]>
Co-authored-by: João Oliveira <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

4 participants