fix: eth: use the correct tipset for Eth APIs #10462

Stebalien · 2023-03-13T21:07:13Z

Related Issues

Proposed Changes

Many of our "state" methods look at the tipset's parent state. However, Eth APIs expect the current tipset. We had sort-of fixed this for balances, but nothing else.

This patch alters most Eth APIs to use the following tipset as their base instead of the requested tipset. This is now possible because the pending tipset is no longer allowed.

NOTE: this patch also adds "pending" support back to EthGetTransactionCount. I expect we actually need this...

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

Stebalien · 2023-03-13T21:11:20Z

This'll need a couple of tests but I wanted to get this up.

Stebalien · 2023-04-21T21:10:47Z

node/impl/full/eth.go

+// blkParamToTipsetAfter returns the next non-nil tipset after the given block param.
+func (a *EthModule) blkParamToTipsetAfter(ctx context.Context, blkParam string) (*types.TipSet, error) {
+	switch blkParam {
+	case "earliest", "pending":


Alternatively, we can reject "pending" in some specific cases, rather than allow it in some specific cases.

Many of our "state" methods look at the tipset's _parent_ state. However, Eth APIs expect the current tipset. We had sort-of fixed this for balances, but nothing else. This patch alters _most_ Eth APIs to use the following tipset as their base instead of the requested tipset.

Stebalien · 2023-08-24T16:38:29Z

node/impl/full/eth.go

@@ -274,6 +282,7 @@ func (a *EthModule) getTipsetByEthBlockNumberOrHash(ctx context.Context, blkPara
 	}

 	if blkParam.BlockHash != nil {
+		// TODO


We don't have a fast way to implement this.

Specifically, we don't have a fast way to implement this if the requested block is not on the main chain.

Stebalien · 2023-08-24T16:39:16Z

node/impl/full/eth.go

+			return 0, err
+		}
+		if !consensus.IsValidForSending(nv, actor) {
+			return 0, xerrors.Errorf("cannot lookup pending transaction count for non-accounts")


We could support this. But we'd need to call TipSetState which may compute the state.

Stebalien · 2023-08-24T16:39:36Z

node/impl/full/eth.go

@@ -709,17 +739,13 @@ func (a *EthModule) EthGetBalance(ctx context.Context, address ethtypes.EthAddre
 		return ethtypes.EthBigInt{}, err
 	}

-	ts, err := a.getTipsetByEthBlockNumberOrHash(ctx, blkParam)
+	// TODO: Consider supporting pending balance?


So, we were supporting "pending" balances. What will this break?

We were also randomly computing the tipset state. So maybe that's not an issue?

It will be cached.

We no longer have a hard parallelism limit like we did before.

We "lock" so we won't compute it multiple times in parallel.

Stebalien · 2023-08-24T16:40:40Z

node/impl/full/eth.go

-			return nil, xerrors.Errorf("getting parent tipset: %w", err)
-		}
-	}
+	res, err := a.StateManager.CallWithGas(ctx, msg, []types.ChainMsg{}, ts, false)


I'm not sure why we were retrying at different heights. We should just bail in this case.

Stebalien · 2023-08-24T16:40:56Z

node/impl/full/eth.go

@@ -1176,7 +1187,7 @@ func (a *EthModule) EthCall(ctx context.Context, tx ethtypes.EthCall, blkParam e
 		return nil, xerrors.Errorf("failed to convert ethcall to filecoin message: %w", err)
 	}

-	ts, err := a.getTipsetByEthBlockNumberOrHash(ctx, blkParam)
+	ts, err := a.getTipsetByEthBlockNumberOrHash(ctx, blkParam, true)


This "removes" support for applying on-top-of the "pending" tipset.

Stebalien · 2023-08-24T16:41:26Z

node/impl/full/eth.go

@@ -613,7 +643,7 @@ func (a *EthModule) EthGetCode(ctx context.Context, ethAddr ethtypes.EthAddress,
 }

 func (a *EthModule) EthGetStorageAt(ctx context.Context, ethAddr ethtypes.EthAddress, position ethtypes.EthBytes, blkParam ethtypes.EthBlockNumberOrHash) (ethtypes.EthBytes, error) {
-	ts, err := a.getTipsetByEthBlockNumberOrHash(ctx, blkParam)
+	ts, err := a.getTipsetByEthBlockNumberOrHash(ctx, blkParam, true)


This used to be incorrect at the pending tipset, now it will error.

Stebalien · 2023-08-24T16:41:36Z

node/impl/full/eth.go

@@ -534,7 +564,7 @@ func (a *EthModule) EthGetCode(ctx context.Context, ethAddr ethtypes.EthAddress,
 		return nil, xerrors.Errorf("cannot get Filecoin address: %w", err)
 	}

-	ts, err := a.getTipsetByEthBlockNumberOrHash(ctx, blkParam)
+	ts, err := a.getTipsetByEthBlockNumberOrHash(ctx, blkParam, true)


This used to be incorrect at the pending tipset, now it will error.

Stebalien · 2023-08-24T16:44:29Z

So, the real question is: Do we want to do it this way? Or do we want to actually support "pending" by, e.g., computing the tipset state.

Computing the tipset state used to be a huge issue... but:

Tipset computation time is now sane again.
The results will be cached and computed at most once and only for "pending".

Stebalien · 2023-08-25T04:11:21Z

Ok, some discussion has been had:

We should correctly support "pending" wherever possible. That means calling TipSetState extensively to compute the tipset state. Luckily, this will be cached much of the time.
I have no good ideas on what to do about ethCall in general. The current logic (once we remove the "don't apply messages from the current tipset" feature) is, in fact, the "most correct". But it's slow.

Stebalien requested a review from a team as a code owner March 13, 2023 21:07

Stebalien force-pushed the steb/eth-tipset-after branch from 928b1a7 to 276afdb Compare March 13, 2023 21:11

raulk mentioned this pull request Mar 14, 2023

fix: revert "Eth API: drop support for 'pending' block parameter." #10474

Merged

7 tasks

Stebalien commented Apr 21, 2023

View reviewed changes

Stebalien marked this pull request as draft April 25, 2023 16:35

Stebalien force-pushed the steb/eth-tipset-after branch from 276afdb to 408e01d Compare August 24, 2023 16:37

Stebalien commented Aug 24, 2023

View reviewed changes

Stebalien mentioned this pull request Aug 24, 2023

eth_call incorrectly reconciling deferred (Filecoin) and immediate (Ethereum) execution #11205

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: eth: use the correct tipset for Eth APIs #10462

fix: eth: use the correct tipset for Eth APIs #10462

Stebalien commented Mar 13, 2023 •

edited

Loading

Stebalien commented Mar 13, 2023

Stebalien Apr 21, 2023

Stebalien Aug 24, 2023

Stebalien Aug 24, 2023

Stebalien Aug 24, 2023

Stebalien Aug 24, 2023

Stebalien Aug 24, 2023

Stebalien Aug 24, 2023

Stebalien Aug 24, 2023

Stebalien Aug 24, 2023

Stebalien Aug 24, 2023

Stebalien commented Aug 24, 2023

Stebalien commented Aug 25, 2023

fix: eth: use the correct tipset for Eth APIs #10462

Are you sure you want to change the base?

fix: eth: use the correct tipset for Eth APIs #10462

Conversation

Stebalien commented Mar 13, 2023 • edited Loading

Related Issues

Proposed Changes

Checklist

Stebalien commented Mar 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Aug 24, 2023

Stebalien commented Aug 25, 2023

Stebalien commented Mar 13, 2023 •

edited

Loading