Implement CIP35 for Donut hard fork #1431

oneeman · 2021-03-11T20:34:30Z

Description

This PR implements CIP35 as specified at https://github.com/celo-org/celo-proposals/blob/master/CIPs/cip-0035.md.

Summary of changes:

Transaction type has a new field EthCompatible to distinguish between normal transactions (with the Celo-only fields) and ethereum-compatible ones (without those fields)
When decoding transactions of RLP, it identifies the type based on the number of fields the RLP list has
When encoding transactions to RLP, it encodes them to have either 9 or 12 fields depending on the value of EthCompatible
When generating or verifying transaction signatures, the signing data used is determined by the value of EthCompatible
When serializing to or deserializing from JSON, the EthCompatible field is encoded as ethCompatible.
When validating transactions (whether in the tx pool or in the state transition function), reject them if either
(a) EthCompatible is true but Donut has not been activated,
(b) EthCompatible is true but they have non-trivial values for any of the Celo-only fields (which can't happen if decoding from RLP, but can if decoding from JSON), or
(c) Donut has been activated but the transaction doesn't use replay protection (EIP-155)

Other changes

Fix an upstream bug in RLP decoding (fixed upstream by rlp: handle case of normal EOF in Stream.readFull() ethereum/go-ethereum#22336)
Remove the rlp:nil tag from txdata.GatewayFee. This tag is not respected for *big.Int values, and so its presence was misleading.
Point the CI to use the celo-monorepo branch oneeman/cip35-e2e-tests. This was not strictly speaking necessary, since I have a PR on celo-monorepo pointing at this branch, the tests will execute there. Regardless, this change will be reverted to point it back to master

Tested

Unit tests for serialization an deserialization of transactions (for both RLP and JSON)
e2e tests (see Add e2e tests for CIP35 (eth-compatible transactions) celo-monorepo#7418) which cover combinations of scenarios such as pre-Donut vs post-Donut, eth-compatible vs normal transactions, sending to a full node or a light client, including vs not including Celo-only fields, sending using eth_sendTransaction vs using eth_sendRawTransaction.

Backwards compatibility

Transaction and other types like it (e.g. RPCTransaction) have a new field. However, as the zero value for boolean fields is false (which means normal Celo transactions), no changes should be required for software importing celo-blockchain packages as a library.
When Donut is activated, a new transaction type will be valid which isn't currently
When Donut is activated, transactions without EIP-155 replay protection (i.e. using the chain id) will be rejected. However, all known Celo wallets and all up to date Ethereum wallets use replay protection, so this isn't likely to affect anyone.
Before and after Donut, transactions serialized to JSON will have a new boolean field, ethCompatible. This should not break existing software as additional fields are generally ignored during JSON parsing. In particular, contractkit is not affected, as may be seen in the e2e tests.

Big ints are encoded and decoded as RLP scalars, which are not nullable. The 'nil' tag had no effect, so this commit removes it.

This is necessary in order to verify non-EIP155 signatures, which are supported as EIP-155 does not require clients to use replay-protection.

core/types/transaction.go

mcortesi

Left some comments

core/types/transaction.go

mcortesi · 2021-03-16T20:19:45Z

core/types/transaction_signing.go

+			tx.data.Payload,
+			s.chainId, uint(0), uint(0),
+		})
+	} else {


golang lint rules would ask you to remove the else claus and just return

Turns out we don't have golint (or any drop-in replacement) enabled in our golangci config. If I add it, it gives a ton of errors of different sorts (including some of this rule, e.g. https://github.com/celo-org/celo-blockchain/blob/master/rpc/client.go#L392-L396)

Personally, I am not a fan of this if/else lint rule. I remember when I first used golint I didn't like it and also didn't like that golint doesn't let you disable individual rules. But if you like that rule I can change them.

mcortesi · 2021-03-16T20:19:54Z

core/types/transaction_signing.go

+			tx.data.Amount,
+			tx.data.Payload,
+		})
+	} else {


core/state_transition.go

mcortesi · 2021-03-16T20:27:22Z

core/tx_pool.go

 	// Update all fork indicator by next pending block number.
 	next := new(big.Int).Add(newHead.Number, big.NewInt(1))
 	pool.istanbul = pool.chainconfig.IsIstanbul(next)
+	pool.donut = pool.chainconfig.IsDonut(next)


do we call validateTx after this is enabled? Or old tx will continue to existt on the txpool?

Good point! validateTx() is only called when adding a transaction. For later on it only checks a subset of what validateTx() checks, and this should be checked as well.

I added a commit making it so that unprotected transactions are removed when Donut is activated. I still need to add a test for it, though.

internal/ethapi/api.go

signer/fourbyte/validation.go

nategraf

Gave this a look and didn't have any comments beyond what @mcortesi already added 👍

…sntCompatible

… is activated

oneeman · 2021-03-18T18:59:45Z

Added a test for pool.handleDonutActivation. Ready for final review

trianglesphere

What's here looks good to me. Looks like there's a tx pool conflict, and the CI branches need to be straightened out, but otherwise everything looks ready.

Conflicts: core/types/transaction.go

oneeman · 2021-03-22T20:57:44Z

What's here looks good to me. Looks like there's a tx pool conflict, and the CI branches need to be straightened out, but otherwise everything looks ready.

Thanks! Merged in master and resolved the conflict.

…e the monorepo PR is merged)

oneeman · 2021-03-22T20:59:44Z

Pointed the CI back to monorepo master. This means the new e2e tests won't run, but they will after the corresponding PR on monorepo is merged (I believe this is a chicken-and-egg scenario, but I could be wrong)

