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 ?

[rvagg]

Actually I think the correct thing to do is not loop because the request is for an explicit tipset, so we can't just adjust the tipset they want. I believe the right behaviour is to just check whether there's an expensive fork and error if not. Perhaps like this:

Suggested change

	if ts.Height() > 0 {
	for {
	pts, err := a.Chain.GetTipSetFromKey(ctx, ts.Parents())
	if err != nil {
	return nil, xerrors.Errorf("failed to find a non-forking epoch: %w", err)
	}
	if !a.StateManager.HasExpensiveForkBetween(pts.Height(), ts.Height()+1) {
	break
	}
	ts, err = a.Chain.GetTipSetFromKey(ctx, ts.Parents())
	if err != nil {
	return nil, xerrors.Errorf("getting parent tipset: %w", err)
	}
	}
	}
	if ts.Height() > 0 {
	pts, err := a.Chain.GetTipSetFromKey(ctx, ts.Parents())
	if err != nil {
	return nil, xerrors.Errorf("failed to find a non-forking epoch: %w", err)
	}
	// Check for expensive forks from the parents to the tipset, including nil tipsets
	if a.StateManager.HasExpensiveForkBetween(pts.Height(), ts.Height()+1) {
	return nil, stmgr.ErrExpensiveFork
	}
	}

Then the call just won't work for those major upgrade epochs. I don't know if that's going to cause complaints but it's only a few every year.

[aarshkshah1992]

@rvagg But the pre-existing behaviour does loop back till it can find an appropriate tipset. I am not sure we should change that behaviour.

[rvagg]

only if ts is not supplied; but I think here we always have a ts so we're in the else part of this block:

lotus/chain/stmgr/call.go

Lines 104 to 132 in f75bdd1

	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
	}
	}