Maintain local copy of the nonce for each used address

If multiple concurrent transactions are sent from the same address
nonce for some of them will be the same. It will result in the
"known transaction error" and such transaction will be dropped
from a queue.

There is 2 alternatives in solving such problem:
- adding an auto-retry to transaction queue, which is very complex
  and error prone, this way we can actually retry transactions
  that are already processed by ethereum
- maintain nonce locally

Second approach is a straightforward. We keep asking for a nonce
from upstream, but if our local nonce is higher we steak to it.
Our local nonce is updated only if transaction succeeds, so there is
no way to send out of order transaction.

Signed-off-by: Dmitry Shulyak <yashulyak@gmail.com>

geth-patches/README.md

@@ -15,7 +15,6 @@ We try to minimize number and amount of changes in those patches as much as poss
  - `0004-whisper-notifications.patch` — adds Whisper notifications (need to be reviewed and documented)
  - `0006-latest-cht.patch` – updates CHT root hashes, should be updated regularly to keep sync fast, until proper Trusted Checkpoint sync is not implemented as part of LES/2 protocol.
  - `0007-README.patch` — update upstream README.md.
- - `0008-tx-pool-nonce.patch` - On GetTransactionCount request with PendingBlockNumber get the nonce from transaction pool
 
 # Updating upstream version
 

geth/transactions/txqueue_manager.go

@@ -10,6 +10,7 @@ import (
 	"github.com/ethereum/go-ethereum/core/types"
 	"github.com/status-im/status-go/geth/common"
 	"github.com/status-im/status-go/geth/log"
+	"github.com/status-im/status-go/geth/params"
 	"github.com/status-im/status-go/geth/transactions/queue"
 )
 
@@ -29,9 +30,11 @@ type Manager struct {
 	accountManager    common.AccountManager
 	txQueue           *queue.TxQueue
 	ethTxClient       EthTransactor
-	addrLock          *AddrLocker
 	notify            bool
 	completionTimeout time.Duration
+
+	addrLock   *AddrLocker
+	localNonce map[gethcommon.Address]uint64
 }
 
 // NewManager returns a new Manager.
@@ -43,6 +46,7 @@ func NewManager(nodeManager common.NodeManager, accountManager common.AccountMan
 		addrLock:          &AddrLocker{},
 		notify:            true,
 		completionTimeout: DefaultTxSendCompletionTimeout,
+		localNonce:        map[gethcommon.Address]uint64{},
 	}
 }
 
@@ -126,19 +130,22 @@ func (m *Manager) CompleteTransaction(id common.QueuedTxID, password string) (ha
 		log.Warn("can't process transaction", "err", err)
 		return hash, err
 	}
-	account, err := m.validateAccount(tx)
+	config, err := m.nodeManager.NodeConfig()
+	if err != nil {
+		return hash, err
+	}
+	account, err := m.validateAccount(config, tx, password)
 	if err != nil {
 		m.txDone(tx, hash, err)
 		return hash, err
 	}
-	// Send the transaction finally.
-	hash, err = m.completeTransaction(tx, account, password)
+	hash, err = m.completeTransaction(config, account, tx)
 	log.Info("finally completed transaction", "id", tx.ID, "hash", hash, "err", err)
 	m.txDone(tx, hash, err)
 	return hash, err
 }
 