### Description This PR adds e2e tests for CIP35 (support for eth-compatible transactions), to be included in the Donut hard fork. ### Other changes * Modifies the `GenesisConfig` type to allow `null` values for `churritoBlock` and `donutBlock`, which indicate the forks are not to be activated. This is necessary because mycelo (used in the new e2e tests) defaults to activating them, so in order to have them not activated we must be able to specify `null` in the config rather than `undefined` (which means no override). * Sets CI to use the branch `oneeman/cip35` of `celo-blockchain`. This should be reverted to `master` once that branch has been merged, before merging this PR. So I added the "do not merge" label to this PR, to ensure it's not merged until that is done. * Moves the mnemonic used for e2e tests out of the function it was in to be a top-level exported constant. This lets the e2e tests use the mnemonic directly if they need to (and also allows them to derive the validator's address rather than hard-code it and relying on the mnemonic not having changed). * Adds support for `verbose: true` when using mycelo to create a genesis file (to not hide mycelo's console output if verbose is true) ### Tested * e2e tests as part of CI ### Related issues - PR on celo-blockchain: celo-org/celo-blockchain#1431 ### Backwards compatibility No issues, but will need to point back to `celo-blockchain`'s master before merging.

Starting at the Donut hard-fork activation: (a) adds support for ethereum-compatible transactions as specified in CIP35, (b) make replay protection (using EIP-155) mandatory. * Remove rlp:"nil" tag from txdata.GatewayFee. Big ints are encoded and decoded as RLP scalars, which are not nullable. The 'nil' tag had no effect, so this commit removes it. * rlp: handle case of normal EOF in Stream.readFull() * Add support for eth-compatible transactions * Make replay protection mandatory once Donut is activated

Or Neeman added 2 commits March 11, 2021 14:16

Remove rlp:"nil" tag from txdata.GatewayFee

f6efb8f

Big ints are encoded and decoded as RLP scalars, which are not nullable. The 'nil' tag had no effect, so this commit removes it.

rlp: handle case of normal EOF in Stream.readFull()

1978dbf

oneeman added do not merge donut labels Mar 11, 2021

oneeman mentioned this pull request Mar 11, 2021

Add e2e tests for CIP35 (eth-compatible transactions) celo-org/celo-monorepo#7418

Merged

oneeman force-pushed the oneeman/cip35 branch 2 times, most recently from f702603 to 5600d27 Compare March 11, 2021 22:15

Or Neeman added 4 commits March 12, 2021 11:16

Add support for eth-compatible transactions

bfbcaf4

Temporary: point CI to monorepo branch oneeman/cip35-e2e-tests

4d9fc73

Support EthCompatible flag in Homestead/Frontier signers.

4d2db7e

This is necessary in order to verify non-EIP155 signatures, which are supported as EIP-155 does not require clients to use replay-protection.

Make replay protection mandatory once Donut is activated

066a975

oneeman force-pushed the oneeman/cip35 branch from 37a9ac7 to 066a975 Compare March 12, 2021 17:42

Fix typo in comment

25e5906

oneeman requested a review from mcortesi March 15, 2021 21:24

oneeman removed the do not merge label Mar 15, 2021

oneeman marked this pull request as ready for review March 15, 2021 21:25

oneeman requested review from a team, hbandura and nategraf as code owners March 15, 2021 21:25

mcortesi reviewed Mar 16, 2021

View reviewed changes

core/types/transaction.go Outdated Show resolved Hide resolved

mcortesi reviewed Mar 16, 2021

View reviewed changes

nategraf reviewed Mar 16, 2021

View reviewed changes

oneeman force-pushed the oneeman/cip35 branch from 72211fc to 5910d1d Compare March 17, 2021 19:51

Or Neeman added 5 commits March 17, 2021 15:14

Rename constant to use golang naming convention

e143b6a

Reduce duplication of logic for checking ErrEthCompatibleTransactionI…

5f3dece

…sntCompatible

Rename and simplify 'ethcompatibletxdata' type

b52e860

Use named fields for converting to and from 'ethCompatibleTxRlpList'

acd5d1f

Move txdata type definition up to where it was before

ba3f60c

oneeman force-pushed the oneeman/cip35 branch from 5910d1d to 00257cb Compare March 17, 2021 21:14

Remove transactions without replay protection from tx pool when Donut…

6cb715a

… is activated

oneeman force-pushed the oneeman/cip35 branch from 00257cb to 6cb715a Compare March 17, 2021 22:45

Add unit test for pool.handleDonutActivation()

a92fb19

oneeman force-pushed the oneeman/cip35 branch from 67c6157 to a92fb19 Compare March 17, 2021 22:47

trianglesphere approved these changes Mar 22, 2021

View reviewed changes

Merge branch 'master' into oneeman/cip35

184d680

Conflicts: core/types/transaction.go

Point CI back to monorepo master (the new e2e test will run again onc…

d157568

…e the monorepo PR is merged)

oneeman merged commit 021d106 into master Mar 23, 2021

oneeman deleted the oneeman/cip35 branch March 23, 2021 15:09

Implement CIP35 for Donut hard fork #1431

Implement CIP35 for Donut hard fork #1431

Uh oh!

Conversation

oneeman commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Other changes

Tested

Backwards compatibility

Uh oh!

Uh oh!

mcortesi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mcortesi Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

oneeman Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

mcortesi Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcortesi Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

oneeman Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

oneeman Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nategraf left a comment

Choose a reason for hiding this comment

Uh oh!

oneeman commented Mar 18, 2021

Uh oh!

trianglesphere left a comment

Choose a reason for hiding this comment

Uh oh!

oneeman commented Mar 22, 2021

Uh oh!

oneeman commented Mar 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

oneeman commented Mar 11, 2021 •

edited

Loading