Use single codepath for sending transactions to a local and remote nodes #527

dshulyak · 2018-01-02T12:30:50Z

Fixes #529

Previously procedure for sending transactions was different based on
which type of node was used, local (light node) or remote.

This change removes separate code path for working with light node,
and the only difference will be backend for rpc client. In case of
light node all of the communication will be done over inproc dial.

Also there is a couple of related changes in this PR:

new EthereumTransactor that provides higher level API for working
with ethereum network, and it is fully conformant with ethclient
new test rpc service that improves flexibility and coverage of
txqueue manager tests

Closes: #529

dshulyak · 2018-01-02T14:53:58Z

@adambabik hello, i believe this change is ready for initial review.
I plan to add more tests for different edge cases and do other refactoring that is described in #338 , but i think it would be better to make separate PRs for those things.

dshulyak · 2018-01-02T15:32:27Z

Looks like i asked prematurely, e2e tests are failing with relevant error.

dshulyak · 2018-01-02T18:31:59Z

At the moment only one test fails - TestCompleteMultipleQueuedTransactions, and I am pretty sure that I know the root cause of this issue. LesBackend internally uses txPool.GetNonce method when it sets the nonce for an incoming transaction. This method looks up a recent nonce at the local map of the pool - pool.nonce. But this method is not exported, and I have to use GetTransactionCount that is not consistent with txPool.GetNonce. I tried to verify it on develop branch:

tx	txPool.GetNonce	GetTransactionCount(pending)
1	0	0
2	1	0
3	2	0

I think I can add another interceptor, to a status rpc.Client, for GetTransactionCount method which will use LesBackend and GetPoolNonce. But I am pretty sure that it is a bug in LightEthereum client.

ethereum/go-ethereum#2880

adambabik · 2018-01-02T21:46:41Z

Hey @dshulyak. Thank you for your contribution. I skimmed through the PR and it looks great! I will submit a detailed review tomorrow.

Regarding the nonce, in the current develop, the code that sends a transaction when using a remote node uses the same JSON-RPC method and arguments to get the nonce as before so the tests should pass just fine, unless these tests use a local node... In this case, a pool is used: https://github.com/ethereum/go-ethereum/blob/master/internal/ethapi/api.go#L1096-L1102 and this go-ethereum issue seems like a root cause. You can hot patch code in vendor/ and see if it fixes this problem. We can incorporate this patch if it will turn out to be necessary.

dshulyak · 2018-01-03T08:39:32Z

@adambabik i don't see a reason why it won't be possible to reproduce this bug with upstream node. maybe it will be required to increase the number of concurrent transactions

adambabik · 2018-01-03T09:36:06Z

@dshulyak you're right, this bug should be also visible when using a remote node. We may not have an automated test for the remote node, though.

To reviewers: the tests were failing because they use a local node and, before changes, they were using LES backend directly instead of the public API (that is eth_sendRawTransaction method). It turns out that the public API has a bug and does not use the transaction pool to return a correct nonce value.

dshulyak · 2018-01-03T09:52:36Z

I think that it is possible yo retry tx on "known transaction" error, both light and full nodes return it. But, i need to understand if there are any other side effects of doing it.

adambabik

It looks great! I added a bunch of comments but they are minor suggestions or code style.

I will create a separate issue for this effort as it's related to #338 indeed, but not exactly.

adambabik · 2018-01-03T09:37:22Z

geth/txqueue/ethtxclient.go

