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.

Sign up for GitHub

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_getTransactionByHash + eth_getTransactionReceipt fixes #9933

Merged
merged 8 commits into from
Dec 23, 2022
Merged

fix: eth_getTransactionByHash + eth_getTransactionReceipt fixes #9933

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
48b4e75
fix: eth_getTransactionByHash: return hash.
raulk Dec 23, 2022
061e214
eth_getTransactionByHash: return nil in some fields when tx is in mpool.
raulk Dec 23, 2022
645221d
fix nonce
ychiaoli18 Dec 23, 2022
ac39d5c
regen docs.
raulk Dec 23, 2022
51c0f1b
fix: Eth JSON-RPC API: eth_getTransactionReceipt logs-related fixes.
raulk Dec 23, 2022
2445487
add test coverage.
raulk Dec 23, 2022
4963c0b
add docs.
raulk Dec 23, 2022
a2af7df
refine docs.
raulk Dec 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified build/openrpc/full.json.gz
View file
Open in desktop
Binary file not shown.
6 changes: 3 additions & 3 deletions chain/types/ethtypes/eth_transactions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ type EthTx struct {
ChainID EthUint64 `json:"chainId"`
Nonce EthUint64 `json:"nonce"`
Hash EthHash `json:"hash"`
BlockHash EthHash `json:"blockHash"`
BlockNumber EthUint64 `json:"blockNumber"`
TransactionIndex EthUint64 `json:"transactionIndex"`
BlockHash *EthHash `json:"blockHash"`
BlockNumber *EthUint64 `json:"blockNumber"`
TransactionIndex *EthUint64 `json:"transactionIndex"`
From EthAddress `json:"from"`
To *EthAddress `json:"to"`
Value EthBigInt `json:"value"`
Expand Down
18 changes: 9 additions & 9 deletions documentation/en/api-v1-unstable-methods.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2319,9 +2319,9 @@ Response:
"chainId": "0x0",
"nonce": "0x5",
"hash": "0x0707070707070707070707070707070707070707070707070707070707070707",
"blockHash": "0x0000000000000000000000000000000000000000000000000000000000000000",
"blockNumber": "0x0",
"transactionIndex": "0x0",
"blockHash": null,
"blockNumber": null,
"transactionIndex": null,
"from": "0x0707070707070707070707070707070707070707",
"to": "0x5cbeecf99d3fdb3f25e309cc264f240bb0664031",
"value": "0x0",
Expand Down Expand Up @@ -2355,9 +2355,9 @@ Response:
"chainId": "0x0",
"nonce": "0x5",
"hash": "0x0707070707070707070707070707070707070707070707070707070707070707",
"blockHash": "0x0000000000000000000000000000000000000000000000000000000000000000",
"blockNumber": "0x0",
"transactionIndex": "0x0",
"blockHash": null,
"blockNumber": null,
"transactionIndex": null,
"from": "0x0707070707070707070707070707070707070707",
"to": "0x5cbeecf99d3fdb3f25e309cc264f240bb0664031",
"value": "0x0",
Expand Down Expand Up @@ -2390,9 +2390,9 @@ Response:
"chainId": "0x0",
"nonce": "0x5",
"hash": "0x0707070707070707070707070707070707070707070707070707070707070707",
"blockHash": "0x0000000000000000000000000000000000000000000000000000000000000000",
"blockNumber": "0x0",
"transactionIndex": "0x0",
"blockHash": null,
"blockNumber": null,
"transactionIndex": null,
"from": "0x0707070707070707070707070707070707070707",
"to": "0x5cbeecf99d3fdb3f25e309cc264f240bb0664031",
"value": "0x0",
Expand Down
38 changes: 33 additions & 5 deletions itests/eth_deploy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ func TestDeployment(t *testing.T) {
// He who writes the second test, shall do that.
// kit.QuietMiningLogs()

blockTime := 100 * time.Millisecond
// reasonable blocktime so that the tx sits in the mpool for a bit during the test.
// although this is non-deterministic...
blockTime := 1 * time.Second
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd consider increasing this -- this test isn't really constrained by epochs (needs ~3 epochs), so even if the blocktime were like 5 seconds, it would still finish pretty quickly

Copy link
Member Author

Choose a reason for hiding this comment

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

Plus the boot up time for the chain (around 10 epochs).

client, _, ens := kit.EnsembleMinimal(
t,
kit.MockProofs(),
Expand Down Expand Up @@ -94,17 +96,25 @@ func TestDeployment(t *testing.T) {
hash := client.EVM().SubmitTransaction(ctx, &tx)
fmt.Println(hash)

mpoolTx, err := client.EthGetTransactionByHash(ctx, &hash)
require.NoError(t, err)

// require that the hashes are identical
require.Equal(t, hash, mpoolTx.Hash)

// these fields should be nil because the tx hasn't landed on chain.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's not play this game, we're inviting flakiness here.

Can we just try to lookup the receipt before making these assertions? If found, let's simply skip them (with a log saying that we're doing so).

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 is actually a pretty important assertion (we had bugs here before, so want to lock these for regression testing). I'm onboard with deflaking, as long as we don't withdraw coverage we need.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Miners in the itest framework have pause and unpause functions exposed, but we should implement a function for the test kit that allows us to pause and unpause all mining.

require.Nil(t, mpoolTx.BlockNumber)
require.Nil(t, mpoolTx.BlockHash)
require.Nil(t, mpoolTx.TransactionIndex)

changes, err := client.EthGetFilterChanges(ctx, pendingFilter)
require.NoError(t, err)
require.Len(t, changes.Results, 1)
require.Equal(t, hash.String(), changes.Results[0])

time.Sleep(5 * time.Second)

var receipt *api.EthTxReceipt
for i := 0; i < 10000000000; i++ {
for i := 0; i < 20; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just actually StateWaitMsg here instead of this funkiness -- if we don't have the tooling to easily get the msgCid, slap a TODO on it for now (I think the work @geoff-vball is doing will make this easy to do).

Copy link
Member Author

Choose a reason for hiding this comment

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

We do need to check that the receipt is null while the transaction hasn't landed on chain -- it's part of the test's coverage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, keep doing that every 500ms, but the cue to stop is StateWaitMsg returning (not "i waited 10 seconds").

Copy link
Member Author

Choose a reason for hiding this comment

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

We still need a timeout for the test (should not let it run indefinitely). We could have a timed context with StateWaitMsg, but at that point we're achieving the same effect as here with more complicated code, so it's a matter of choice.

receipt, err = client.EthGetTransactionReceipt(ctx, hash)
fmt.Println(receipt, err)
if err != nil || receipt == nil {
time.Sleep(500 * time.Millisecond)
continue
Expand All @@ -113,10 +123,28 @@ func TestDeployment(t *testing.T) {
}
require.NoError(t, err)
require.NotNil(t, receipt)
// logs must be an empty array, not a nil value, to avoid tooling compatibility issues
require.Empty(t, receipt.Logs)
// a correctly formed logs bloom, albeit empty, has 256 zeroes
require.Len(t, receipt.LogsBloom, 256)
require.Equal(t, ethtypes.EthBytes(make([]byte, 256)), receipt.LogsBloom)

// Success.
require.EqualValues(t, ethtypes.EthUint64(0x1), receipt.Status)

// Verify that the chain transaction now has new fields set.
chainTx, err := client.EthGetTransactionByHash(ctx, &hash)
require.NoError(t, err)

// require that the hashes are identical
require.Equal(t, hash, chainTx.Hash)
require.NotNil(t, chainTx.BlockNumber)
require.Greater(t, uint64(*chainTx.BlockNumber), uint64(0))
require.NotNil(t, chainTx.BlockHash)
require.NotEmpty(t, *chainTx.BlockHash)
require.NotNil(t, chainTx.TransactionIndex)
require.Equal(t, uint64(*chainTx.TransactionIndex), uint64(0)) // only transaction

// Verify that the deployer is now an account.
client.AssertActorType(ctx, deployer, manifest.EthAccountKey)

Expand Down
77 changes: 64 additions & 13 deletions node/impl/full/eth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,31 @@ var (
_ EthEventAPI = *new(api.FullNode)
)

// EthModule provides a default implementation of EthModuleAPI.
// It can be swapped out with another implementation through Dependency
// Injection (for example with a thin RPC client).
var EmptyLogsBloom = make([]byte, 256)

// EthModule provides the default implementation of the standard Ethereum JSON-RPC API.
//
// # Execution model reconciliation
//
// Ethereum relies on an immediate block-based execution model. The block that includes
// a transaction is also the block that executes it. Each block specifies the state root
// resulting from executing all transactions within it (output state).
//
// In Filecoin, at every epoch there is an unknown number of round winners all of whom are
// entitled to publish a block. Blocks are collected into a tipset. A tipset is committed
// only when the subsequent tipset is built on it (i.e. it becomes a parent). Block producers
// execute the parent tipset and specify the resulting state root in the block being produced.
// In other words, contrary to Ethereum, each block specifies the input state root.
//
// Ethereum clients expect transactions returned via eth_getBlock* to have a receipt
// (due to immediate execution). For this reason:
//
// - eth_blockNumber returns the latest executed epoch (head - 1)
// - The 'latest' block refers to the latest executed epoch (head - 1)
// - The 'pending' block refers to the current speculative tipset (head)
// - eth_getTransactionByHash returns the inclusion tipset of a message, but
// only after it has executed.
// - eth_getTransactionReceipt ditto.
type EthModule struct {
fx.In

Expand Down Expand Up @@ -1480,7 +1502,14 @@ func newEthTxFromFilecoinMessage(ctx context.Context, smsg *types.SignedMessage,
r, s, v = ethtypes.EthBigIntZero, ethtypes.EthBigIntZero, ethtypes.EthBigIntZero
}

hash, err := ethtypes.NewEthHashFromCid(smsg.Cid())
if err != nil {
return ethtypes.EthTx{}, err
}

tx := ethtypes.EthTx{
Hash: hash,
Nonce: ethtypes.EthUint64(smsg.Message.Nonce),
ChainID: ethtypes.EthUint64(build.Eip155ChainId),
From: fromEthAddr,
To: toAddr,
Expand Down Expand Up @@ -1559,24 +1588,46 @@ func newEthTxFromFilecoinMessageLookup(ctx context.Context, msgLookup *api.MsgLo
return ethtypes.EthTx{}, err
}

var (
bn = ethtypes.EthUint64(parentTs.Height())
ti = ethtypes.EthUint64(txIdx)
)

tx.ChainID = ethtypes.EthUint64(build.Eip155ChainId)
tx.Hash = txHash
tx.BlockHash = blkHash
tx.BlockNumber = ethtypes.EthUint64(parentTs.Height())
tx.TransactionIndex = ethtypes.EthUint64(txIdx)
tx.BlockHash = &blkHash
tx.BlockNumber = &bn
tx.TransactionIndex = &ti
return tx, nil
}

func newEthTxReceipt(ctx context.Context, tx ethtypes.EthTx, lookup *api.MsgLookup, replay *api.InvocResult, events []types.Event, sa StateAPI) (api.EthTxReceipt, error) {
var (
transactionIndex ethtypes.EthUint64
blockHash ethtypes.EthHash
blockNumber ethtypes.EthUint64
)

if tx.TransactionIndex != nil {
transactionIndex = *tx.TransactionIndex
}
if tx.BlockHash != nil {
blockHash = *tx.BlockHash
}
if tx.BlockNumber != nil {
blockNumber = *tx.BlockNumber
}

receipt := api.EthTxReceipt{
TransactionHash: tx.Hash,
TransactionIndex: tx.TransactionIndex,
BlockHash: tx.BlockHash,
BlockNumber: tx.BlockNumber,
From: tx.From,
To: tx.To,
TransactionIndex: transactionIndex,
BlockHash: blockHash,
BlockNumber: blockNumber,
Type: ethtypes.EthUint64(2),
LogsBloom: []byte{0},
LogsBloom: EmptyLogsBloom,
Logs: make([]ethtypes.EthLog, 0),
}

if receipt.To == nil && lookup.Receipt.ExitCode.IsSuccess() {
Expand Down Expand Up @@ -1608,10 +1659,10 @@ func newEthTxReceipt(ctx context.Context, tx ethtypes.EthTx, lookup *api.MsgLook
l := ethtypes.EthLog{
Removed: false,
LogIndex: ethtypes.EthUint64(i),
TransactionIndex: tx.TransactionIndex,
TransactionHash: tx.Hash,
BlockHash: tx.BlockHash,
BlockNumber: tx.BlockNumber,
TransactionIndex: transactionIndex,
BlockHash: blockHash,
BlockNumber: blockNumber,
}

for _, entry := range evt.Entries {
Expand Down