Skip to content

Commit

Permalink
ethclient: fix tx sender cache miss detection (#23877)
Browse files Browse the repository at this point in the history 
This fixes a bug in TransactionSender where it would return the
zero address for transactions where the sender address wasn't
cached already.

Co-authored-by: Felix Lange <[email protected]>
  • Loading branch information
@PlasmaPower @fjl
PlasmaPower and fjl authored Nov 17, 2021
1 parent fa96718 commit 16341e0
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 39 deletions.
2 changes: 2 additions & 0 deletions ethclient/ethclient.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,8 @@ func (ec *Client) TransactionSender(ctx context.Context, tx *types.Transaction,
if err == nil {
return sender, nil
}

// It was not found in cache, ask the server.
var meta struct {
Hash common.Hash
From common.Address
Expand Down
157 changes: 119 additions & 38 deletions ethclient/ethclient_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,9 +187,34 @@ var (
testBalance = big.NewInt(2e15)
)

var genesis = &core.Genesis{
Config: params.AllEthashProtocolChanges,
Alloc: core.GenesisAlloc{testAddr: {Balance: testBalance}},
ExtraData: []byte("test genesis"),
Timestamp: 9000,
BaseFee: big.NewInt(params.InitialBaseFee),
}

var testTx1 = types.MustSignNewTx(testKey, types.LatestSigner(genesis.Config), &types.LegacyTx{
Nonce: 0,
Value: big.NewInt(12),
GasPrice: big.NewInt(params.InitialBaseFee),
Gas: params.TxGas,
To: &common.Address{2},
})

var testTx2 = types.MustSignNewTx(testKey, types.LatestSigner(genesis.Config), &types.LegacyTx{
Nonce: 1,
Value: big.NewInt(8),
GasPrice: big.NewInt(params.InitialBaseFee),
Gas: params.TxGas,
To: &common.Address{2},
})

func newTestBackend(t *testing.T) (*node.Node, []*types.Block) {
// Generate test chain.
genesis, blocks := generateTestChain()
blocks := generateTestChain()

// Create node
n, err := node.New(&node.Config{})
if err != nil {
Expand All @@ -212,25 +237,22 @@ func newTestBackend(t *testing.T) (*node.Node, []*types.Block) {
return n, blocks
}

func generateTestChain() (*core.Genesis, []*types.Block) {
func generateTestChain() []*types.Block {
db := rawdb.NewMemoryDatabase()
config := params.AllEthashProtocolChanges
genesis := &core.Genesis{
Config: config,
Alloc: core.GenesisAlloc{testAddr: {Balance: testBalance}},
ExtraData: []byte("test genesis"),
Timestamp: 9000,
BaseFee: big.NewInt(params.InitialBaseFee),
}
generate := func(i int, g *core.BlockGen) {
g.OffsetTime(5)
g.SetExtra([]byte("test"))
if i == 1 {
// Test transactions are included in block #2.
g.AddTx(testTx1)
g.AddTx(testTx2)
}
}
gblock := genesis.ToBlock(db)
engine := ethash.NewFaker()
blocks, _ := core.GenerateChain(config, gblock, engine, db, 1, generate)
blocks, _ := core.GenerateChain(genesis.Config, gblock, engine, db, 2, generate)
blocks = append([]*types.Block{gblock}, blocks...)
return genesis, blocks
return blocks
}

func TestEthClient(t *testing.T) {
Expand All @@ -242,30 +264,33 @@ func TestEthClient(t *testing.T) {
tests := map[string]struct {
test func(t *testing.T)
}{
"TestHeader": {
"Header": {
func(t *testing.T) { testHeader(t, chain, client) },
},
"TestBalanceAt": {
"BalanceAt": {
func(t *testing.T) { testBalanceAt(t, client) },
},
"TestTxInBlockInterrupted": {
"TxInBlockInterrupted": {
func(t *testing.T) { testTransactionInBlockInterrupted(t, client) },
},
"TestChainID": {
"ChainID": {
func(t *testing.T) { testChainID(t, client) },
},
"TestGetBlock": {
"GetBlock": {
func(t *testing.T) { testGetBlock(t, client) },
},
"TestStatusFunctions": {
"StatusFunctions": {
func(t *testing.T) { testStatusFunctions(t, client) },
},
"TestCallContract": {
"CallContract": {
func(t *testing.T) { testCallContract(t, client) },
},
"TestAtFunctions": {
"AtFunctions": {
func(t *testing.T) { testAtFunctions(t, client) },
},
"TransactionSender": {
func(t *testing.T) { testTransactionSender(t, client) },
},
}

t.Parallel()
Expand Down Expand Up @@ -321,6 +346,11 @@ func testBalanceAt(t *testing.T, client *rpc.Client) {
want *big.Int
wantErr error
}{
"valid_account_genesis": {
account: testAddr,
block: big.NewInt(0),
want: testBalance,
},
"valid_account": {
account: testAddr,
block: big.NewInt(1),
Expand Down Expand Up @@ -358,23 +388,25 @@ func testBalanceAt(t *testing.T, client *rpc.Client) {
func testTransactionInBlockInterrupted(t *testing.T, client *rpc.Client) {
ec := NewClient(client)

// Get current block by number
// Get current block by number.
block, err := ec.BlockByNumber(context.Background(), nil)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
// Test tx in block interupted

// Test tx in block interupted.
ctx, cancel := context.WithCancel(context.Background())
cancel()
tx, err := ec.TransactionInBlock(ctx, block.Hash(), 1)
tx, err := ec.TransactionInBlock(ctx, block.Hash(), 0)
if tx != nil {
t.Fatal("transaction should be nil")
}
if err == nil || err == ethereum.NotFound {
t.Fatal("error should not be nil/notfound")
}
// Test tx in block not found
if _, err := ec.TransactionInBlock(context.Background(), block.Hash(), 1); err != ethereum.NotFound {

// Test tx in block not found.
if _, err := ec.TransactionInBlock(context.Background(), block.Hash(), 20); err != ethereum.NotFound {
t.Fatal("error should be ethereum.NotFound")
}
}
Expand All @@ -392,12 +424,13 @@ func testChainID(t *testing.T, client *rpc.Client) {

func testGetBlock(t *testing.T, client *rpc.Client) {
ec := NewClient(client)

// Get current block number
blockNumber, err := ec.BlockNumber(context.Background())
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if blockNumber != 1 {
if blockNumber != 2 {
t.Fatalf("BlockNumber returned wrong number: %d", blockNumber)
}
// Get current block by number
Expand Down Expand Up @@ -445,6 +478,7 @@ func testStatusFunctions(t *testing.T, client *rpc.Client) {
if progress != nil {
t.Fatalf("unexpected progress: %v", progress)
}

// NetworkID
networkID, err := ec.NetworkID(context.Background())
if err != nil {
Expand All @@ -453,20 +487,22 @@ func testStatusFunctions(t *testing.T, client *rpc.Client) {
if networkID.Cmp(big.NewInt(0)) != 0 {
t.Fatalf("unexpected networkID: %v", networkID)
}
// SuggestGasPrice (should suggest 1 Gwei)

// SuggestGasPrice
gasPrice, err := ec.SuggestGasPrice(context.Background())
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if gasPrice.Cmp(big.NewInt(1875000000)) != 0 { // 1 gwei tip + 0.875 basefee after a 1 gwei fee empty block
if gasPrice.Cmp(big.NewInt(1000000000)) != 0 {
t.Fatalf("unexpected gas price: %v", gasPrice)
}
// SuggestGasTipCap (should suggest 1 Gwei)

// SuggestGasTipCap
gasTipCap, err := ec.SuggestGasTipCap(context.Background())
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if gasTipCap.Cmp(big.NewInt(1000000000)) != 0 {
if gasTipCap.Cmp(big.NewInt(234375000)) != 0 {
t.Fatalf("unexpected gas tip cap: %v", gasTipCap)
}
}
Expand Down Expand Up @@ -500,9 +536,11 @@ func testCallContract(t *testing.T, client *rpc.Client) {

func testAtFunctions(t *testing.T, client *rpc.Client) {
ec := NewClient(client)

// send a transaction for some interesting pending status
sendTransaction(ec)
time.Sleep(100 * time.Millisecond)

// Check pending transaction count
pending, err := ec.PendingTransactionCount(context.Background())
if err != nil {
Expand Down Expand Up @@ -561,23 +599,66 @@ func testAtFunctions(t *testing.T, client *rpc.Client) {
}
}

func testTransactionSender(t *testing.T, client *rpc.Client) {
ec := NewClient(client)
ctx := context.Background()

// Retrieve testTx1 via RPC.
block2, err := ec.HeaderByNumber(ctx, big.NewInt(2))
if err != nil {
t.Fatal("can't get block 1:", err)
}
tx1, err := ec.TransactionInBlock(ctx, block2.Hash(), 0)
if err != nil {
t.Fatal("can't get tx:", err)
}
if tx1.Hash() != testTx1.Hash() {
t.Fatalf("wrong tx hash %v, want %v", tx1.Hash(), testTx1.Hash())
}

// The sender address is cached in tx1, so no additional RPC should be required in
// TransactionSender. Ensure the server is not asked by canceling the context here.
canceledCtx, cancel := context.WithCancel(context.Background())
cancel()
sender1, err := ec.TransactionSender(canceledCtx, tx1, block2.Hash(), 0)
if err != nil {
t.Fatal(err)
}
if sender1 != testAddr {
t.Fatal("wrong sender:", sender1)
}

// Now try to get the sender of testTx2, which was not fetched through RPC.
// TransactionSender should query the server here.
sender2, err := ec.TransactionSender(ctx, testTx2, block2.Hash(), 1)
if err != nil {
t.Fatal(err)
}
if sender2 != testAddr {
t.Fatal("wrong sender:", sender2)
}
}

func sendTransaction(ec *Client) error {
// Retrieve chainID
chainID, err := ec.ChainID(context.Background())
if err != nil {
return err
}
// Create transaction
tx := types.NewTransaction(0, common.Address{1}, big.NewInt(1), 22000, big.NewInt(params.InitialBaseFee), nil)
signer := types.LatestSignerForChainID(chainID)
signature, err := crypto.Sign(signer.Hash(tx).Bytes(), testKey)
nonce, err := ec.PendingNonceAt(context.Background(), testAddr)
if err != nil {
return err
}
signedTx, err := tx.WithSignature(signer, signature)

signer := types.LatestSignerForChainID(chainID)
tx, err := types.SignNewTx(testKey, signer, &types.LegacyTx{
Nonce: nonce,
To: &common.Address{2},
Value: big.NewInt(1),
Gas: 22000,
GasPrice: big.NewInt(params.InitialBaseFee),
})
if err != nil {
return err
}
// Send transaction
return ec.SendTransaction(context.Background(), signedTx)
return ec.SendTransaction(context.Background(), tx)
}
2 changes: 1 addition & 1 deletion ethclient/signer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func (s *senderFromServer) Equal(other types.Signer) bool {
}

func (s *senderFromServer) Sender(tx *types.Transaction) (common.Address, error) {
if s.blockhash == (common.Hash{}) {
if s.addr == (common.Address{}) {
return common.Address{}, errNotCached
}
return s.addr, nil
Expand Down

0 comments on commit 16341e0

Please sign in to comment.