+)
+
+// EthereumTransactor provides methods to create transactions for ethereum network.
+type EthereumTransactor interface {


To keep it consistent, maybe we can call it EthTransactor, similarly to EthTxClient?

adambabik · 2018-01-03T09:40:24Z

geth/txqueue/txqueue_manager.go

+
+	defaultGas = 90000
+
+	cancelTimeout = time.Minute


Maybe defaultTimeout to match defaultGas naming convention?

adambabik · 2018-01-03T09:51:04Z

geth/txqueue/txqueue_manager.go

 	args := queuedTx.Args
-
+	var gasPrice *big.Int


This is fine but else can be omitted:

gasPrice := (*big.Int)(args.GasPrice) if gasPrice == nil { // ... }

adambabik · 2018-01-03T10:02:03Z

geth/txqueue/txqueue_manager_test.go

 	})

 	err := txQueueManager.QueueTransaction(tx)
 	s.NoError(err)

 	var wg sync.WaitGroup
 	var mu sync.Mutex
-	completeTxErrors := make(map[error]int)
+	completeTxErrors := map[error]int{}


I think it should be possible to get rid of this map and only assert errCompleteTransaction to be always nil.

adambabik · 2018-01-03T10:04:10Z

geth/txqueue/txqueue_manager_test.go

-	// As we want to avoid network connection, we mock LES with a known error
-	// and treat as success.
-	s.nodeManagerMock.EXPECT().LightEthereumService().Return(nil, errTxAssumedSent)
+	nodeConfig, nodeErr := params.NewNodeConfig("/tmp", params.RopstenNetworkID, true)


As this block repeats three times, maybe we can extract it to a separate TxQueueTestSuite method?

adambabik · 2018-01-03T10:06:45Z

geth/txqueue/fake/txservice.go

