feat[l2geth]: Pass up contract revert reasons during DoEstimateGas #774

smartcontracts · 2021-05-05T23:43:11Z

Description
Attempting to add some tweaks to geth that will fix #773. Essentially just porting the code over from the latest upstream version of Geth (with a few tweaks to make the types match up).

Metadata
Fixes #773

changeset-bot · 2021-05-05T23:43:13Z

🦋 Changeset detected

Latest commit: 9cd7685

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@eth-optimism/integration-tests	Patch
@eth-optimism/l2geth	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codecov-commenter · 2021-05-06T00:33:13Z

Codecov Report

Merging #774 (1be5ef5) into v0.3.0-rc (812b5ed) will increase coverage by 0.25%.
The diff coverage is n/a.

❗ Current head 1be5ef5 differs from pull request most recent head 611cc2d. Consider uploading reports for the commit 611cc2d to get more accurate results

@@              Coverage Diff              @@
##           v0.3.0-rc     #774      +/-   ##
=============================================
+ Coverage      79.02%   79.27%   +0.25%     
=============================================
  Files             48       48              
  Lines           1878     1872       -6     
  Branches         297      297              
=============================================
  Hits            1484     1484              
+ Misses           394      388       -6

Impacted Files	Coverage Δ
...imistic-ethereum/libraries/trie/Lib_MerkleTrie.sol	`74.88% <ø> (+1.01%)`	⬆️
...c-ethereum/libraries/trie/Lib_SecureMerkleTrie.sol	`78.57% <ø> (+13.86%)`	⬆️

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 812b5ed...611cc2d. Read the comment docs.

transmissions11 · 2021-05-06T01:24:52Z

Just built and tried out on my machine! Confirmed working woohoo! 🎉

smartcontracts · 2021-05-06T01:25:05Z

@tynes @karlfloersch looks like this PR is working. Which branch should we try to merge into?

tynes · 2021-05-06T01:53:53Z

Could you squash this into a couple of commits and then add changesets for l2geth and integration-tests? Then you can base it off off master

smartcontracts · 2021-05-06T04:09:04Z

Unfortunately this fundamentally relies on some features that we added in v0.3.0. I could alternatively make this PR against v0.4.0.

gakonst

Great work. Small changes requested. I am OK with merging this against v0.3.0 AS LONG AS we merge 0.3.0 to master soon.

gakonst · 2021-05-06T07:08:17Z

l2geth/accounts/abi/abi.go

+	if err != nil {
+		return "", err
+	}
+	return unpacked[0].(string), nil


Could this ever panic, e.g. when len(unpacked) == 0? Might be worth handling that case. Do we know that the cast will always work?

I believe this will never panic. Unpacking errors should be caught above and because of the RevertSelector type we should be guaranteed to have a length=1 array.

gakonst · 2021-05-06T07:09:10Z

l2geth/internal/ethapi/api.go

