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

feat(rpc): internal F3 RPC methods to power go-f3 sidecar #4646

Merged
merged 10 commits into from
Aug 19, 2024
Merged

Conversation

hanabi1224
Copy link
Contributor

@hanabi1224 hanabi1224 commented Aug 16, 2024

Summary of changes

Part of #4644

To construct a go-f3 node, an EC(expected consensus) backend is required. When we run a f3 node as sidecar, we cannot directly pass an EC backend, instead, we expose a set of RPC methods on forest side, and implement a go EC backend with RPC client.

Some design considerations:

  • performance may or may not be an issue. We start from naive json data serialization and optimize it later.
  • implementing RPC in the P2P layer might be over complicated and harder to debug or troubleshoot
// EC backend interface defined in `go-f3`
type Backend interface {
	// GetTipsetByEpoch should return the tipset immediately before the one requested.
	// If the epoch requested is null, it returns the latest not-null one.
	GetTipsetByEpoch(ctx context.Context, epoch int64) (TipSet, error)
	// GetTipset returns the tipset with the given key.
	GetTipset(context.Context, gpbft.TipSetKey) (TipSet, error)
	// GetHead returns the current head tipset of the chain
	GetHead(context.Context) (TipSet, error)
	// GetParent returns the parent of the current tipset.
	GetParent(context.Context, TipSet) (TipSet, error)
	// GetPowerTable returns the power table at the tipset given as an argument.
	GetPowerTable(context.Context, gpbft.TipSetKey) (gpbft.PowerEntries, error)
}

// lotus code for constructing a go-f3 node
	ec := &ecWrapper{
	ChainStore:   params.ChainStore,
	StateManager: params.StateManager,
}
verif := blssig.VerifierWithKeyOnG1()

module, err := f3.New(mctx, params.ManifestProvider, ds,
	params.Host, params.PubSub, verif, ec)

// with this change, we can construct EC backend for a sidecar f3 node like this
ec := ecrpcclient.New(forest_rpc_endpoint)

Changes introduced in this pull request:

  • implement F3.* RPC methods
  • API parity tests are not applicable
  • go EC implementation and tests (TODO: run go tests in CI)

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

@hanabi1224 hanabi1224 marked this pull request as ready for review August 19, 2024 01:09
@hanabi1224 hanabi1224 requested a review from a team as a code owner August 19, 2024 01:09
@hanabi1224 hanabi1224 requested review from LesnyRumcajs and elmattic and removed request for a team August 19, 2024 01:09
@LesnyRumcajs
Copy link
Member

go EC implementation and tests (TODO: run go tests in CI)

Let's have a tracking issue for that.

f3-sidecar/README.md Outdated Show resolved Hide resolved
f3-sidecar/README.md Show resolved Hide resolved
f3-sidecar/ec_test.go Outdated Show resolved Hide resolved
}

pub enum GetPowerTable {}
impl RpcMethod<1> for GetPowerTable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a pretty massive method. Can it be refactored?

Copy link
Contributor Author

@hanabi1224 hanabi1224 Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of duplicate code handling different variant of power state, v12-v14 has already been simplified with a macro. v8-v11 are all different. I guess I can update fil-actor-states to unify the API for v8-v11 to further simplify the code with a single macro, how about doing that in a subsequent PR since it requires a release of fil-actor-states

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it require a release or can we move that logic entirely to Forest's shim? In any case, I'm good with that as long as it's indeed a follow-up and not an abandonware issue. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I can move the entire thing to forest as well

src/rpc/methods/f3/types.rs Show resolved Hide resolved
@hanabi1224
Copy link
Contributor Author

Let's have a tracking issue for that.

@LesnyRumcajs #4652

@hanabi1224 hanabi1224 requested a review from ruseinov August 19, 2024 13:02
let ts = ctx.chain_index().load_required_tipset(&tsk)?;
let state: power::State = ctx.state_manager.get_actor_state(&ts)?;
let mut id_power_worker_mappings = vec![];
match &state {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: since this match is so long - perhaps it would be more readable to separate this into helper methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is tracked by ChainSafe/fil-actor-states#324

Copy link
Contributor

@ruseinov ruseinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me apart from a massive match block, but I don't consider it a blocker.

@ruseinov
Copy link
Contributor

I would introduce a CI job that runs the EC tests too, perhaps a different PR is in order, not sure how much of a priority that is though.

@hanabi1224
Copy link
Contributor Author

hanabi1224 commented Aug 19, 2024

I would introduce a CI job that runs the EC tests too, perhaps a different PR is in order, not sure how much of a priority that is though.

@ruseinov Will do that in a subsequent PR since running the Go tests is a bit more complicated than normal as they depend on a running forest node.

@hanabi1224 hanabi1224 added this pull request to the merge queue Aug 19, 2024
Merged via the queue into main with commit ce50f0a Aug 19, 2024
30 checks passed
@hanabi1224 hanabi1224 deleted the hm/f3-rpc branch August 19, 2024 14:30
@hanabi1224 hanabi1224 mentioned this pull request Aug 20, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants