Integrated contracts-v2 #67

smartcontracts · 2020-10-29T22:16:26Z

Description

Lots of changes, but the primary ones are:

Changing how we parse transactions to convert them to "ovm messages"
Updating state_manager.go with the new API
Updating our logging statements
Updating our state dump (I'd rather we pull this from somewhere external)

Contributing Agreement

I have read and understood the Optimism Contributing Guide and Code of Conduct and am following those guidelines in this pull request.

smartcontracts · 2020-10-29T22:17:46Z

core/state_transition.go

+			return nil, 0, false, err
+		}
+
+		evm.Context.Origin = msg.From()


Hopefully this will be made unnecessary by a change @K-Ho is making to the OVM_ExecutionManager. Basically needed so I can get msg.sender == ovmStateManager.owner() to return true.

smartcontracts · 2020-10-29T22:18:03Z

core/state_transition.go

+		}
+	} else {
+		// OVM_ENABLED
+		st.buyGas()


This is the only part of the preCheck that we actually care about.

Why is it safe to skip the nonce check?

It wasn't :-( I added the nonce check again.

smartcontracts · 2020-10-29T22:18:23Z

core/state_transition.go

 	if contractCreation {
-		// Here we are going to call the EM directly


All of this is now handled in toExecutionManagerRun, so not needed here anymore.

awesome refactor

smartcontracts · 2020-10-29T22:19:04Z

core/state_transition.go

-
-		// If the tx fails we won't have incremented the nonce. In this case, increment it manually
-		log.Debug("Incrementing nonce due to transaction failure")
-		st.state.SetNonce(msg.From(), st.state.GetNonce(sender.Address())+1)


We need to figure out the best way to handle this nonce eviction thing. @karlfloersch should we just go ahead and update the eviction logic?

Once we've stabilized Synthetix running in Contracts v2, and have deployed Mintr to this Geth & have it work, that seems to me to be the next highest priority

smartcontracts · 2020-10-29T22:19:14Z

core/state_transition.go

-	log.Debug("return data", "data", hexutil.Encode(ret))
+	if !vm.UsingOVM {
+		// OVM_DISABLED
+		st.state.AddBalance(st.evm.Coinbase, new(big.Int).Mul(new(big.Int).SetUint64(st.gasUsed()), st.gasPrice))


No balances in the OVM.

smartcontracts · 2020-10-29T22:22:37Z

core/vm/evm.go

+		}
+
+		if caller.Address() == OvmExecutionManager.Address &&
+			!strings.HasPrefix(strings.ToLower(addr.Hex()), "0xdeaddeaddeaddeaddeaddeaddeaddeaddead") &&


0xdead and 0x0000 addresses can't be the target.

Why? What do these mean? Could we use a variable instead of hardcoded addrs?

0xdead addresses are used for all of the OVM contracts that run in L2 (so the execution manager, state manager, etc.) Users aren't allowed to call these addresses via ovmCALLs & similar opcodes. We could probably use a variable but we need to coordinate this variable across everything.

smartcontracts · 2020-10-29T22:22:54Z

core/vm/evm.go

+	if !UsingOVM {
+		// OVM_DISABLED
+		// Fail if we're trying to transfer more than the available balance
+		if !evm.Context.CanTransfer(evm.StateDB, caller.Address(), value) {


Breaks things in the OVM (don't need it anyway).

smartcontracts · 2020-10-29T22:23:29Z

core/vm/evm.go

+	if UsingOVM {
+		// OVM_ENABLED
+		if isTarget {
+			evm.OriginalTargetResult = ret[96:]


Ignore the first 96 bytes (the return status and ABI encoding parameters).

Is there any chance len(ret) < 96?

Yes -- fixed to address this.

smartcontracts · 2020-10-29T22:24:12Z

core/vm/interpreter.go

@@ -124,6 +124,8 @@ func NewEVMInterpreter(evm *EVM, cfg Config) *EVMInterpreter {
 	}
 }

+var returnDataCopy string


This is the hack that bypasses an apparent bug

Curious, what was the bug - also why do we need to do this manually? Isin.returnData not always set correctly?

This is a pretty crazy bug 😮

Has to do with internal Geth memory management... we gotta look into it further

@gakonst unclear what the bug is. It appears that some combination of RETURNDATACOPY and other opcodes results in MSTORE overwriting in.returnData. Very strange.

smartcontracts · 2020-10-29T22:25:02Z

internal/ethapi/api.go

@@ -806,7 +806,7 @@ func DoCall(ctx context.Context, b Backend, args CallArgs, blockNrOrHash rpc.Blo
 		}
 	}
 	// Set default gas & gas price if none were set
-	gas := uint64(math.MaxUint64 / 2)
+	gas := uint64(10000000)


Will break with max gas because EM asserts that gas is < maxGasPerQueuePerEpoch (not sure what the right value is to put here -- block gas limit?)

I'm using ExecutionManager.GetMaxTransactionGasLimit

tynes · 2020-10-30T14:39:42Z

core/state_transition_ovm.go

+var ZeroAddress = common.HexToAddress("0x0000000000000000000000000000000000000000")
+
+type ovmTransaction struct {
+	Timestamp     *big.Int       "json:\"timestamp\""


Why the escaped "?

Took this from another part of geth where it used the escaped ".

tynes · 2020-10-30T14:47:59Z

core/state_transition_ovm.go

+	}
+
+	// Signature type
+	data.WriteByte(byte(sigtype)) // 1 byte: 00 == EOACreate, 01 == EIP 155, 02 == ETH Sign Message


I've played with a few different styles of serialization, using bytes.Buffer, io.Writer and []byte. Curious if you have any preferences.

My favorite so far has been using []byte directly, see

go-ethereum/rollup/types.go

Line 382 in b2e6790

func (c *CTCTxEIP155) Encode(b []byte) error {

tynes · 2020-10-30T14:51:40Z

core/state_transition_ovm.go

+	}
+}
+
+func getQueueOrigin(


I'm sorry about this

tynes · 2020-10-30T14:52:45Z

core/vm/evm.go

@@ -69,6 +88,7 @@ func run(evm *EVM, contract *Contract, input []byte, readOnly bool) ([]byte, err
 			return RunPrecompiledContract(p, input, contract)
 		}
 	}
+


Maybe no need to add extra whitelines for minimal diff?

tynes · 2020-10-30T14:57:20Z

core/vm/ovm_state_dump.go

+
+	OvmStateManager = OvmStateDump.Accounts["OVM_StateManager"]
+	OvmExecutionManager = OvmStateDump.Accounts["OVM_ExecutionManager"]
+	UsingOVM = os.Getenv("USING_OVM") == "true"


Noted that this will be added to deploy config

karlfloersch

Got part of the way through & will finish shortly but so far it's looking great!

karlfloersch · 2020-11-02T03:14:09Z

core/state_transition.go

-
-		// If the tx fails we won't have incremented the nonce. In this case, increment it manually
-		log.Debug("Incrementing nonce due to transaction failure")
-		st.state.SetNonce(msg.From(), st.state.GetNonce(sender.Address())+1)


Once we've stabilized Synthetix running in Contracts v2, and have deployed Mintr to this Geth & have it work, that seems to me to be the next highest priority

karlfloersch · 2020-11-02T03:17:26Z

core/types/transaction.go

@@ -326,6 +326,13 @@ func (tx *Transaction) AsMessage(s Signer) (Message, error) {

 	var err error
 	msg.from, err = Sender(s, tx)
+
+	if tx.meta.L1MessageSender != nil {


I'm not entirely sure but I think it looks about right tbh.

Also, right here you should add:

if tx.meta.lastL1BlockNumber != nil { msg.l1BlockNumber = tx.meta.lastL1BlockNumber }

Oh though I forgot -- we have to rename txId here - https://github.com/ethereum-optimism/go-ethereum/pull/67/files#diff-5500355cc4409489bee920d34f30a486e081409325cc12793ea7895fb64ac7daR605 to lastL1BlockNumber

karlfloersch · 2020-11-02T03:20:05Z

core/state_transition_ovm.go

+func toExecutionManagerRun(evm *vm.EVM, msg Message) (Message, error) {
+	tx := ovmTransaction{
+		evm.Context.Time,
+		evm.Context.BlockNumber, // TODO (what's the correct block number?)


For an L1 to L2 transaction, is block.number supposed to be the L1 block number?

Yep

@karlfloersch @tynes what's the correct value for this?

Ah so we've got to set it in the same way you set the L1MessageSender from the meta. I wrote up how that works below

gakonst · 2020-11-02T14:04:35Z

core/state_transition_ovm.go

+	}
+
+	v, r, s := tx.RawSignatureValues()
+	v = big.NewInt(int64(v.Uint64() - 35 - 2*420))


Suggested change

v = big.NewInt(int64(v.Uint64() - 35 - 2*420))

v = big.NewInt(int64(v.Uint64() - 35 - 2* CHAIN_ID))

Could refactor this out to a CHAIN_ID constant, maybe also add a comment that this is EIP-155?

core/state_transition.go

gakonst · 2020-11-02T14:16:54Z

core/state_transition.go

+		}
+	} else {
+		// OVM_ENABLED
+		st.buyGas()


Why is it safe to skip the nonce check?

gakonst · 2020-11-02T15:00:51Z

core/state_transition.go

 	if contractCreation {
-		// Here we are going to call the EM directly


awesome refactor

gakonst · 2020-11-02T15:02:48Z

core/vm/evm.go

+		var isUnknown = true
+		for name, account := range OvmStateDump.Accounts {
+			if contract.Address() == account.Address {
+				isUnknown = false
+				abi := &(account.ABI)
+				method, err := abi.MethodById(input)
+				if err != nil {
+					log.Debug("Calling Known Contract", "Name", name, "ERROR", err)
+				} else {
+					log.Debug("Calling Known Contract", "Name", name, "Method", method.RawName)
+				}
+			}
+		}


OvmStateDump.Accounts is already a map, can't you just do account, ok := OvmStateDumpAccounts[name]?

gakonst · 2020-11-02T15:52:10Z

core/vm/evm.go

+		// ovmADDRESS will be set by the execution manager to the target address whenever it's
+		// about to create a new contract. This value is currently stored at the [15] storage slot.
+		// Can pull this specific storage slot to get the address that the execution manager is
+		// trying to create to, and create to it.
+		slot := common.HexToHash(strconv.FormatInt(15, 16))
+		contractAddr = common.BytesToAddress(evm.StateDB.GetState(OvmExecutionManager.Address, slot).Bytes())


Is this referring to this? This is maybe a footgun in case the layout of the ExecutionManager changes, maybe consider adding a big big warning to this and the contracts?

Also, consider refactoring the else branch to a helper function?

gakonst · 2020-11-02T15:52:41Z

core/vm/interpreter.go

@@ -124,6 +124,8 @@ func NewEVMInterpreter(evm *EVM, cfg Config) *EVMInterpreter {
 	}
 }

+var returnDataCopy string


Curious, what was the bug - also why do we need to do this manually? Isin.returnData not always set correctly?

gakonst · 2020-11-02T15:56:04Z

internal/ethapi/api.go

@@ -859,7 +863,10 @@ func DoCall(ctx context.Context, b Backend, args CallArgs, blockNrOrHash rpc.Blo
 	// Setup the gas pool (also for unmetered requests)
 	// and apply the message.
 	gp := new(core.GasPool).AddGas(math.MaxUint64)
-	res, gas, failed, err := core.ApplyMessage(evm, msg, gp)
+	log.Debug(">>>>>> Serving an eth_call <<<<<<", "From", addr.Hex(), "To", args.To.Hex())


Suggested change

log.Debug(">>>>>> Serving an eth_call <<<<<<", "From", addr.Hex(), "To", args.To.Hex())

Definitely should remove this logline, there will be tons of these

gakonst · 2020-11-02T15:56:16Z

internal/ethapi/api.go

+	log.Debug(">>>>>> Serving an eth_call <<<<<<", "From", addr.Hex(), "To", args.To.Hex())
+	evm.Context.EthCallSender = &addr
+	res, gas, failed, err := core.ApplyMessage(evm, outmsg, gp)
+	log.Debug("<<<<<< Served an eth_call  >>>>>>", "From", addr.Hex(), "To", args.To.Hex())


Suggested change

log.Debug("<<<<<< Served an eth_call >>>>>>", "From", addr.Hex(), "To", args.To.Hex())

(just a reminder to remove once done debugging)

gakonst · 2020-11-02T15:59:00Z

core/vm/evm.go

@@ -266,6 +356,65 @@ func (evm *EVM) Call(caller ContractRef, addr common.Address, input []byte, gas
 			contract.UseGas(contract.Gas)
 		}
 	}
+
+	if UsingOVM {


What is the rationale behind the whole isTarget/OriginalTarget logic? What can go wrong here?

…-ethereum into dev/contracts-v2-v2

…o dev/contracts-v2-v2

tynes · 2020-11-06T03:35:27Z

core/state_processor.go

+
+	if vm.UsingOVM {
+		// OVM_ENABLED
+		log.Debug("<<<<<< Served an OVM transaction  >>>>>>")


I don't know if we need these loglines anymore, but it probably isn't a bad idea to log something along the lines of ovm enabled once at startup if the env var that turns on ovm is set

tynes · 2020-11-06T03:37:16Z

core/state_processor.go

 	if err != nil {
 		return nil, err
 	}
+


I don't think we need to add extra newlines as it just creates a bigger diff from upstream. Go code generally has less newlines between blocks of code compared to JS based on reading a bunch of Go codebases 🤷

tynes · 2020-11-06T03:40:08Z

core/state_transition_ovm.go

+
+func getQueueOrigin(
+	queueOrigin *big.Int,
+) (types.QueueOrigin, error) {


I'm confused about this because there should only be 2 queue origins?

tynes · 2020-11-06T17:10:28Z

ethereum-optimism/optimism#358 must be merged at the same time

tynes · 2020-11-06T17:49:57Z

core/state_transition_ovm.go

+
+	if len(address) == 0 {
+		log.Warn("No TX_INGESTION_SIGNER_ADDRESS supplied. Using ZERO_ADDRESS default.")
+		address = "0000000000000000000000000000000000000000"


This always breaks in this code path because address len is 40

* Works, but need to figure out this bug * Remove unnecessary log statements * Finalizing integration * Small code cleanups * Added a few more comments * Various bugfixes * Works, but need to figure out this bug * Remove unnecessary log statements * Finalizing integration * Small code cleanups * Added a few more comments * Various bugfixes * Added custom fillbytes function * Removed old test file for now * Fix linting errors * Fix linting errors * Reduce gas limit again * Various fixes! * Minor updates to get l1 ingestion address * Fix remaining bugs * Fix lint errors * Fix broken tests. WARN I skipped some * loglines: remove before deployment * core: less diff by removing newlines * core: less diff by removing newlines * rollup: remove logline in signtx * core/ovm: handle errors when type casting * ovm: log applying message Co-authored-by: Karl Floersch <karl@karlfloersch.com> Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>

smartcontracts added 2 commits October 29, 2020 13:14

Works, but need to figure out this bug

e3ca5a8

Remove unnecessary log statements

4f90f0e

smartcontracts commented Oct 29, 2020

View reviewed changes

tynes reviewed Oct 30, 2020

View reviewed changes

core/state_transition_ovm.go

}

}

func getQueueOrigin(

Copy link

Collaborator

tynes Oct 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry about this

tynes reviewed Oct 30, 2020

View reviewed changes

smartcontracts added 2 commits October 30, 2020 19:45

Finalizing integration

f6482de

Small code cleanups

dd0caa0

karlfloersch reviewed Nov 2, 2020

View reviewed changes

gakonst reviewed Nov 2, 2020

View reviewed changes

smartcontracts and others added 18 commits November 2, 2020 12:23

Added a few more comments

dd3d4a3

Various bugfixes

cfbb0c0

Works, but need to figure out this bug

c467c4a

Remove unnecessary log statements

fede6c8

Finalizing integration

16dad48

Small code cleanups

bc8af0d

Added a few more comments

f25f336

Various bugfixes

a8ed700

Merge branch 'dev/contracts-v2-v2' of github.com:ethereum-optimism/go…

8bd1147

…-ethereum into dev/contracts-v2-v2

Added custom fillbytes function

f1b39f5

Removed old test file for now

e9f95d3

Fix linting errors

56680fb

Fix linting errors

3a03061

Reduce gas limit again

c2f6f1e

Various fixes!

511c45c

Minor updates to get l1 ingestion address

41b4998

Merge branch 'master' of github.com:ethereum-optimism/go-ethereum int…

b5124c2

…o dev/contracts-v2-v2

Fix remaining bugs

d20c3a9

Fix lint errors

1969725

tynes reviewed Nov 6, 2020

View reviewed changes

Fix broken tests. WARN I skipped some

0ef864e

tynes added 4 commits November 6, 2020 09:40

loglines: remove before deployment

a495e0d

core: less diff by removing newlines

3004a0a

core: less diff by removing newlines

1ac2e87

rollup: remove logline in signtx

3b72f28

tynes reviewed Nov 6, 2020

View reviewed changes

tynes added 2 commits November 6, 2020 09:55

core/ovm: handle errors when type casting

9790e0f

ovm: log applying message

1bf63ca

tynes merged commit 93c5a6a into master Nov 6, 2020

tynes deleted the dev/contracts-v2-v2 branch November 6, 2020 21:06

tynes restored the dev/contracts-v2-v2 branch November 6, 2020 21:06

		if contractCreation {
		// Here we are going to call the EM directly

	v = big.NewInt(int64(v.Uint64() - 35 - 2*420))
	v = big.NewInt(int64(v.Uint64() - 35 - 2* CHAIN_ID))

Integrated contracts-v2 #67

Integrated contracts-v2 #67

Conversation

smartcontracts commented Oct 29, 2020

Description

Contributing Agreement

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlfloersch 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

gakonst Nov 2, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tynes commented Nov 6, 2020

Choose a reason for hiding this comment

gakonst Nov 2, 2020 •

edited

Loading