-func (m *Manager) validateAccount(tx *common.QueuedTx) (*common.SelectedExtKey, error) {
+func (m *Manager) validateAccount(config *params.NodeConfig, tx *common.QueuedTx, password string) (*common.SelectedExtKey, error) {
 	selectedAccount, err := m.accountManager.SelectedAccount()
 	if err != nil {
 		log.Warn("failed to get a selected account", "err", err)
@@ -149,29 +156,37 @@ func (m *Manager) validateAccount(tx *common.QueuedTx) (*common.SelectedExtKey,
 		log.Warn("queued transaction does not belong to the selected account", "err", queue.ErrInvalidCompleteTxSender)
 		return nil, queue.ErrInvalidCompleteTxSender
 	}
-	return selectedAccount, nil
-}
-
-func (m *Manager) completeTransaction(queuedTx *common.QueuedTx, selectedAccount *common.SelectedExtKey, password string) (hash gethcommon.Hash, err error) {
-	log.Info("complete transaction", "id", queuedTx.ID)
-	log.Info("verifying account password for transaction", "id", queuedTx.ID)
-	config, err := m.nodeManager.NodeConfig()
-	if err != nil {
-		return hash, err
-	}
 	_, err = m.accountManager.VerifyAccountPassword(config.KeyStoreDir, selectedAccount.Address.String(), password)
 	if err != nil {
 		log.Warn("failed to verify account", "account", selectedAccount.Address.String(), "error", err.Error())
-		return hash, err
+		return nil, err
 	}
+	return selectedAccount, nil
+}
+
+func (m *Manager) completeTransaction(config *params.NodeConfig, selectedAccount *common.SelectedExtKey, queuedTx *common.QueuedTx) (hash gethcommon.Hash, err error) {
+	log.Info("complete transaction", "id", queuedTx.ID)
+	// nonce should be incremented only if tx completed without error
+	// if upstream node returned nonce higher than ours we will stick to it
 	m.addrLock.LockAddr(queuedTx.Args.From)
-	defer m.addrLock.UnlockAddr(queuedTx.Args.From)
+	localNonce := m.localNonce[queuedTx.Args.From]
+	var nonce uint64
+	defer func() {
+		if err == nil {
+			m.localNonce[queuedTx.Args.From] = nonce + 1
+		}
+		m.addrLock.UnlockAddr(queuedTx.Args.From)
+
+	}()
 	ctx, cancel := context.WithTimeout(context.Background(), defaultTimeout)
 	defer cancel()
-	nonce, err := m.ethTxClient.PendingNonceAt(ctx, queuedTx.Args.From)
+	nonce, err = m.ethTxClient.PendingNonceAt(ctx, queuedTx.Args.From)
 	if err != nil {
 		return hash, err
 	}
+	if localNonce > nonce {
+		nonce = localNonce
+	}
 	args := queuedTx.Args
 	gasPrice := (*big.Int)(args.GasPrice)
 	if args.GasPrice == nil {
@@ -226,7 +241,8 @@ func (m *Manager) completeTransaction(queuedTx *common.QueuedTx, selectedAccount
 	}
 	ctx, cancel = context.WithTimeout(context.Background(), defaultTimeout)
 	defer cancel()
-	if err := m.ethTxClient.SendTransaction(ctx, signedTx); err != nil {
+	err = m.ethTxClient.SendTransaction(ctx, signedTx)
+	if err != nil {
 		return hash, err
 	}
 	return signedTx.Hash(), nil

geth/transactions/txqueue_manager_test.go

@@ -2,6 +2,7 @@ package transactions
 
 import (
 	"context"
+	"errors"
 	"math/big"
 	"sync"
 	"testing"
@@ -61,19 +62,19 @@ func (s *TxQueueTestSuite) TearDownTest() {
 	s.client.Close()
 }
 
-func (s *TxQueueTestSuite) setupTransactionPoolAPI(account *common.SelectedExtKey, nonce hexutil.Uint64, gas hexutil.Big, txErr error) {
-	s.txServiceMock.EXPECT().GetTransactionCount(gomock.Any(), account.Address, gethrpc.PendingBlockNumber).Return(&nonce, nil)
-	s.txServiceMock.EXPECT().GasPrice(gomock.Any()).Return(big.NewInt(10), nil)
-	s.txServiceMock.EXPECT().EstimateGas(gomock.Any(), gomock.Any()).Return(&gas, nil)
-	s.txServiceMock.EXPECT().SendRawTransaction(gomock.Any(), gomock.Any()).Return(gethcommon.Hash{}, txErr)
+func (s *TxQueueTestSuite) setupTransactionPoolAPI(txCount int, account *common.SelectedExtKey, nonce hexutil.Uint64, gas hexutil.Big, txErr error) {
+	s.txServiceMock.EXPECT().GetTransactionCount(gomock.Any(), account.Address, gethrpc.PendingBlockNumber).Times(txCount).Return(&nonce, nil)
+	s.txServiceMock.EXPECT().GasPrice(gomock.Any()).Times(txCount).Return(big.NewInt(10), nil)
+	s.txServiceMock.EXPECT().EstimateGas(gomock.Any(), gomock.Any()).Times(txCount).Return(&gas, nil)
+	s.txServiceMock.EXPECT().SendRawTransaction(gomock.Any(), gomock.Any()).Times(txCount).Return(gethcommon.Hash{}, txErr)
 }
 
-func (s *TxQueueTestSuite) setupStatusBackend(account *common.SelectedExtKey, password string, passwordErr error) {
+func (s *TxQueueTestSuite) setupStatusBackend(txCount int, account *common.SelectedExtKey, password string, passErr error) {
 	nodeConfig, nodeErr := params.NewNodeConfig("/tmp", params.RopstenNetworkID, true)
-	s.nodeManagerMock.EXPECT().NodeConfig().Return(nodeConfig, nodeErr)
-	s.accountManagerMock.EXPECT().SelectedAccount().Return(account, nil)
-	s.accountManagerMock.EXPECT().VerifyAccountPassword(nodeConfig.KeyStoreDir, account.Address.String(), password).Return(
-		nil, passwordErr)
+	s.nodeManagerMock.EXPECT().NodeConfig().Times(txCount).Return(nodeConfig, nodeErr)
+	s.accountManagerMock.EXPECT().SelectedAccount().Times(txCount).Return(account, nil)
+	s.accountManagerMock.EXPECT().VerifyAccountPassword(nodeConfig.KeyStoreDir, account.Address.String(), password).Times(txCount).Return(
+		nil, passErr)
 }
 
 func (s *TxQueueTestSuite) TestCompleteTransaction() {
@@ -83,11 +84,11 @@ func (s *TxQueueTestSuite) TestCompleteTransaction() {
 		Address:    common.FromAddress(TestConfig.Account1.Address),
 		AccountKey: &keystore.Key{PrivateKey: key},
 	}
-	s.setupStatusBackend(account, password, nil)
+	s.setupStatusBackend(1, account, password, nil)
 
 	nonce := hexutil.Uint64(10)
 	gas := hexutil.Big(*big.NewInt(defaultGas + 1))
-	s.setupTransactionPoolAPI(account, nonce, gas, nil)
+	s.setupTransactionPoolAPI(1, account, nonce, gas, nil)
 
 	txQueueManager := NewManager(s.nodeManagerMock, s.accountManagerMock)
 
@@ -127,11 +128,11 @@ func (s *TxQueueTestSuite) TestCompleteTransactionMultipleTimes() {
 		Address:    common.FromAddress(TestConfig.Account1.Address),
 		AccountKey: &keystore.Key{PrivateKey: key},
 	}
-	s.setupStatusBackend(account, password, nil)
+	s.setupStatusBackend(1, account, password, nil)
 
 	nonce := hexutil.Uint64(10)
 	gas := hexutil.Big(*big.NewInt(defaultGas + 1))
-	s.setupTransactionPoolAPI(account, nonce, gas, nil)
+	s.setupTransactionPoolAPI(1, account, nonce, gas, nil)
 
 	txQueueManager := NewManager(s.nodeManagerMock, s.accountManagerMock)
 	txQueueManager.DisableNotificactions()
@@ -182,6 +183,8 @@ func (s *TxQueueTestSuite) TestCompleteTransactionMultipleTimes() {
 }
 
 func (s *TxQueueTestSuite) TestAccountMismatch() {
+	nodeConfig, nodeErr := params.NewNodeConfig("/tmp", params.RopstenNetworkID, true)
+	s.nodeManagerMock.EXPECT().NodeConfig().Return(nodeConfig, nodeErr)
 	s.accountManagerMock.EXPECT().SelectedAccount().Return(&common.SelectedExtKey{
 		Address: common.FromAddress(TestConfig.Account2.Address),
 	}, nil)
@@ -214,7 +217,7 @@ func (s *TxQueueTestSuite) TestInvalidPassword() {
 		Address:    common.FromAddress(TestConfig.Account1.Address),
 		AccountKey: &keystore.Key{PrivateKey: key},
 	}
-	s.setupStatusBackend(account, password, keystore.ErrDecrypt)
+	s.setupStatusBackend(1, account, password, keystore.ErrDecrypt)
 
 	txQueueManager := NewManager(s.nodeManagerMock, s.accountManagerMock)
 	txQueueManager.DisableNotificactions()
@@ -227,7 +230,6 @@ func (s *TxQueueTestSuite) TestInvalidPassword() {
 	})
 
 	s.NoError(txQueueManager.QueueTransaction(tx))
-
 	_, err := txQueueManager.CompleteTransaction(tx.ID, password)
 	s.Equal(err.Error(), keystore.ErrDecrypt.Error())
 
@@ -279,3 +281,77 @@ func (s *TxQueueTestSuite) TestCompletionTimedOut() {
 	rst := txQueueManager.WaitForTransaction(tx)
 	s.Equal(ErrQueuedTxTimedOut, rst.Error)
 }
+
+// TestLocalNonce verifies that local nonce will be used unless
+// upstream nonce is updated and higher than a local
+// in test we will run 3 transaction with nonce zero returned by upstream
+// node, after each call local nonce will be incremented
+// then, we return higher nonce, as if another node was used to send 2 transactions
+// upstream nonce will be equal to 5, we update our local counter to 5+1
+// as the last step, we verify that if tx failed nonce is not updated
+func (s *TxQueueTestSuite) TestLocalNonce() {
+	txCount := 3
+	password := TestConfig.Account1.Password
+	key, _ := crypto.GenerateKey()
+	account := &common.SelectedExtKey{
+		Address:    common.FromAddress(TestConfig.Account1.Address),
+		AccountKey: &keystore.Key{PrivateKey: key},
+	}
+	s.setupStatusBackend(txCount+2, account, password, nil)
+
+	nonce := hexutil.Uint64(0)
+	gas := hexutil.Big(*big.NewInt(defaultGas + 1))
+	s.setupTransactionPoolAPI(txCount, account, nonce, gas, nil)
+
+	manager := NewManager(s.nodeManagerMock, s.accountManagerMock)
+	manager.DisableNotificactions()
+
+	manager.Start()
+	defer manager.Stop()
+
+	for i := 0; i < txCount; i++ {
+		tx := common.CreateTransaction(context.Background(), common.SendTxArgs{
+			From: common.FromAddress(TestConfig.Account1.Address),
+			To:   common.ToAddress(TestConfig.Account2.Address),
+		})
+
+		s.NoError(manager.QueueTransaction(tx))
+		hash, err := manager.CompleteTransaction(tx.ID, password)
+		rst := manager.WaitForTransaction(tx)
+		// simple sanity checks
+		s.NoError(err)
+		s.NoError(rst.Error)
+		s.Equal(rst.Hash, hash)
+		s.Equal(uint64(i)+1, manager.localNonce[tx.Args.From])
+	}
+	nonce = hexutil.Uint64(5)
+	s.txServiceMock.EXPECT().GetTransactionCount(gomock.Any(), account.Address, gethrpc.PendingBlockNumber).Return(&nonce, nil)
+	s.txServiceMock.EXPECT().GasPrice(gomock.Any()).Return(big.NewInt(10), nil)
+	s.txServiceMock.EXPECT().EstimateGas(gomock.Any(), gomock.Any()).Return(&gas, nil)
+	s.txServiceMock.EXPECT().SendRawTransaction(gomock.Any(), gomock.Any()).Return(gethcommon.Hash{}, nil)
+	tx := common.CreateTransaction(context.Background(), common.SendTxArgs{
+		From: common.FromAddress(TestConfig.Account1.Address),
+		To:   common.ToAddress(TestConfig.Account2.Address),
+	})
+	s.NoError(manager.QueueTransaction(tx))
+	hash, err := manager.CompleteTransaction(tx.ID, password)
+	rst := manager.WaitForTransaction(tx)
+	s.NoError(err)
+	s.NoError(rst.Error)
+	s.Equal(rst.Hash, hash)
+	s.Equal(uint64(nonce)+1, manager.localNonce[tx.Args.From])
+
+	testErr := errors.New("test")
+	s.txServiceMock.EXPECT().GetTransactionCount(gomock.Any(), account.Address, gethrpc.PendingBlockNumber).Return(nil, testErr)
+	tx = common.CreateTransaction(context.Background(), common.SendTxArgs{
+		From: common.FromAddress(TestConfig.Account1.Address),
+		To:   common.ToAddress(TestConfig.Account2.Address),
+	})
+	s.NoError(manager.QueueTransaction(tx))
+	hash, err = manager.CompleteTransaction(tx.ID, password)
+	rst = manager.WaitForTransaction(tx)
+
+	s.EqualError(testErr, err.Error())
+	s.EqualError(testErr, rst.Error.Error())
+	s.Equal(uint64(nonce)+1, manager.localNonce[tx.Args.From])
+}