Adding L1MessageSender to Transaction by willmeister · Pull Request #4 · ethereum-optimism/go-ethereum

willmeister · 2020-05-14T22:53:44Z

Adds L1MessageSender to Transaction

…h computation

ben-chain

Woohoo!! This is looking awesome, super sweet to have the whole ? addition to the RLP--looks super clean 😀

I left a few small comments--I think most critical question is understanding why this new field is being omitted from the hash even if present as it feels scary to have equivalent hashes for two different transactions.

Last thought--not sure it's necessarily in this issue's scope, but we will need to make sure that users cannot sendRawTransaction with an L1MessageSender as this would result in accidental fraud. Might be another ticket, but hadn't thought of it until just now while reviewing.

ben-chain · 2020-05-19T16:26:26Z

@@ -219,13 +247,14 @@ func (tx *Transaction) Size() common.StorageSize {
 // XXX Rename message to something less arbitrary?


Below you'll notice I got confused about what these messages are. Since there is a note here already in geth to consider renaming this, and we are adding a new "message" keyword in this PR, we might want to consider it. xD

Not sure how this is used, just updated it since it is Transaction-like. Even if poorly named, we should probably keep parity with Geth (👀) on anything that we don't have a good reason to change or pulling in updates from Geth will be even hairier than it would otherwise be.

ben-chain · 2020-05-19T16:30:34Z

+		t.Fatalf("encode error: %v", err)
+	}
+	if bytes.Equal(txc, should) {
+		t.Errorf("RLP encoding with L1MessageSender should be different than without. Got %x", txc)


👌👌👌

ben-chain · 2020-05-19T16:35:52Z

 		if tx.ChainId().Cmp(parsedTx.ChainId()) != 0 {
 			t.Errorf("invalid chain id, want %d, got %d", tx.ChainId(), parsedTx.ChainId())
 		}
+		if tx.L1MessageSender() != nil && parsedTx.L1MessageSender() != nil && *tx.L1MessageSender() != *parsedTx.L1MessageSender() {


Noting that it seems like we would not catch an error here if tx.L1MessageSender() != nil but parsedTx.L1MessageSender() == nil. Not positive it's an issue but thought worth noting.

If parsedTx.L1MessageSender() == nil, evaluating *parsedTx.L1MessageSender() will throw a Null Pointer Exception, since we're trying to access the memory address pointed to by nil.

Ahh great, and presumably all three args are evaluated even if the second one is false. Got it!

No, it should eagerly exit the evaluation. Sorry, I misread your comment. Yes, I'll add a check if one is nil but the other is not.

ben-chain · 2020-05-19T16:42:18Z

+}
+
+// Tests that L1MessageSender has no impact on hash
+func TestL1MessageSenderHash(t *testing.T) {


I see the logic above where you are enforcing this, but don't fully understand why--is it related to not breaking tests?

I feel like it shouldn't matter either way since these transactions will already be hashed on chain, and probably not with an RLP encoding. But still confused why this is done.

Yeah, this would break all tests with pre-computed hashes and interaction with any 3rd party tools (for instance, ethers throws saying that hash != expected hash). I'd be concerned if it's possible to have the same exact message but with a different L1 sender or an empty one, but the L1 sender will only be populated and submitted by our L1 subscriber, so the nonce, at a minimum, should be different, right? If so, then we can maintain compatibility without worrying about uniqueness.

Ahhh okay yeah I see your point WRT compatibility.

Hadn't thought of nonces being different too, so good point there, I think you're right. It would require that we always pass in the correct nonce in these transactions, which shouldn't be a problem. Although it's. an interesting point that we don't currently expose that nonce to the execution context unless I'm mistaken. Might want to add that 🤔

ben-chain · 2020-05-19T16:45:45Z

+	rollupBlockBuilder, err := rollup.NewTransitionBatchBuilder(db, blockchain, blockSubmitter, 5*time.Minute, 9_000_000_000, 200)
 	if err != nil {
-		panic(fmt.Errorf("failed to create Rollup Block Builder: %v", err)
+		panic(fmt.Errorf("failed to create Rollup Block Builder: %v", err))


Oh wow, were our builds previously failing? 😅😂

What's weird is that builds were not, but yes, running tests would fail. It appears as if tests are JIT compiled or something.

Co-authored-by: ben-chain <ben@pseudonym.party>

…ssageSender in rawdb

…m/go-ethereum into YAS-377/L1MessageSender

* 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>

* 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>

willmeister added 5 commits May 14, 2020 17:52

First stab at adding L1MessageSender to Transaction

c6a3784

Adding logic to omit L1MessageSender when nil and never use it in has…

b3ad38d

…h computation

Adding tests

bcee9ca

uncommenting out RLP nil logic used for testing

2ba4bcf

Formatting

183c425

ben-chain approved these changes May 19, 2020

View reviewed changes

willmeister and others added 5 commits May 19, 2020 12:42

Update core/types/transaction.go

0183a44

Co-authored-by: ben-chain <ben@pseudonym.party>

Adding extra check in transaction unit test and testing with nil L1Me…

5858857

…ssageSender in rawdb

Merge branch 'YAS-377/L1MessageSender' of github.com:ethereum-optimis…

3c590f9

…m/go-ethereum into YAS-377/L1MessageSender

Making NewTransaction use the pointer it is passed.

e9eba42

Adding function comments for the wrapper functions

3eec500

willmeister merged commit 67ee6f3 into master May 19, 2020

willmeister deleted the YAS-377/L1MessageSender branch May 19, 2020 19:29

		@@ -219,13 +247,14 @@ func (tx *Transaction) Size() common.StorageSize {
		// XXX Rename message to something less arbitrary?

Conversation

willmeister commented May 14, 2020

Uh oh!

ben-chain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants