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

feat(eth): fix EthGetTransactionCount for pending block parameter #12520

Merged
merged 9 commits into from
Oct 1, 2024

Conversation

virajbhartiya
Copy link
Member

@virajbhartiya virajbhartiya commented Sep 27, 2024

Related Issues

Fixes #10357

Proposed Changes

  1. Added a condition to check for the "pending" block parameter.
  2. Implemented mempool nonce retrieval for "pending" blocks.
  3. Included error handling for the mempool query.
  4. Ensured the mempool is only accessed when necessary (i.e., for "pending" blocks).

Checklist

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

@virajbhartiya
Copy link
Member Author

virajbhartiya commented Sep 27, 2024

@aarshkshah1992 I have added tests in fevm_tests.go. EthGetTransactionCount seemed to be tested also in TestGetCodeAndNonce but I have written a standalone test for it. Referenced the following for implementing it

func TestGetCodeAndNonce(t *testing.T) {
kit.QuietMiningLogs()
blockTime := 100 * time.Millisecond
client, _, ens := kit.EnsembleMinimal(t, kit.MockProofs(), kit.ThroughRPC())
ens.InterconnectAll().BeginMining(blockTime)
ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
defer cancel()
// Accounts should have empty code, empty nonce.
{
// A random eth address should have no code.
_, ethAddr, filAddr := client.EVM().NewAccount()
bytecode, err := client.EVM().EthGetCode(ctx, ethAddr, ethtypes.NewEthBlockNumberOrHashFromPredefined("latest"))
require.NoError(t, err)
require.Empty(t, bytecode)
// Nonce should also be zero
nonce, err := client.EVM().EthGetTransactionCount(ctx, ethAddr, ethtypes.NewEthBlockNumberOrHashFromPredefined("latest"))
require.NoError(t, err)
require.Zero(t, nonce)
// send some funds to the account.
kit.SendFunds(ctx, t, client, filAddr, types.FromFil(10))
// The code should still be empty, target is now a placeholder.
bytecode, err = client.EVM().EthGetCode(ctx, ethAddr, ethtypes.NewEthBlockNumberOrHashFromPredefined("latest"))
require.NoError(t, err)
require.Empty(t, bytecode)
// Nonce should still be zero.
nonce, err = client.EVM().EthGetTransactionCount(ctx, ethAddr, ethtypes.NewEthBlockNumberOrHashFromPredefined("latest"))
require.NoError(t, err)
require.Zero(t, nonce)
}
// Check contract code.
{
// install a contract
contractHex, err := os.ReadFile("./contracts/SelfDestruct.hex")
require.NoError(t, err)
contract, err := hex.DecodeString(string(contractHex))
require.NoError(t, err)
createReturn := client.EVM().DeployContract(ctx, client.DefaultKey.Address, contract)
contractAddr := createReturn.EthAddress
contractFilAddr := *createReturn.RobustAddress
// The newly deployed contract should not be empty.
bytecode, err := client.EVM().EthGetCode(ctx, contractAddr, ethtypes.NewEthBlockNumberOrHashFromPredefined("latest"))
require.NoError(t, err)
require.NotEmpty(t, bytecode)
// Nonce should be one.
nonce, err := client.EVM().EthGetTransactionCount(ctx, contractAddr, ethtypes.NewEthBlockNumberOrHashFromPredefined("latest"))
require.NoError(t, err)
require.Equal(t, ethtypes.EthUint64(1), nonce)
// Destroy it.
_, _, err = client.EVM().InvokeContractByFuncName(ctx, client.DefaultKey.Address, contractFilAddr, "destroy()", nil)
require.NoError(t, err)
// The code should be empty again.
bytecode, err = client.EVM().EthGetCode(ctx, contractAddr, ethtypes.NewEthBlockNumberOrHashFromPredefined("latest"))
require.NoError(t, err)
require.Empty(t, bytecode)
// Nonce should go back to zero
nonce, err = client.EVM().EthGetTransactionCount(ctx, contractAddr, ethtypes.NewEthBlockNumberOrHashFromPredefined("latest"))
require.NoError(t, err)
require.Zero(t, nonce)
}
}

itests/fevm_test.go Outdated Show resolved Hide resolved
node/impl/full/eth.go Outdated Show resolved Hide resolved
Co-authored-by: Rod Vagg <rod@vagg.org>
itests/fevm_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

good stuff !

@aarshkshah1992
Copy link
Contributor

@rvagg Happy for me to merge this ?

itests/fevm_test.go Outdated Show resolved Hide resolved
@virajbhartiya
Copy link
Member Author

I am unable to understand why TestGatewayRateLimits failed, it's passing when I run it locally

@rvagg
Copy link
Member

rvagg commented Oct 1, 2024

The gateway test is on me, unfortunately: #12001 (comment)

Most of the flakies are catalogued in #12001 if you want to look them up. There are a few more not documented in there too: https://github.com/filecoin-project/lotus/issues?q=is%3Aissue+is%3Aopen+flaky

So you can ignore that one.

@rvagg rvagg enabled auto-merge (squash) October 1, 2024 04:47
@rvagg rvagg merged commit 13fb322 into filecoin-project:master Oct 1, 2024
80 of 81 checks passed
@rvagg
Copy link
Member

rvagg commented Oct 1, 2024

🥳 thanks @virajbhartiya !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

EthGetTransactionCount ignores input
3 participants