Omit ethCompatible on RPC responses for non legacy txs

[piersy]

Our RPC responses for non legacy transactions (txType != 0) were returning the ethCompatible field set to false, which is confusing because some of those transactions are eth compatible. The ethCompatible flag was added only to allow distinguishing LegacyTransactions that were compatible with ethereum vs those that were not. This was needed because the LegacyTransaction struct was overloaded to handle celo legacy transactions (with feeCurrency, gatewayFee & gatewayFeeRecipient optionally set) and eth legacy transactions that wouldn't set those fields. The only thing that distinguished a legacy eth transaction from a legacy celo transaction that didn't set any of feeCurrency, gatewayFee & gatewayFeeRecipient would be the signature. So rather than requiring multiple signature verification attempts in order to determine the type of the transaction an extra field was added that would indicate if it was eth compatible.

[github-actions[bot]]

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit e67c1fd73f4e4441bc55a5b3793ba11549a53d2d

coverage: 51.3% of statements across all listed packages

coverage:  63.2% of statements in consensus/istanbul
coverage:  43.3% of statements in consensus/istanbul/announce
coverage:  56.2% of statements in consensus/istanbul/backend
coverage:   0.0% of statements in consensus/istanbul/backend/backendtest
coverage:  24.3% of statements in consensus/istanbul/backend/internal/replica
coverage:  66.0% of statements in consensus/istanbul/core
coverage:  50.0% of statements in consensus/istanbul/db
coverage:   0.0% of statements in consensus/istanbul/proxy
coverage:  64.2% of statements in consensus/istanbul/uptime
coverage:  51.8% of statements in consensus/istanbul/validator
coverage:  79.2% of statements in consensus/istanbul/validator/random

[github-actions[bot]]

[piersy]

Hi @karlb I'm not sure what you mean, generally our changes go into master and at some point we make a release. What's different here?

[piersy]

Hi @arthurgousset, that seems like a sensible change and in line with the change here, so feel free to add it to this PR. Thanks!

[karlb]

Hi @karlb I'm not sure what you mean, generally our changes go into master and at some point we make a release. What's different here?

I was wondering if we can just update forno after we release this or if we need some longer period for people to notice such a change, if we have any way to check if this breaks anyones code, etc. I guess it's fine and we expect people to notice on the testnet before going to mainnet or read the changelog carefully on release.

[arthurgousset]

Hi @arthurgousset, that seems like a sensible change and in line with the change here, so feel free to add it to this PR. Thanks!

That's done in https://github.com/celo-org/celo-blockchain/pull/2302/commits/a190929414fe46845f1db5e2593b32c1a55a38fc ✅ Re-requested a review from @karlb.

[arthurgousset]

Looks like "End-to-end transfers test" and "Unit tests" are failing :(

[karlb]

The tests are broken by https://github.com/celo-org/celo-monorepo/blob/f08e269d88c6680641427952ce4743a7d26336a6/packages/celotool/src/e2e-tests/transfer_tests.ts#L494 and probably other similar checks. I'll take care of it.