fix: ETH RPC API: ETH Call should use the parent state root of the subsequent tipset

[aarshkshah1992]

Related Issues

Closes #11205

Proposed Changes

ETH Call should use the parent state root of the subsequent tipset. This can be obtained from the TipsetState on the Statemanager

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

• Commits have a clear commit message.
• PR title is in the form of of <PR type>: <area>: <change being made>
  • example: fix: mempool: Introduce a cache for valid signatures
  • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
  • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
• If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
• New features have usage guidelines and / or documentation updates in
  • Lotus Documentation
  • Discussion Tutorials
• Tests exist for new functionality or change in behavior
• CI is green

[jennijuju]

@aarshkshah1992 - can you please assign a reviewer?

[Stebalien]

Well, this is bringing back past nightmares with respect to #10216.

[aarshkshah1992]

@Stebalien Are you saying that this wont work ?

[rvagg]

Working through the logic here - TipSetState will end up executing the messages on top of parent state if the requested tipset doesn't already have a height+1 not on a different fork that we can use to get the state from.

When you get down to the bottom of tipSetState(), having failed to find a valid existing height+1 state, you run ExecuteTipSet, which itself will call HandleStateForks which the callInternal() code above in this PR does so a bunch of work to avoid calling if it can tell that there's an "expensive" migration.

So we might need to port that logic down to here to handle the expensive-migration case.

But other than that, I think this results in roughly the same outcome of having all current tipset messages applied to produce a new state and then applying only the requested msg on top of that.

I'm not sure if/how null rounds come in to play with all of this though, maybe it doesn't matter as per the discussion in #11205.

[aarshkshah1992]

@rvagg

Given that TipSetState already handles state forks for us, can we just not re-do it in callInternal for our use case ? We can control that behaviour in callInternal with a flag.

[rvagg]

already handles state forks for us,

but that's the problem, imagine if we did this across an expensive migration boundary and triggered a migration just to get the state at this tipset.

All of this logic exists to check that condition:

lotus/chain/stmgr/call.go

Lines 104 to 132 in 187e837

	if ts == nil {
	ts = sm.cs.GetHeaviestTipSet()
	// Search back till we find a height with no fork, or we reach the beginning.
	// We need the _previous_ height to have no fork, because we'll
	// run the fork logic in `sm.TipSetState`. We need the _current_
	// height to have no fork, because we'll run it inside this
	// function before executing the given message.
	for ts.Height() > 0 {
	pts, err = sm.cs.GetTipSetFromKey(ctx, ts.Parents())
	if err != nil {
	return nil, xerrors.Errorf("failed to find a non-forking epoch: %w", err)
	}
	// Checks for expensive forks from the parents to the tipset, including nil tipsets
	if !sm.hasExpensiveForkBetween(pts.Height(), ts.Height()+1) {
	break
	}
	ts = pts
	}
	} else if ts.Height() > 0 {
	pts, err = sm.cs.GetTipSetFromKey(ctx, ts.Parents())
	if err != nil {
	return nil, xerrors.Errorf("failed to find a non-forking epoch: %w", err)
	}
	if sm.hasExpensiveForkBetween(pts.Height(), ts.Height()+1) {
	return nil, ErrExpensiveFork
	}
	}

But that's only applied after you've potentially executed that tipset because TipSetState can result in an ExecuteTipSet if it doesn't already have it handy, that executes ApplyBlocks which in turn runs HandleStateForks.

So I think, if you run this, it'll still do what it says in the comment at the top of the for ("Try calling until we find a height with no migration."), but it'll actually perform a migration before it checks whether there's an expensive migration.

I believe what we want is to run an hasExpensiveForkBetween(pts.Height(), ts.Height()+1) ourselves and walk back up parents until it's false. But currently hasExpensiveForkBetween is a private method of StateManager.

[aarshkshah1992]

Ahaa good catch @rvagg I have fixed this.

[aarshkshah1992]

Hey @raulk

This PR needs a review from someone who understands the ETH RPC stack really well. But @Stebalien is fully focused on F3 currently. Would you or Mikers or Fridrick be able to help review this by any chance ?

[aarshkshah1992]

@rvagg Have addressed both your review comments. Please can you take one final look at the PR ?

[momack2]

@rvagg @aarshkshah1992 can we get a status update here? What's needed to get this fully reviewed?

[raulk]

I'm confused. Unless I'm misreading this, wouldn't this make the behaviour non-deterministic for an Eth user agent? An Eth wallet/library/dapp can't know if a given Filecoin epoch happened to run an "expensive fork". How is it even useful for a user agent to have its message applied on an epoch prior to the requested epoch?

[aarshkshah1992]

@raulk Have removed this. If the user specifies a tipset that causes an expensive fork, we simply error out.

Note: The erroring out on an expensive fork is pre-existing behaviour that we are not changing with this PR.

[rvagg]

@momack2 just waiting on expert eyes to review so the blind don't lead the blind down a dodgy alleyway

[github-actions]

All checks have completed

❌ Failed Test / Test (itest-gateway) (pull_request)
❌ Failed Test / Test (itest-path_type_filters) (pull_request)

[aarshkshah1992]

@raulk I agree. We have removed the looping so if the user specifies a tipset that can cause an expensive fork, we just error out the request(should be few of those as Rodd mentions).

[aarshkshah1992]

@momack2 We were waiting on a review from someone who understands the ETH<>Filecoin stack well. Now that Raul has reviewed it, we can land this. Have addressed the latest round of review. Will merge on a green tick.

[rvagg]

I wouldn't mind hearing @raulk's response to the fact that we error on a migration epoch if he has time to chime in. But, it's also something that can be easily changed at a future date if we hear objections after this lands.
It might be good to bring up in a sync meeting actually, just that specific point.
Otherwise this lgtm.