Skip to content
This repository was archived by the owner on Apr 11, 2021. It is now read-only.

feat: raw tx #255

Merged
merged 10 commits into from
Mar 9, 2021
Merged

feat: raw tx #255

merged 10 commits into from
Mar 9, 2021

Conversation

karlfloersch
Copy link
Contributor

@karlfloersch karlfloersch commented Mar 6, 2021

Description

Makes L2Geth operate on raw tx data!

Metadata

Fixes

  • Fixes # [Link to Issue]

Contributing Agreement

@karlfloersch
Copy link
Contributor Author

Maybe we should rename RawTransaction to RawBlob? or Raw? Or idk

@karlfloersch
Copy link
Contributor Author

Also @tynes you might want to add to this PR & delete all the newly dead code in the sync service? I assume there is dead code there? Or maybe there isn't? Idk

@@ -640,5 +642,92 @@ func (s *SyncService) ApplyTransaction(tx *types.Transaction) error {
if err != nil {
return fmt.Errorf("invalid transaction: %w", err)
}

// Set the raw transaction data in the meta
txRaw, err := getRawTransaction(*tx)
Copy link
Collaborator

@tynes tynes Mar 7, 2021

Choose a reason for hiding this comment

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

Assuming the upgrade to use standard RLP serialization, this will be able to be removed

Copy link
Contributor

Choose a reason for hiding this comment

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

I just tried this branch against my WIP encoding PR (https://github.com/ethereum-optimism/contracts/pull/300/files) and replaced getRawTransaction(*tx) with rlp.EncodeToBytes(tx) and it worked first try. Woot!

@tynes tynes marked this pull request as ready for review March 7, 2021 01:44
@tynes
Copy link
Collaborator

tynes commented Mar 7, 2021

newRPCTransaction needs updating still

func newRPCTransaction(tx *types.Transaction, blockHash common.Hash, blockNumber uint64, index uint64) *RPCTransaction {

Update:
This has been addressed in 559e171

@tynes tynes changed the title Feat/raw tx feat: raw tx Mar 8, 2021
@tynes
Copy link
Collaborator

tynes commented Mar 8, 2021

This results in mismatched state roots. To reproduce:

$  ./scripts/start.sh --rollup.clienthttp http://127.0.0.1:7878 --verifier --datadir $HOME/.ethereum --rollup.statedumppath https://raw.githubusercontent.com/ethereum-optimism/regenesis/master/mainnet/1.json

From https://github.com/ethereum-optimism/scripts

$ VERIFIER_ENDPOINT=http://127.0.0.1:8545 SEQUENCER_ENDPOINT=https://mainnet.optimism.io node scripts/verifier/compare-verifier-sequencer.js

The compare-verifier-sequencer.js script will find the first mismatched state root. I think it has to do with the enum for tx type

@tynes
Copy link
Collaborator

tynes commented Mar 8, 2021

I've updated a branch in the contracts repo based on the contracts that are currently deployed to production to include the fixes required to get this to work.

The branch is found here:
https://github.com/ethereum-optimism/contracts/tree/v0.1.4/enums
From within the repo, run the command:

$ yarn serve

Then when starting geth, use the flag:

--rollup.statedumppath http://127.0.0.1:8081/state-dump.latest.json

Doing a state root check will not be sufficient since the genesis state roots will be different since this involves a change to the contract code of a predeploy. Working towards syncing mainnet and then will do a state diff to see if there are any differences. The only difference should be the codeHash for the sequencer entrypoint

@tynes
Copy link
Collaborator

tynes commented Mar 9, 2021

With the addition of 3319556 then ethereum-optimism/contracts#303 is not needed to be merged and it will also prevent more difficulties in managing versions and we can move towards ethereum-optimism/contracts#300

@tynes
Copy link
Collaborator

tynes commented Mar 9, 2021

f2114cb adds a safety check with an error log if a transaction with no raw is detected. This should never happen but will prevent a panic in case it does

@tynes tynes merged commit 31b5deb into master Mar 9, 2021
@tynes tynes deleted the feat/raw_tx branch March 9, 2021 19:05
tynes added a commit that referenced this pull request Apr 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants