Limiting Geth to one transaction per block by willmeister · Pull Request #3 · ethereum-optimism/go-ethereum

willmeister · 2020-05-12T14:11:46Z

Limits Geth to a maximum of 1 tx per block.

masonforest

left a couple comments but LGTM 🚢

…o very high for docker build, formatting code

ben-chain

🔥🔥🔥This is awesome! Left a couple of notes but looks great. One thing I noticed is that we don't have the nice formatted comments above these functions explicitly specifying a description, arguments, and return vals like we have in typescript. Seems like geth generally doesn't have this--what do we think about doing that for our additions to the codebase?

Another general note is that it took me some time to be sure I understood what was going on between the different functions try..., build, add and so on. This was made worse by having a name collision/ambiguity with block for geth vs RollupBlock here. So a nit would be that we should consider calling the rollup side something other than a "block" for disambiguity.

ben-chain · 2020-05-14T16:25:10Z


-		ethNoFork, _  = NewProtocolManager(configNoFork, nil, downloader.FullSync, 1, new(event.TypeMux), new(testTxPool), engine, chainNoFork, dbNoFork, 1, nil)
-		ethProFork, _ = NewProtocolManager(configProFork, nil, downloader.FullSync, 1, new(event.TypeMux), new(testTxPool), engine, chainProFork, dbProFork, 1, nil)
+		blockSubmitterNoFork = rollup.NewBlockSubmitter()


Interesting test here -- looks like this is hard fork related and will not be applicable to our code. Assuming it's just to keep the tests passing that you had to go in here, can you confirm?

In any case, interesting to think if we'll need to replicate this type of thing for purposes of network upgrades!

Yeah, just following the pattern. I think the important part is to have 2 separate instances of everything. What they're used for matters a lot less to me.

ben-chain · 2020-05-14T16:37:26Z

+func GetBlockRollupGasUsage(block *types.Block) uint64 {
+	return params.SstoreSetGas + uint64(len(block.Transactions()[0].Data()))*params.TxDataNonZeroGasEIP2028
+}


👌🤘Nice! Two notes here:

Because L1->L2 transactions "have already been rolled up" and the sequencer just sorts them into the canonical chain, the TxDataNonZeroGasEIP2028 will not apply for L1->L2 transactions. Premature to do that before we know exactly how L1->L2 will be handled, but thought it was worth noting.

In the future we might also want to add some factor here for other gas cost like merklization of the block; it's probably much smaller than the calldata cost for any large number of transactions though. Again probably premature, until we have those numbers.

Good points! Each RollupBlock starts with a gas buffer already used to account for small stuff we miss, but anything that scales linearly should definitely be accounted for.

…pulling logic out of TransitionBatchBuilder

* Limiting Geth to one transaction per block * Adding TransitionBatchBuilder to build & submit rollup blocks

* Get basic getStorage/setStorage stubs working * Clean up tests * Add state_manager * Add StateManager set & getStorage * Add state mananger create function * Add get & increment nonce * Add getCodeContractBytecode * Add GetCodeContractHash * Add getCodeContractHash to the state manager * Add associateCodeContract to state manager * Pass the tests * go fmt * Add stateTransition to test with * Fix tests * Test deploying contract with transition state * Call executeTransaction on contract deployment * Added ExecutionManager deployment * Get contract deployments working * Cleanup logging * Get stubbed ExecutionManager working * Get a simple contract to deploy through the ExecutionManager * Refactor simpleAbiEncode * Revert unnecessary changes * Remove comments * Revert changes outside of this PR * Revert changes outside of this PR * Revert changes outside of this PR * Fix broken tests * Move OVM bytecode & ABI into constants * Add crazy printlines * Remove crazy comments * Add a bunch of debug printlns * Add helper fn for applying msgs to the EVM * Update ExecutionManager bytecode * Shim CREATE for EM to use correct addr * Add SimpleStorage test * Add the EM/SM to all new states * Force all txs to be routed through the EM * Remove unused files * Remove unused comments * Increment nonce after failed tx * Add debug statements * Use evm.Time for timestamp * Change EM deployment, fix broken tests, clean up * Add an OVM test & remove printlns * Fix lint errors & remove final printlns * Final cleanup--remove some comments * Limiting Geth to one transaction per block (#3) * Limiting Geth to one transaction per block * Adding TransitionBatchBuilder to build & submit rollup blocks * Adding L1MessageSender to Transaction (#4) * Adding L1MessageSender to Transaction * Adding logic to omit L1MessageSender in encoding / decoding when nil and never use it in hash computation Co-authored-by: ben-chain <ben@pseudonym.party> * Fixing Geth Tests (#6) Fixing broken tests, skipping tests we intentionally break, and configuring CI within Github Actions * Hex Trie -> Binary Trie (#7) *** Changing Hex Trie to Binary Trie *** Note: This changes and/or comments out a bunch of tests, so if things break down the line, this is likely the cause! * Ingest Block Batches (#8) Handling BlockBatches in Geth at `SendBlockBatches` endpoint (eth_sendBlockBatches) Other: * Adding PR template * Adding ability to set timestamp and making blocks use configured timestamp * Adding ability to encode original tx nonce in calldata * Adding L1MessageSender to Contract Creation Txs * Add L1MessageSender to Message * Increment nonce on CREATE failure * Fix bug where evm.Time=0 * Use state dump with hardcoded EM & SM addrs - ExecutionMgr address should always be 0x0000...dead0000 - StateMgr address should always be 0x0000...dead0001 * Move EM deployment into genesis block maker * Update EM contracts to latest version * Update EM to remove events * Fix the OVM tests * Skip an ungodly number of tests * Fix lint errors * Clean up logging * Cleanup more logs * Use local reference to state manager * Rename applyOvmToState(..) * Remove unneeded check * Clean up logging & add EM ABI panic * Add gas metering to SM & small refactor * Update core/vm/state_manager.go Co-authored-by: Kevin Ho <kevinjho1996@gmail.com> Co-authored-by: Mason Fischer <mason@kissr.co> Co-authored-by: Will Meister <william.k.meister@gmail.com> Co-authored-by: ben-chain <ben@pseudonym.party> Co-authored-by: Kevin Ho <kevinjho1996@gmail.com>

Limiting Geth to one transaction per block

bfc1a0b

masonforest approved these changes May 12, 2020

View reviewed changes

Comment thread Dockerfile Outdated

Comment thread docker/entrypoint.sh Outdated

Comment thread miner/worker.go

willmeister added 7 commits May 12, 2020 10:25

making target gas limit configurable and removing unnecessary dependency

87e6beb

first stab at rollup block builder

65054d4

adding logging and reducing the number of panics

e0595df

fixing deadlock, adding more debug statements, defaulting verbosity t…

60ff754

…o very high for docker build, formatting code

reverting change

e510eb7

Adding tests to rollup block builder and fixing bugs

a377355

Adding more unit tests, adding documentation for RollupBlockBuilder

61430c4

ben-chain approved these changes May 14, 2020

View reviewed changes

willmeister added 2 commits May 14, 2020 13:25

adding existing data tests, renaming RollupBlock to TransitionBatch, …

c641835

…pulling logic out of TransitionBatchBuilder

formatting

e6b9381

willmeister merged commit e14cfa5 into master May 14, 2020

willmeister deleted the YAS-354/L1BatchGenerator branch May 14, 2020 19:45

karlfloersch pushed a commit that referenced this pull request Jul 16, 2020

Limiting Geth to one transaction per block (#3)

281115c

* Limiting Geth to one transaction per block * Adding TransitionBatchBuilder to build & submit rollup blocks

ben-chain mentioned this pull request Dec 4, 2020

feat: State Diff data for fraud proofs #118

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limiting Geth to one transaction per block#3

Limiting Geth to one transaction per block#3
willmeister merged 10 commits intomasterfrom
YAS-354/L1BatchGenerator

willmeister commented May 12, 2020

Uh oh!

masonforest left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ben-chain left a comment

Uh oh!

ben-chain May 14, 2020

Uh oh!

willmeister May 14, 2020

Uh oh!

ben-chain May 14, 2020

Uh oh!

willmeister May 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

willmeister commented May 12, 2020

Uh oh!

masonforest left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ben-chain left a comment

Choose a reason for hiding this comment

Uh oh!

ben-chain May 14, 2020

Choose a reason for hiding this comment

Uh oh!

willmeister May 14, 2020

Choose a reason for hiding this comment

Uh oh!

ben-chain May 14, 2020

Choose a reason for hiding this comment

Uh oh!

willmeister May 14, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants