Add stateful checkTx #276

adrianbrink · 2017-08-19T20:30:38Z

add tests
double check the logic in go-ethereum for validateTx and update it accordingly

codecov · 2017-08-19T20:30:39Z

Codecov Report

Merging #276 into develop will increase coverage by 1.86%.
The diff coverage is 55.55%.

@@             Coverage Diff             @@
##           develop     #276      +/-   ##
===========================================
+ Coverage    24.91%   26.77%   +1.86%     
===========================================
  Files           11       11              
  Lines          578      605      +27     
===========================================
+ Hits           144      162      +18     
- Misses         410      417       +7     
- Partials        24       26       +2

Impacted Files	Coverage Δ
ethereum/backend.go	`0% <ø> (ø)`	⬆️
ethereum/txs.go	`0% <0%> (ø)`	⬆️
app/app.go	`42.26% <58.13%> (+4.93%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9518a4d...7af08f8. Read the comment docs.

odeke-em · 2017-08-19T22:35:11Z

@adrianbrink could you please educate me on the benefit from using a cached state for this purpose other than the normal benefits of using a cached copy of something(avoid extraneous lookup)?
However, I figure that for Ethereum, the state is continually changing so we'd rather always query the current copy.

adrianbrink · 2017-08-20T07:27:21Z

The purpose of a cachedState here is that it allows us to send multiple TXs from the same account in a single block.

When I send a transaction J from account A, I increase the nonce from x to x+1. When Ethermint receives a transaction in CheckTx it checks that this condition is met, however it doesn't update any state so it always check against the state from the last block.
Let's say I send another transaction G from account A in the same block. I dutifully increase the nonce from x+1 to x+2. When Ethermint receives transaction G in CheckTx it will return an error, since the nonce isn't strictly increasing. This time the nonce jumped by 2 instead of 1.

Another reason is that with transaction H I might create a new account B. This account won't show up in the state that checkTx sees until the next block is created. However it is perfectly valid to send a transaction from account B in the same block, which won't work without state, since Ethermint currently thinks that account B doesn't exist.

If we have cached state all these use cases work.

ebuchman · 2017-08-22T19:00:33Z

app/app.go

@@ -207,6 +218,10 @@ func (app *EthermintApplication) validateTx(tx *ethTypes.Transaction) abciTypes.
 			AppendLog(core.ErrInvalidSender.Error())
 	}

+	// Update ether balances
+	currentState.SubBalance(from, tx.Value())


what about subtracting gas fees ?

these should also probably go after the checks for having enough.

ebuchman · 2017-08-22T19:00:57Z

app/app_test.go

@@ -132,7 +132,6 @@ func TestBumpingNonces(t *testing.T) {
 	stack.Stop() // nolint: errcheck
 }

-// TestMultipleTxOneAcc sends multiple TXs from the same account in the same block


why lose the comment ?

ebuchman · 2017-08-22T19:32:54Z

app/app_test.go

-	stack, err := makeTestSystemNode(tempDatadir, addresses, mockclient)
+// Test transaction from Acc1 to new Acc2 and then from Acc2 to another address
+// in the same block
+func TestFromAccToAcc(t *testing.T) {


nice. did we check this fails without the rest of this PR ?

yes, here https://github.com/tendermint/ethermint/blob/develop/app/app_test.go#L172

ebuchman · 2017-08-22T19:38:10Z

app/app.go

@@ -164,6 +173,11 @@ func (app *EthermintApplication) Commit() abciTypes.Result {
 		app.logger.Error("Error getting latest ethereum state", "err", err) // nolint: errcheck
 		return abciTypes.ErrInternalError.AppendLog(err.Error())
 	}
+	state, err := app.getCurrentState()
+	if err != nil {
+		app.logger.Error("Error getting latest state", "err", err) // nolint: errcheck


what does it take for this to err? do we need to catch it properly?

It could happen if the underlying ethereum state gets corrupted.

We might want to abort here.

For now it only affects the checktx state though.

presumably state will be nil if there is an error and the call to Copy below will panic. So we should instead catch that and return an InternalError result so Tendermint knows something went wrong, logs the error, and halts

jaekwon · 2017-08-23T10:01:05Z

app/app.go

+		backend:         backend,
+		rpcClient:       client,
+		getCurrentState: backend.Ethereum().BlockChain().State,
+		cacheState:      state.Copy(),


I think maybe call it checkTxState, to emphasize that it's not for deliverTx caching.

jaekwon · 2017-08-23T11:01:19Z

app/app.go

 	from, err := ethTypes.Sender(signer, tx)
 	if err != nil {
 		return abciTypes.ErrBaseInvalidSignature.
 			AppendLog(core.ErrInvalidSender.Error())
 	}

+	// Heuristic limit, reject transactions over 32KB to prevent DOS attacks
+	if tx.Size() > 32*1024 {


This should be a const declared above, at least.

Why 32KB? This seems to break from Ethereum...

jaekwon · 2017-08-23T11:04:25Z

app/app.go

-		return abciTypes.ErrBaseInvalidInput.
-			SetLog(core.ErrNegativeValue.Error())
+	// Check for nonce errors
+	if currentState.GetNonce(from) > tx.Nonce() {


Nonce needs to be strictly incrementing by 1.

Ethereum itself doesn't currently do that.

What is the benefit of doing it our way?

https://ethereum.stackexchange.com/questions/2808/what-happens-when-a-transaction-nonce-is-too-high

See that Ethereum blocks still require nonces to be strictly incrementing by 1, even if there is no "ErrNonceTooHigh" error, due to the way the mempool works in Ethereum. (nonces that are too high are kept in the mempool for a while).

We should just use this error instead: https://github.com/tendermint/abci/blob/master/types/types.proto#L15

For future reference:

For a transaction to be valid it needs to have the same nonce as the account that sends it.

Do we still need this check if we just check for equality directly below ?

No, we don't. I removed it.

jaekwon · 2017-08-23T11:15:12Z

app/app.go

+	// Update ether balances
+	// amount + gasprice * gaslimit
+	currentState.SubBalance(from, tx.Cost())
+	currentState.AddBalance(*tx.To(), tx.Value())


Nonce should be incremented as well.

Can we test this nonce issue?

Added it on top of the file TestIncrementingNonces.

jaekwon · 2017-08-23T11:15:58Z

app/app_test.go

@@ -49,10 +49,12 @@ func (mc *MockClient) Call(method string, params map[string]interface{}, result
 	switch method {
 	case "status":
 		result = &ctypes.ResultStatus{}
+


Kinda strange to have blank spaces here.

jaekwon · 2017-08-23T11:22:08Z

app/app_test.go

+// 1. put Acc1 and Acc2 to genesis with some amounts (X)
+// 2. transfer 10 amount from Acc1 to Acc2
+// 3. in the same block transfer from Acc2 to another Acc all his amounts (X+10)
+func TestFromAccToAcc2(t *testing.T) {


There's a lot of repeated code between this and the one above it.
(I think) we should refactor the tests to pull out the common components, and also create a new test that ensures that two checkTxs where the second has the wrong nonce (e.g. too high or too low) will fail.

jaekwon · 2017-08-23T11:22:42Z

setup/genesis.json

@@ -13,6 +13,6 @@
    "gasLimit": "0x8000000",
    "alloc": {
        "0x7eff122b94897ea5b0e2a9abf47b86337fafebdc": { "balance": "10000000000000000000000000000000000" },
-	    "0xc6713982649D9284ff56c32655a9ECcCDA78422A": { "balance": "10000000000000000000000000000000000" }
+	      "0xc6713982649D9284ff56c32655a9ECcCDA78422A": { "balance": "10000000000000000000000000000000000" }


some spacing issue, not aligned with the line above this.

jaekwon

Almost done, please address these issues!

adrianbrink · 2017-08-25T15:42:31Z

I refactored the app_test.go file, so that it is much cleaner now. The setup is now being done in test local functions as well as the teardown. It uses some functional magic to handle the teardown of test cases. I'm a big fan of this approach.

@odeke-em I would appreciate a review :-)

Just need to run make test_integrations

zramsay · 2017-10-09T12:58:11Z

TODO.md

@@ -0,0 +1,3 @@
+# TODO
+
+* Document performance numbers for Tendermint and Ethermint and write a blog post about it


this can just be an issue, no? TODO files in a repo are unnecessary IMO ... it confuses what there is to do with issues

Tendermint RPC lib seems broken

CLAassistant · 2017-10-12T16:01:43Z

All committers have signed the CLA.

* Add stateful checkTx * Tests for stateful checktx * Mirror validateTx logic from tx_pool.go. Move checkTxState updates * Move transaction size limit into const. Remove uneeded fmt import * Enable nonces to be strictly increasing * Extract all directory setup into functional style tests This change removes the duplicated code to setup temporary directories for each test. Now all code is grouped in the setupTestCase() function instead of being littered around all the test cases. There are no side effects of this. * Refactor app_test.go to remove the duplicated code The tests where getting unwieldy since they duplicated a lot of code. With the new setup it is much easier to create new test cases, since the setup and tear-down are handled nicely. There are no side effects. * Add test case for non-incremental nonces This test case checks that transactions have to have strictly increasing nonces. It tests by sending a transaction with nonce 0, and then tries to send a transaction with nonce 2. The second transaction should fail, since it is not strictly increasing. Some of the error descriptions have changed, but no one should be matching for a specific error string. * Refactor tests and RPC library This change is the last refactor of the app_tests and was needed for clarity. Also, it upgrades everything to use a typed RPC client instead of passing strings to get to raw endpoints. * Add check whether it's contract creation * Update Changelog Signed-off-by: Adrian Brink <[email protected]>

* update rpc tests * cleanup * add log assertion to getTransacionReceipt * fix queurier_test * address comment

adrianbrink added the Status: In Progress label Aug 19, 2017

adrianbrink self-assigned this Aug 19, 2017

adrianbrink added Status: Review Needed and removed Status: In Progress labels Aug 20, 2017

ebuchman reviewed Aug 22, 2017

View reviewed changes

adrianbrink added Status: Revision Needed and removed Status: Review Needed labels Aug 23, 2017

jaekwon reviewed Aug 23, 2017

View reviewed changes

adrianbrink added Status: Review Needed and removed Status: Revision Needed labels Aug 23, 2017

adrianbrink force-pushed the feature/checktx branch from 2862557 to 6e00ad4 Compare August 23, 2017 10:43

jaekwon reviewed Aug 23, 2017

View reviewed changes

jaekwon previously requested changes Aug 23, 2017

View reviewed changes

adrianbrink added Status: Revision Needed and removed Status: Review Needed labels Aug 24, 2017

adrianbrink changed the title ~~Add stateful checkTx~~ WIP: Add stateful checkTx Aug 24, 2017

adrianbrink force-pushed the feature/checktx branch from 7315a50 to 2b71315 Compare August 25, 2017 15:17

adrianbrink mentioned this pull request Aug 27, 2017

WIP: Refactor tests for app. Use tendermint/rpc/client instead of tendermint/rpc/lib/client #279

Closed

1 task

adrianbrink added Status: Review Needed and removed Status: Revision Needed labels Aug 27, 2017

Adrian Brink and others added 13 commits October 3, 2017 13:20

Implement feedback

409f867

Add check whether it's contract creation

715eae8

Update Changelog

b98d916

test

44b3494

test1

4849530

t

529c854

Finally debugged

c3891dd

Work through the PR again. Most of this is solved.

13f95a9

Just need to run make test_integrations

Final CheckTx fixes

d45e5d0

Remove redundant test

54b37ed

Triggering rebuild

19eb957

minor changes

89dc3ad

Add TODO.md file

c139ec6

adrianbrink force-pushed the feature/checktx branch from a74445c to c139ec6 Compare October 3, 2017 11:27

adrianbrink removed the Status: Review Needed label Oct 3, 2017

zramsay reviewed Oct 9, 2017

View reviewed changes

Refactor code

e8505a1

Tendermint RPC lib seems broken

Adrian Brink added 2 commits October 13, 2017 16:38

Disable integration tests

6326ef1

Remove last line

7af08f8

adrianbrink merged commit ace6c40 into develop Oct 16, 2017

adrianbrink deleted the feature/checktx branch October 16, 2017 08:34

zramsay mentioned this pull request Oct 24, 2017

Enable stateful CheckTx #9

Closed

i-norden pushed a commit to vulcanize/old_ethermint that referenced this pull request Aug 31, 2020

update rpc tests (cosmos#276)

ce0feb3

* update rpc tests * cleanup * add log assertion to getTransacionReceipt * fix queurier_test * address comment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stateful checkTx #276

Add stateful checkTx #276

adrianbrink commented Aug 19, 2017 •

edited

Loading

codecov bot commented Aug 19, 2017 •

edited

Loading

odeke-em commented Aug 19, 2017

adrianbrink commented Aug 20, 2017

ebuchman Aug 22, 2017

ebuchman Aug 22, 2017

ebuchman Aug 22, 2017

ebuchman Aug 22, 2017

ramilexe Aug 22, 2017

ebuchman Aug 22, 2017

adrianbrink Aug 23, 2017

ebuchman Sep 12, 2017

jaekwon Aug 23, 2017

jaekwon Aug 23, 2017

jaekwon Aug 23, 2017

jaekwon Aug 23, 2017

adrianbrink Aug 23, 2017 •

edited

Loading

jaekwon Aug 23, 2017

adrianbrink Aug 23, 2017

ebuchman Aug 28, 2017

adrianbrink Aug 28, 2017

jaekwon Aug 23, 2017

jaekwon Aug 23, 2017

adrianbrink Aug 25, 2017

jaekwon Aug 23, 2017

jaekwon Aug 23, 2017

jaekwon Aug 23, 2017

jaekwon left a comment

adrianbrink commented Aug 25, 2017

zramsay Oct 9, 2017

adrianbrink Oct 10, 2017

CLAassistant commented Oct 12, 2017 •

edited

Loading

		@@ -0,0 +1,3 @@
		# TODO

		* Document performance numbers for Tendermint and Ethermint and write a blog post about it

Add stateful checkTx #276

Add stateful checkTx #276

Conversation

adrianbrink commented Aug 19, 2017 • edited Loading

codecov bot commented Aug 19, 2017 • edited Loading

Codecov Report

odeke-em commented Aug 19, 2017

adrianbrink commented Aug 20, 2017

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

adrianbrink Aug 23, 2017 • edited Loading

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

jaekwon left a comment

Choose a reason for hiding this comment

adrianbrink commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Oct 12, 2017 • edited Loading

adrianbrink commented Aug 19, 2017 •

edited

Loading

codecov bot commented Aug 19, 2017 •

edited

Loading

adrianbrink Aug 23, 2017 •

edited

Loading

CLAassistant commented Oct 12, 2017 •

edited

Loading