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

fix: eth: use the correct state-tree when resolving addresses #11387

Merged
merged 4 commits into from
Nov 17, 2023

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Nov 7, 2023

Related Issues

Part of #11355

Proposed Changes

We need to always use the state-tree from the tipset after the message executed (i.e., the state-tree produced by executing the tipset). If we use any other state-tree, we might not find the address we're trying to resolve.

This change also has some implication for pending messages: there's no guarantee we'll be able to generate a 0x-style address for a pending native message. So, instead of trying, I've removed support for pending native messages from the Eth API. Messages from EthAccounts will still work, and native messages will still show up in blocks/traces, they just won't show up as "pending". Which should affect exactly nobody.

I'm also taking this opportunity to cleanup some edge-cases:

  1. Pass contexts where appropriate.
  2. Remove all state access from ethTxHashFromSignedMessage.

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
  • CI is green

@Stebalien Stebalien requested a review from a team as a code owner November 7, 2023 04:31
@Stebalien
Copy link
Member Author

Based on #11385, merge that first.

@Stebalien Stebalien requested review from raulk and arajasek November 7, 2023 04:32
chain/types/ethtypes/eth_transactions.go Show resolved Hide resolved
chain/types/ethtypes/eth_transactions.go Show resolved Hide resolved
chain/types/ethtypes/eth_transactions.go Show resolved Hide resolved
Comment on lines +436 to +441
tx, err := ethtypes.EthTxFromSignedEthMessage(smsg)
if err != nil {
return ethtypes.EmptyEthHash, err
return ethtypes.EthHash{}, xerrors.Errorf("failed to convert from signed message: %w", err)
}
return ethTx.Hash, nil

return tx.TxHash()
Copy link
Member Author

Choose a reason for hiding this comment

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

This does the same thing as the old code, this new code just makes it clear that there's no state access.

node/impl/full/eth_utils.go Show resolved Hide resolved
@@ -84,13 +85,18 @@ func (m *EthTxHashManager) ProcessSignedMessage(ctx context.Context, msg *types.
return
}

ethTx, err := newEthTxFromSignedMessage(ctx, msg, m.StateAPI)
ethTx, err := ethtypes.EthTxFromSignedEthMessage(msg)
Copy link
Member Author

Choose a reason for hiding this comment

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

Again, I'm just removing the state access (this is always a message from an EthAccount, so we never need to look at the state to resolve addresses).

@Stebalien Stebalien force-pushed the steb/fix-eth-addr-lookup branch from 17bd1f9 to 1802f3f Compare November 7, 2023 04:38
@Stebalien Stebalien requested a review from fridrik01 November 16, 2023 14:02
Copy link
Contributor

@fridrik01 fridrik01 left a comment

Choose a reason for hiding this comment

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

LGTM

return ethtypes.EthTx{}, xerrors.Errorf("failed to resolve Ethereum address: %w", err)
}

tx.From = fromAddr
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be fine I guess since we are now setting From inside ethtypes.EthTxFromSignedEthMessage

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that was the idea.


err = m.TransactionHashLookup.UpsertHash(ethTx.Hash, msg.Cid())
err = m.TransactionHashLookup.UpsertHash(txHash, msg.Cid())
Copy link
Contributor

Choose a reason for hiding this comment

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

Were we inserting the wrong hash into eth_tx_hashes?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I'm just refactoring this to make it clear that it doesn't access any state.

Base automatically changed from steb/uncle-tracing to master November 17, 2023 17:08
We need to always use the state-tree from the tipset _after_ the message
executed. If we use any other state-tree, we might not find the address
we're trying to resolve.

This change also has some implication for pending messages: there's no
guarantee we'll be able to generate a 0x-style address for a pending
native message. So, instead of trying, I've removed support for pending
native messages from the Eth API. Messages from EthAccounts will still
work, and native messages will still show up in blocks/traces, they just
won't show up as "pending". Which should affect exactly nobody.

I'm also taking this opportunity to cleanup some edge-cases:

1. Pass contexts where appropriate.
2. Remove all state access from `ethTxHashFromSignedMessage`.

Part of #11355
@Stebalien Stebalien force-pushed the steb/fix-eth-addr-lookup branch from 9f9990f to c0c3614 Compare November 17, 2023 17:10
@Stebalien Stebalien enabled auto-merge (squash) November 17, 2023 17:15
@Stebalien Stebalien merged commit 9b4df6a into master Nov 17, 2023
80 of 87 checks passed
@Stebalien Stebalien deleted the steb/fix-eth-addr-lookup branch November 17, 2023 17:20
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.

2 participants