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

Maintain local copy of the nonce for each used address #538

Merged
merged 3 commits into from
Feb 7, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 0 additions & 19 deletions geth-patches/0008-tx-pool-nonce.patch

This file was deleted.

4 changes: 1 addition & 3 deletions geth-patches/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@ We try to minimize number and amount of changes in those patches as much as poss
- [`0004-whisper-notifications.patch`](./0004-whisper-notifications.patch) — adds Whisper notifications (need to be reviewed and documented)
- [`0006-latest-cht.patch`](./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`](./0007-README.patch) — update upstream README.md.
- [`0008-tx-pool-nonce.patch`](./0008-tx-pool-nonce.patch) - On GetTransactionCount request with PendingBlockNumber get the nonce from transaction pool
- [`0010-geth-17-fix-npe-in-filter-system.patch`](./0010-geth-17-fix-npe-in-filter-system.patch) - Temp patch for 1.7.x to fix a NPE in the filter system

- [`0010-geth-17-fix-npe-in-filter-system.patch`](./0010-geth-17-fix-npe-in-filter-system.patch) - Temp patch for 1.7.x to fix a NPE in the filter system.
# Updating upstream version

When a new stable release of `go-ethereum` comes out, we need to upgrade our fork and vendored copy.
Expand Down
57 changes: 39 additions & 18 deletions geth/transactions/txqueue_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@ package transactions
import (
"context"
"math/big"
"sync"
"time"

ethereum "github.com/ethereum/go-ethereum"
gethcommon "github.com/ethereum/go-ethereum/common"
"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"
)

Expand All @@ -29,10 +31,12 @@ type Manager struct {
accountManager common.AccountManager
txQueue *queue.TxQueue
ethTxClient EthTransactor
addrLock *AddrLocker
notify bool
completionTimeout time.Duration
rpcCallTimeout time.Duration

addrLock *AddrLocker
localNonce sync.Map
}

// NewManager returns a new Manager.
Expand All @@ -45,6 +49,7 @@ func NewManager(nodeManager common.NodeManager, accountManager common.AccountMan
notify: true,
completionTimeout: DefaultTxSendCompletionTimeout,
rpcCallTimeout: defaultTimeout,
localNonce: sync.Map{},
}
}

Expand Down Expand Up @@ -128,19 +133,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)
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: I'd still put tx on a first place.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't get config from Manager?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mainly to avoid multiple calls in mocks

selectedAccount, err := m.accountManager.SelectedAccount()
if err != nil {
log.Warn("failed to get a selected account", "err", err)
Expand All @@ -151,29 +159,42 @@ 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)
m.addrLock.LockAddr(queuedTx.Args.From)
defer m.addrLock.UnlockAddr(queuedTx.Args.From)
var localNonce uint64
if val, ok := m.localNonce.Load(queuedTx.Args.From); ok {
localNonce = val.(uint64)
}
var nonce uint64
defer func() {
// nonce should be incremented only if tx completed without error
// if upstream node returned nonce higher than ours we will stick to it
if err == nil {
m.localNonce.Store(queuedTx.Args.From, nonce+1)
}
m.addrLock.UnlockAddr(queuedTx.Args.From)

}()
ctx, cancel := context.WithTimeout(context.Background(), m.rpcCallTimeout)
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 upstream node returned nonce higher than ours we will use it, as it probably means
// that another client was used for sending transactions
if localNonce > nonce {
nonce = localNonce
}
args := queuedTx.Args
gasPrice := (*big.Int)(args.GasPrice)
if args.GasPrice == nil {
Expand Down
Loading