+		ok, res := executable(hi)
+		if !ok {
+			if len(res) >= 4 && bytes.Equal(res[:4], abi.RevertSelector) {
+				reason, errUnpack := abi.UnpackRevert(res)
+				err := errors.New("execution reverted")
+				if errUnpack == nil {
+					err = fmt.Errorf("execution reverted: %v", reason)
+				}
+				return 0, err
+			}


l2geth/accounts/abi/abi.go

gakonst · 2021-05-06T07:30:56Z

l2geth/accounts/abi/abi.go

+	if !bytes.Equal(data[:4], RevertSelector) {
+		return "", errors.New("invalid data for unpacking")
+	}
+	typ, _ := NewType("string", "", nil)


Worth instantiating this once globally as a var similarly to how you did with RevertSelector above. Also may want to add a note on why the error is not handled, or handle it explicitly

gakonst · 2021-05-06T07:32:00Z

l2geth/accounts/abi/abi.go

+// `Error(string)`. So it's a special tool for it.
+func UnpackRevert(data []byte) (string, error) {
+	if len(data) < 4 {
+		return "", errors.New("invalid data for unpacking")


Suggested change

return "", errors.New("invalid data for unpacking")

return "", errors.New("revert data length less than 4 bytes")

Same comment as below, this is a copy/paste from: https://github.com/ethereum/go-ethereum/blob/cc606be74c6f1f05b0b0a6226a400e734b9aac31/accounts/abi/abi.go#L259-L279.

gakonst · 2021-05-06T07:32:13Z

l2geth/accounts/abi/abi.go

+		return "", errors.New("invalid data for unpacking")
+	}
+	if !bytes.Equal(data[:4], RevertSelector) {
+		return "", errors.New("invalid data for unpacking")


Suggested change

return "", errors.New("invalid data for unpacking")

return "", errors.New("revert data did not match revert function selector")

This (and most of the changes here) are just a copy/paste from https://github.com/ethereum/go-ethereum/blob/cc606be74c6f1f05b0b0a6226a400e734b9aac31/accounts/abi/abi.go#L259-L279.

gakonst · 2021-05-06T07:33:08Z

l2geth/internal/ethapi/api.go

-		if !executable(hi) {
+		ok, res := executable(hi)
+		if !ok {
+			if len(res) >= 4 && bytes.Equal(res[:4], abi.RevertSelector) {


Is this condition needed? Both of these checks are made inside UnpackRevert.

Geth essentially does this check, I just copied that logic over: https://github.com/ethereum/go-ethereum/blob/cc606be74c6f1f05b0b0a6226a400e734b9aac31/internal/ethapi/api.go#L1054-L1061

integration-tests/test/erc20.spec.ts

gakonst

LGTM - thanks for the explanations.

tynes

Can you squash into 2 commits - one for l2geth, one for integration tests and then add a changeset for a total of 3 commits?

fix: error in comment fix: I got things backwards fix: Use UnpackValues instead of Unpack Update l2geth/accounts/abi/abi.go Co-authored-by: Georgios Konstantopoulos <[email protected]>

fix: build error

smartcontracts · 2021-05-07T22:27:16Z

@tynes done.

* feat: Attempt to decode txs as RLP first (#563) Co-authored-by: smartcontracts <[email protected]> * l2geth: remove eth_sendRawEthSignTransaction endpoint (#589) * feat[contracts]: Use standard RLP transaction format (#566) * feat[contracts]: Use standard RLP transaction format * fix[l2geth]: Encode transaction as RLP * fix: Correct gas estimation in integration tests * fix: Correct gas estimation in integration tests * Update packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_SequencerEntrypoint.sol Co-authored-by: ben-chain <[email protected]> * fix[contracts]: Use isCreate instead of checking target address * fix[contracts]: Minor optimization in SequencerEntrypoint * fix[contracts]: Pass max gas to contract call in EOA contract Co-authored-by: ben-chain <[email protected]> * feat[contracts]: Make ProxyEOA compatible with eip1967 (#592) * feat[contracts]: Make ProxyEOA compatible with eip1967 * fix[contracts]: Fix bug introduced by indirect constant * chore[contracts]: Add changeset * Update .changeset/old-cycles-invite.md Co-authored-by: Georgios Konstantopoulos <[email protected]> * l2geth: remove ovmsigner (#591) * l2geth: remove ovmsigner Also reduce the diff Co-authored-by: smartcontracts * l2geth: add changeset * l2geth: set rlp encoded tx in txmeta in RPC layer (#644) * l2geth: set rlp encoded tx in txmeta in RPC layer * l2geth: remove extra setter of txmeta * chore: add changeset * feat: Have ExecutionManager pass data upwards (#643) * feat[contracts]: Make ExecutionManager return data * fix[l2geth]: fix linting error * fix[contracts]: Fix build error * fix[contracts]: fix failing unit tests * Add changeset Co-authored-by: Karl Floersch <[email protected]> * rpc: only allow txs with no calldata when there is value (#645) * l2geth: api checks for 0 value * chore: add changeset * l2geth: remove check for specific gasprice * feat[contracts]: Add value transfer support to ECDSAContractAccount (#619) * feat[contracts]: Use standard RLP transaction format (#566) * feat[contracts]: Use standard RLP transaction format * fix[l2geth]: Encode transaction as RLP * fix: Correct gas estimation in integration tests * fix: Correct gas estimation in integration tests * Update packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_SequencerEntrypoint.sol Co-authored-by: ben-chain <[email protected]> * fix[contracts]: Use isCreate instead of checking target address * fix[contracts]: Minor optimization in SequencerEntrypoint * fix[contracts]: Pass max gas to contract call in EOA contract Co-authored-by: ben-chain <[email protected]> * feat[contracts]: Add value transfer to contract account * fix[contracts]: Tweak transfer logic and add tests * fix[geth]: Remove logic that rejects value gt 0 txs * fix: nonce issue in rpc tests * fix: use correct wallet in rpc value tests * Update rpc.spec.ts * cleanup: remove double definition * chore: add changeset * chore: add changeset * tests: delete dead test * l2geth: log the tx value * l2geth: pass through zero value at top level * test: receipt passes * test: more specifically set balance Co-authored-by: ben-chain <[email protected]> Co-authored-by: Mark Tyneway <[email protected]> * dtl: remove legacy encoding (#618) * dtl: remove legacy decoding * tests: remove dead test * chore: add changeset * Add Goerli v3 deployment (#651) * Add Goerli v3 deployment * Add Goerli v3 to README * dtlL fix syncing off by one (#687) * dtl: syncing off by one error * chore: add changeset * dtl: index the value field (#686) * chore: add changeset * chore: add changeset * dtl: pass through value field * core-utils: update and test toRpcString * lint: fix * l2geth: parse value fields * chore: add changeset * rpc: gas fixes (#695) * l2geth: prevent fees lower than 21000 * l2geth: remove old check for too high tx gaslimit * tests: update to use new min gas estimated value * chore: add changeset * test: update expected values * test: remove dead test * examples: fix waffle example + gas changes in tests (#724) * examples: fix waffle example * tests: update gas price in assertion * chore: add changeset * l2geth: estimate gas assertion in decimal * test: use configurable key * ops: delete extra whitespace (#731) * fix: prevent eth sendtransaction (#725) * api: prevent unsafe calls * api: fill in txmeta * chore: add changeset * chore: add changeset * l2geth + contracts: standard interface for systems contracts and userland contracts (#721) * l2geth: fix call returndata parsing * contracts: standardize simulateMessage and run to return bytes * chore: add changeset * chore: add changeset * l2geth: more simple decoding * contracts: remove named arguments * chore: fix linter errors * Add contract deployment to Kovan (#715) * fix: remove type check in rollup client (#750) * l2geth: remove tx type check in client * chore: add changeset * dtl: prevent null reference in L1 handler (#757) * dtl: prevent reference of null value * chore: add changeset * test: eth_call exceptions (#800) * feat[l2geth]: Pass up contract revert reasons during DoEstimateGas (#774) * wip: Starting work on geth revert reasons during estimate gas fix: error in comment fix: I got things backwards fix: Use UnpackValues instead of Unpack Update l2geth/accounts/abi/abi.go Co-authored-by: Georgios Konstantopoulos <[email protected]> * Add integration test for reverts fix: build error * chore: Add changeset Co-authored-by: Georgios Konstantopoulos <[email protected]> * chore: add changeset (#831) * Migrate ETH between gateways (#778) * add migrate ETH functionality * contracts: add eth gateway docstring (#832) Co-authored-by: Mark Tyneway <[email protected]> Co-authored-by: smartcontracts <[email protected]> Co-authored-by: Mark Tyneway <[email protected]> Co-authored-by: smartcontracts <[email protected]> Co-authored-by: ben-chain <[email protected]> Co-authored-by: Georgios Konstantopoulos <[email protected]> Co-authored-by: Maurelian <[email protected]> Co-authored-by: Kevin Ho <[email protected]>

* feat: Attempt to decode txs as RLP first (#563) Co-authored-by: smartcontracts <[email protected]> * l2geth: remove eth_sendRawEthSignTransaction endpoint (#589) * feat[contracts]: Use standard RLP transaction format (#566) * feat[contracts]: Use standard RLP transaction format * fix[l2geth]: Encode transaction as RLP * fix: Correct gas estimation in integration tests * fix: Correct gas estimation in integration tests * Update packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_SequencerEntrypoint.sol Co-authored-by: ben-chain <[email protected]> * fix[contracts]: Use isCreate instead of checking target address * fix[contracts]: Minor optimization in SequencerEntrypoint * fix[contracts]: Pass max gas to contract call in EOA contract Co-authored-by: ben-chain <[email protected]> * feat[contracts]: Make ProxyEOA compatible with eip1967 (#592) * feat[contracts]: Make ProxyEOA compatible with eip1967 * fix[contracts]: Fix bug introduced by indirect constant * chore[contracts]: Add changeset * Update .changeset/old-cycles-invite.md Co-authored-by: Georgios Konstantopoulos <[email protected]> * l2geth: remove ovmsigner (#591) * l2geth: remove ovmsigner Also reduce the diff Co-authored-by: smartcontracts * l2geth: add changeset * l2geth: set rlp encoded tx in txmeta in RPC layer (ethereum-optimism#644) * l2geth: set rlp encoded tx in txmeta in RPC layer * l2geth: remove extra setter of txmeta * chore: add changeset * feat: Have ExecutionManager pass data upwards (ethereum-optimism#643) * feat[contracts]: Make ExecutionManager return data * fix[l2geth]: fix linting error * fix[contracts]: Fix build error * fix[contracts]: fix failing unit tests * Add changeset Co-authored-by: Karl Floersch <[email protected]> * rpc: only allow txs with no calldata when there is value (ethereum-optimism#645) * l2geth: api checks for 0 value * chore: add changeset * l2geth: remove check for specific gasprice * feat[contracts]: Add value transfer support to ECDSAContractAccount (ethereum-optimism#619) * feat[contracts]: Use standard RLP transaction format (#566) * feat[contracts]: Use standard RLP transaction format * fix[l2geth]: Encode transaction as RLP * fix: Correct gas estimation in integration tests * fix: Correct gas estimation in integration tests * Update packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_SequencerEntrypoint.sol Co-authored-by: ben-chain <[email protected]> * fix[contracts]: Use isCreate instead of checking target address * fix[contracts]: Minor optimization in SequencerEntrypoint * fix[contracts]: Pass max gas to contract call in EOA contract Co-authored-by: ben-chain <[email protected]> * feat[contracts]: Add value transfer to contract account * fix[contracts]: Tweak transfer logic and add tests * fix[geth]: Remove logic that rejects value gt 0 txs * fix: nonce issue in rpc tests * fix: use correct wallet in rpc value tests * Update rpc.spec.ts * cleanup: remove double definition * chore: add changeset * chore: add changeset * tests: delete dead test * l2geth: log the tx value * l2geth: pass through zero value at top level * test: receipt passes * test: more specifically set balance Co-authored-by: ben-chain <[email protected]> Co-authored-by: Mark Tyneway <[email protected]> * dtl: remove legacy encoding (ethereum-optimism#618) * dtl: remove legacy decoding * tests: remove dead test * chore: add changeset * Add Goerli v3 deployment (ethereum-optimism#651) * Add Goerli v3 deployment * Add Goerli v3 to README * dtlL fix syncing off by one (ethereum-optimism#687) * dtl: syncing off by one error * chore: add changeset * dtl: index the value field (ethereum-optimism#686) * chore: add changeset * chore: add changeset * dtl: pass through value field * core-utils: update and test toRpcString * lint: fix * l2geth: parse value fields * chore: add changeset * rpc: gas fixes (ethereum-optimism#695) * l2geth: prevent fees lower than 21000 * l2geth: remove old check for too high tx gaslimit * tests: update to use new min gas estimated value * chore: add changeset * test: update expected values * test: remove dead test * examples: fix waffle example + gas changes in tests (ethereum-optimism#724) * examples: fix waffle example * tests: update gas price in assertion * chore: add changeset * l2geth: estimate gas assertion in decimal * test: use configurable key * ops: delete extra whitespace (ethereum-optimism#731) * fix: prevent eth sendtransaction (ethereum-optimism#725) * api: prevent unsafe calls * api: fill in txmeta * chore: add changeset * chore: add changeset * l2geth + contracts: standard interface for systems contracts and userland contracts (ethereum-optimism#721) * l2geth: fix call returndata parsing * contracts: standardize simulateMessage and run to return bytes * chore: add changeset * chore: add changeset * l2geth: more simple decoding * contracts: remove named arguments * chore: fix linter errors * Add contract deployment to Kovan (ethereum-optimism#715) * fix: remove type check in rollup client (ethereum-optimism#750) * l2geth: remove tx type check in client * chore: add changeset * dtl: prevent null reference in L1 handler (ethereum-optimism#757) * dtl: prevent reference of null value * chore: add changeset * test: eth_call exceptions (ethereum-optimism#800) * feat[l2geth]: Pass up contract revert reasons during DoEstimateGas (ethereum-optimism#774) * wip: Starting work on geth revert reasons during estimate gas fix: error in comment fix: I got things backwards fix: Use UnpackValues instead of Unpack Update l2geth/accounts/abi/abi.go Co-authored-by: Georgios Konstantopoulos <[email protected]> * Add integration test for reverts fix: build error * chore: Add changeset Co-authored-by: Georgios Konstantopoulos <[email protected]> * chore: add changeset (ethereum-optimism#831) * Migrate ETH between gateways (ethereum-optimism#778) * add migrate ETH functionality * contracts: add eth gateway docstring (ethereum-optimism#832) Co-authored-by: Mark Tyneway <[email protected]> Co-authored-by: smartcontracts <[email protected]> Co-authored-by: Mark Tyneway <[email protected]> Co-authored-by: smartcontracts <[email protected]> Co-authored-by: ben-chain <[email protected]> Co-authored-by: Georgios Konstantopoulos <[email protected]> Co-authored-by: Maurelian <[email protected]> Co-authored-by: Kevin Ho <[email protected]>

smartcontracts changed the base branch from master to v0.3.0-rc May 6, 2021 00:22

smartcontracts force-pushed the feat/geth-revert-reasons branch from 1be5ef5 to 611cc2d Compare May 6, 2021 00:31

smartcontracts added A-geth C-feature Category: features labels May 6, 2021

smartcontracts marked this pull request as ready for review May 6, 2021 01:24

smartcontracts requested review from karlfloersch and tynes as code owners May 6, 2021 01:24

gakonst suggested changes May 6, 2021

View reviewed changes

gakonst approved these changes May 7, 2021

View reviewed changes

tynes requested changes May 7, 2021

View reviewed changes

smartcontracts and others added 3 commits May 7, 2021 18:26

wip: Starting work on geth revert reasons during estimate gas

84156b1

fix: error in comment fix: I got things backwards fix: Use UnpackValues instead of Unpack Update l2geth/accounts/abi/abi.go Co-authored-by: Georgios Konstantopoulos <[email protected]>

Add integration test for reverts

bb4ebc7

fix: build error

chore: Add changeset

9cd7685

smartcontracts force-pushed the feat/geth-revert-reasons branch from a32ea31 to 9cd7685 Compare May 7, 2021 22:26

tynes approved these changes May 7, 2021

View reviewed changes

tynes merged commit d0e4250 into v0.3.0-rc May 7, 2021

tynes deleted the feat/geth-revert-reasons branch May 7, 2021 23:35

smartcontracts mentioned this pull request May 8, 2021

Update Geth to produce revert reasoning instead of UNPREDICTABLE_GAS_LIMIT #773

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat[l2geth]: Pass up contract revert reasons during DoEstimateGas #774

feat[l2geth]: Pass up contract revert reasons during DoEstimateGas #774

smartcontracts commented May 5, 2021 •

edited

Loading

changeset-bot bot commented May 5, 2021 •

edited

Loading

codecov-commenter commented May 6, 2021 •

edited

Loading

transmissions11 commented May 6, 2021 •

edited

Loading

smartcontracts commented May 6, 2021

tynes commented May 6, 2021

smartcontracts commented May 6, 2021

gakonst left a comment •

edited

Loading

gakonst May 6, 2021

smartcontracts May 6, 2021

gakonst May 6, 2021

gakonst May 6, 2021

gakonst May 6, 2021

smartcontracts May 6, 2021

gakonst May 6, 2021

smartcontracts May 6, 2021

gakonst May 6, 2021

smartcontracts May 6, 2021

gakonst left a comment •

edited

Loading

tynes left a comment

smartcontracts commented May 7, 2021

	return "", errors.New("invalid data for unpacking")
	return "", errors.New("revert data length less than 4 bytes")

	return "", errors.New("invalid data for unpacking")
	return "", errors.New("revert data did not match revert function selector")

feat[l2geth]: Pass up contract revert reasons during DoEstimateGas #774

feat[l2geth]: Pass up contract revert reasons during DoEstimateGas #774

Conversation

smartcontracts commented May 5, 2021 • edited Loading

changeset-bot bot commented May 5, 2021 • edited Loading

🦋 Changeset detected

codecov-commenter commented May 6, 2021 • edited Loading

Codecov Report

transmissions11 commented May 6, 2021 • edited Loading

smartcontracts commented May 6, 2021

tynes commented May 6, 2021

smartcontracts commented May 6, 2021

gakonst left a comment • 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

gakonst left a comment • edited Loading

Choose a reason for hiding this comment

tynes left a comment

Choose a reason for hiding this comment

smartcontracts commented May 7, 2021

smartcontracts commented May 5, 2021 •

edited

Loading

changeset-bot bot commented May 5, 2021 •

edited

Loading

codecov-commenter commented May 6, 2021 •

edited

Loading

transmissions11 commented May 6, 2021 •

edited

Loading

gakonst left a comment •

edited

Loading

gakonst left a comment •

edited

Loading