+// FakePublicTxApi used to generate mock by mockgen util.
+// This was done because PublicTransactionPoolAPI is located in internal/ethapi module
+// and there is no easy way to generate mocks from internal modules.
+type FakePublicTxApi interface {


Maybe we can call it FakePublicTransactionPoolAPI to make it clearer what it is?

adambabik · 2018-01-03T10:06:56Z

geth/txqueue/fake/txservice.go

+
+import (
+	context "context"
+	big "math/big"


Is big required?

adambabik · 2018-01-03T10:07:43Z

geth/txqueue/fake/txservice.go

+	"github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/common/hexutil"
+	"github.com/ethereum/go-ethereum/rpc"
+	gomock "github.com/golang/mock/gomock"


I believe gomock is not required before the package name here.

adambabik · 2018-01-03T10:09:03Z

geth/txqueue/fake/mock.go

@@ -0,0 +1,91 @@
+// Code generated by MockGen. DO NOT EDIT.


Please add a proper entry to mock target: https://github.com/status-im/status-go/blob/develop/Makefile#L101-L105

adambabik · 2018-01-03T10:21:48Z

@dshulyak I created a separate issue for this PR: #529.

dshulyak · 2018-01-03T11:04:14Z

@adambabik thanks for a review, i pushed an update

andytudhope · 2018-01-03T11:39:50Z

Hey @dshulyak! Adam suggested that we offer a bounty for this issue because you had done such good work. I have created and funded it and linked this PR to the issue with a minor edit. Could you please log into http://openbounty.status.im/ click on your profile picture, select to Update Your Payment Details and make sure to click the Update button after putting in an address you control?

dshulyak · 2018-01-03T12:50:40Z

that's awesome @andytudhope, I updated my address as you suggested

dshulyak · 2018-01-03T15:30:24Z

Can someone re-trigger a job on travis? It failed due to 'https://github.com/alecthomas/gometalinter/': Failed to connect to github.com port 443: Connection timed out

divan · 2018-01-03T15:39:14Z

@dshulyak done.

adambabik

Sorry about doing this review in two iterations but I found a bit more time just today.

Minor remarks overall, just please make sure to look at the comments relating to TestInvalidPassword and TestCompleteTransaction.

adambabik · 2018-01-04T21:42:49Z

geth/txqueue/txqueue_manager.go

-	} else {
-		hash, err = m.completeLocalTransaction(queuedTx, password)
-	}
+	hash, err := m.completeTransaction(selectedAccount, queuedTx, password)


Minor: I noticed that maybe this arguments grouping could be better: m.completeTransaction(queuedTx, selectedAccount, password)

adambabik · 2018-01-04T21:43:57Z

geth/txqueue/txqueue_manager.go

 	var emptyHash gethcommon.Hash
-
+	log.Info("verifying account password for transaction", "id", queuedTx.ID)


I would move this log just above m.accountManager.VerifyAccountPassword or remove at all.

adambabik · 2018-01-04T21:49:28Z

geth/txqueue/txqueue_manager_test.go

@@ -81,31 +114,30 @@ func (s *TxQueueTestSuite) TestCompleteTransaction() {
 	s.NoError(err)

 	go func() {
-		_, errCompleteTransaction := txQueueManager.CompleteTransaction(tx.ID, TestConfig.Account1.Password)
-		s.Equal(errTxAssumedSent, errCompleteTransaction)
+		_, errCompleteTransaction := txQueueManager.CompleteTransaction(tx.ID, password)


After these changes, we do not expect any errors so we can verify if the returned hash is equal to tx.Hash, right?

Also, a few lines above, there is txQueueManager.SetTransactionReturnHandler(). I think we should also change it to expect err to be nil.

adambabik · 2018-01-04T21:53:15Z

geth/txqueue/txqueue_manager_test.go

 	// Transaction should be already removed from the queue.
 	s.False(txQueueManager.TransactionQueue().Has(tx.ID))

 	// Wait for all CompleteTransaction calls.
 	wg.Wait()
-	s.Equal(completeTxErrors[errTxAssumedSent], 1)
+	s.Equal(1, completedTx, "only 1 tx expected to be completed")
+	s.Equal(txCount-1, inprogressTx, txCount-1, "txs expected to be reported as inprogress")


Shouldn't it be:

s.Equal(txCount-1, inprogressTx, "txs expected to be reported as inprogress")

?

adambabik · 2018-01-04T22:04:58Z

geth/txqueue/txqueue_manager_test.go

-	s.nodeManagerMock.EXPECT().LightEthereumService().Return(nil, keystore.ErrDecrypt)
+	nonce := hexutil.Uint64(10)
+	gas := hexutil.Big(*big.NewInt(defaultGas + 1))
+	s.setupTransactionPoolAPI(account, nonce, gas, keystore.ErrDecrypt)


This test does not test a real scenario. keystore.ErrDecrypt is returned by SendRawTransaction while it should be by VerifyAccountPassword. I would recommend to change that and also make sure that SendRawTransaction is not called.

adambabik · 2018-01-04T22:06:24Z

geth/txqueue/txqueue_manager_test.go

 	})

 	err := txQueueManager.QueueTransaction(tx)
 	s.NoError(err)

 	var wg sync.WaitGroup
 	var mu sync.Mutex
-	completeTxErrors := make(map[error]int)
-	for i := 0; i < 3; i++ {
+	var completedTx int


Minor: as there are multiple vars, it's beneficial to group them with var ().

adambabik · 2018-01-04T22:12:44Z

vendor/github.com/ethereum/go-ethereum/internal/ethapi/api.go

@@ -956,6 +956,14 @@ func (s *PublicTransactionPoolAPI) GetRawTransactionByBlockHashAndIndex(ctx cont

 // GetTransactionCount returns the number of transactions the given address has sent for the given block number
 func (s *PublicTransactionPoolAPI) GetTransactionCount(ctx context.Context, address common.Address, blockNr rpc.BlockNumber) (*hexutil.Uint64, error) {
+	// go-ethereum issue https://github.com/ethereum/go-ethereum/issues/2880


We need to add this to go-ethereum patches. cc @divan

I think there is a way to get rid of it. we can maintain nonce locally for each address that status is using. increment it on every successful transaction, and reset in case if it was changed remotely. it is easy with new addr locker and will prevent issues both with upstream and local nodes.

will do a separate pr after additional testing

It may introduce a lot of complexity into status-go and something that potentially can break in the future if go-ethereum implementation changes. But if you find some elegant solution that would be awesome.

I have seen this PR ethereum/go-ethereum#15794 and read the whole discussion in ethereum/go-ethereum#2880 and it looks like a separate JSON-RPC method is the best solution that can preserve backward compatibility of eth_getTransactionCount and allows to get the number of pending transactions from the pool.

adding a separate method also seemed like a good idea to me, but it may take some time until it will be merged and a new geth release will be available. and even then status app needs to be always aware that upstream node is not an old geth release or another client, e.g parity.

Yeah, that should be implemented via patches to keep track of our changes into the go-ethereum. This is blocked by #492, which will be merged tomorrow (internally agreed deadline).

adambabik

LGTM 👏

adambabik · 2018-01-07T21:05:01Z

testing/testing.go

+	select {
+	case <-c:
+		return nil
+	case <-timer.C:


case <-time.After(d): return ErrTimeout

will do the same.

adambabik · 2018-01-07T21:06:28Z

geth/txqueue/txqueue_manager_test.go

@@ -265,9 +307,11 @@ func (s *TxQueueTestSuite) TestDiscardTransaction() {
 	err := txQueueManager.QueueTransaction(tx)
 	s.NoError(err)

+	w := make(chan struct{})


I am not a fan of one char variables. In such contexts, done := make(chan struct{}) works pretty well and indicates an intention clearly.

dshulyak · 2018-01-08T07:03:23Z

thanks, I will change those things in one of the pr's that is based on this one.

travis failed because of #539

divan

Great work!
The modification to the vendored go-ethereum should be made through patches, so this PR is currently blocked by #492. Will try to resolve this block asap.

adambabik · 2018-01-11T12:17:04Z

geth/txqueue/txqueue_manager.go

-		log.Info("default gas will be used. estimated gas", gas, "is lower than", defaultGas)
-		gas = big.NewInt(defaultGas)
+	gas := (*big.Int)(args.Gas)
+	if args.Gas == nil {


I think it's good. The other way is to pass existing gas value to EstimateGas arguments and it will take it as upper bound, however, a lower value might be returned.

Skipping EstimateGas call if gas is defined should be better, though.

also the idea was to keep EthTxClient compatible with https://github.com/ethereum/go-ethereum/blob/master/ethclient/ethclient.go#L36 , it is not of high priority of course

divan · 2018-01-17T17:58:40Z

As #492 has been merged, changes to internal/ethapi/api.go should be implemented as a patch (see https://github.com/status-im/status-go/tree/develop/geth-patches directory). @dshulyak when you have a time, could you make this change?

The exact way of applying patches to our fork might be changed in #551, but so far patch files are in geth-patches dir.

Previously procedure for sending transactions was different based on which type of node was used, local (light node) or remote. This change removes separate code path for working with light node, and the only difference will be backend for rpc client. In case of light node all of the communication will be done over inproc dial. Also there is a couple of related changes in this PR: - new EthereumTransactor that provides higher level API for working with ethereum network, and it is fully conformant with ethclient - new test rpc service that improves flexibility and coverage of txqueue manager tests

dshulyak · 2018-01-17T18:48:43Z

@divan pushed the patch

divan · 2018-01-17T20:21:26Z

@dshulyak perfect, thanks! Actually, I think we should not wait for #551, because the process of applying patches should be repeatable anyway, so we can add new patches meanwhile. Thus, unblocking this PR now.

divan · 2018-01-18T10:31:25Z

Trying to understand why tests on Travis time outs before merging.

divan · 2018-01-18T16:55:58Z

@dshulyak awesome job.

This change is not needed after the following PR was merged: #527

dshulyak changed the title ~~Use single codepath for sending transactions to a local and remote nodes~~ [WIP] Use single codepath for sending transactions to a local and remote nodes Jan 2, 2018

dshulyak force-pushed the txmanager branch 4 times, most recently from 31f4cd7 to f4f5034 Compare January 2, 2018 14:50

dshulyak changed the title ~~[WIP] Use single codepath for sending transactions to a local and remote nodes~~ Use single codepath for sending transactions to a local and remote nodes Jan 2, 2018

dshulyak force-pushed the txmanager branch 2 times, most recently from db1b531 to dcc0874 Compare January 2, 2018 16:02

dshulyak force-pushed the txmanager branch 2 times, most recently from 316e0f8 to 10b69eb Compare January 3, 2018 08:26

adambabik suggested changes Jan 3, 2018

View reviewed changes

dshulyak mentioned this pull request Jan 3, 2018

Refactoring of TxQueue and Manager #530

Merged

adambabik suggested changes Jan 4, 2018

View reviewed changes

dshulyak force-pushed the txmanager branch 4 times, most recently from 29c5534 to 55bcd13 Compare January 5, 2018 19:51

adambabik approved these changes Jan 7, 2018

View reviewed changes

divan approved these changes Jan 8, 2018

View reviewed changes

adambabik added the blocked label Jan 10, 2018

adambabik reviewed Jan 11, 2018

View reviewed changes

dshulyak and others added 6 commits January 17, 2018 20:12

Run complete transaction sequantially for each address

5acd03c

go-ethereum: Get pending nonce from transaction pool

4c54ecc

Add mock generation to makefile and cleanup tests

4c51b67

Do not ignore gas argument receive from client

29fda61

Add a patch with getting nonce from transaction pool

5c40903

dshulyak force-pushed the txmanager branch from fa7e42e to 5c40903 Compare January 17, 2018 18:47

divan removed the blocked label Jan 17, 2018

Remove travis_wait from public tests

953a847

divan merged commit 0771e7d into status-im:develop Jan 18, 2018

dshulyak deleted the txmanager branch January 18, 2018 16:57

dshulyak restored the txmanager branch January 18, 2018 16:57

dshulyak added a commit that referenced this pull request Feb 15, 2018

Remove status backend from les internal api

6aae94d

This change is not needed after the following PR was merged: #527

dshulyak added a commit that referenced this pull request Feb 15, 2018

Remove status backend from les internal api

b1b0a7b

This change is not needed after the following PR was merged: #527

dshulyak mentioned this pull request Feb 15, 2018

Remove status backend from les internal api #670

Merged

pedropombeiro pushed a commit that referenced this pull request Feb 15, 2018

Remove status backend from les internal api

853c5ef

This change is not needed after the following PR was merged: #527

pedropombeiro pushed a commit that referenced this pull request Feb 16, 2018

Remove status backend from les internal api

863209f

This change is not needed after the following PR was merged: #527

		var emptyHash gethcommon.Hash

		log.Info("verifying account password for transaction", "id", queuedTx.ID)

Use single codepath for sending transactions to a local and remote nodes #527

Use single codepath for sending transactions to a local and remote nodes #527

Conversation

dshulyak commented Jan 2, 2018 • edited by andytudhope Loading

dshulyak commented Jan 2, 2018

dshulyak commented Jan 2, 2018

dshulyak commented Jan 2, 2018 • edited Loading

adambabik commented Jan 2, 2018

dshulyak commented Jan 3, 2018

adambabik commented Jan 3, 2018

dshulyak commented Jan 3, 2018

adambabik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambabik commented Jan 3, 2018

dshulyak commented Jan 3, 2018

andytudhope commented Jan 3, 2018

dshulyak commented Jan 3, 2018

dshulyak commented Jan 3, 2018

divan commented Jan 3, 2018

adambabik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambabik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshulyak commented Jan 8, 2018

divan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divan commented Jan 17, 2018

dshulyak commented Jan 17, 2018

divan commented Jan 17, 2018

divan commented Jan 18, 2018

divan commented Jan 18, 2018

dshulyak commented Jan 2, 2018 •

edited by andytudhope

Loading

dshulyak commented Jan 2, 2018 •

edited

Loading