Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

all: v1.15.0 pre-release merge review fixes #481

Closed

Conversation

protolambda
Copy link
Collaborator

@protolambda protolambda commented Jan 28, 2025

Description

Fixes on top of #474

Ledger diff

Original op-geth ledger mac workaround, due to usage-ID not being detected correctly, and being 0:
https://github.com/ethereum-optimism/op-geth/pull/223/files
Upstream PR was closed, since it was deemed a workaround:
ethereum/go-ethereum#28863
This issue then tracked the non-workaround, of fixing the USB lib:
ethereum/go-ethereum#28711
The USB lib was then fixed in:
ethereum/go-ethereum#28945
And recently mdehoog from Base made upstream Ledger fixes, for new devices, that caused a merge conflict:
ethereum/go-ethereum#31004

We're going with the upstream ledger code, and dropping the diff, since it should no longer be necessary.

Signer diff

We should just entirely disable the Prague signer, until L2-Pectra support is allocated to.
The state-transition doesn't check the allowed tx-types,
beyond just using the right kind of signer, which naturally filters transactions.

Returning a prague-signer, if there's some hint of prague in the config,
in LatestSigner is unsafe, as it would allow blob-transactions to be accepted into the chain.

So for now we don't return prague-signer in LatestSigner and MakeSigner.
We can later update the diff to not allow blob-txs selectively on optimism-config.

Snap-sync legacy-diff

ContractCode seemed to be removed from your diff.
Technically we need it to serve unprefixed contract-code nodes from legacy DBs to snap-sync.
Any node that resynced will have prefixes, but I think the oldest bedrock-transition DB snapshots don't have this prefixing.
Note that non-snap-sync code access upstream is still attached to the legacy code-reading method.

core/genesis.go diff

  • The overrides application moved to its own object & apply func in upstream.
  • OP-Stack overrides were moved over correctly
  • A TODO remained:
// TODO: Reinstate transitioned network check. Not sure where exactly to put it after this refactor.

On old code:

// If the bedrock block is not 0, that implies that the network was migrated at the bedrock block.
// In this case the genesis state may not be in the state database (e.g. op-geth is performing a snap
// sync without an existing datadir) & even if it were, would not be useful as op-geth is not able to
// execute the pre-bedrock STF.
// transitionedNetwork := genesis != nil && genesis.Config != nil && genesis.Config.BedrockBlock != nil && genesis.Config.BedrockBlock.Uint64() != 0

In the upstream commit,
the following comment was left:

**(b) The genesis initialization condition has been simplified**
There is a special mode supported by the Geth is that: Geth can be
initialized with an existing chain segment, which can fasten the node sync
process by retaining the chain freezer folder.

Originally, if the triedb is regarded as uninitialized and the genesis block can
be found in the chain freezer, the genesis block along with genesis state will be
committed. This condition has been simplified to checking the presence of chain
config in key-value store. The existence of chain config can represent the genesis
has been committed.

So upstream they're doing something similar: load an existing chain, without existing genesis. But then they proceed to construct and commit the genesis into the regular DB.

In the storedCfg == nil case, the ToBlock() and Commit() calls here handle the migrated-chain case where we start with a genesis with embedded StateHash instead of full genesis trie.
So for stateless snap-sync nodes, this code should run.

However, the bedrock-migration datadir genesis config may (not sure, to be confirmed) not have the extra StateHash field.
In that case, we may need to avoid committing the DB again, as we'll be unable to reconstruct the genesis block state-root from the genesis config.
I added a safeguard for this, to not try to overwrite the genesis data with incomplete data.
If we download the migration datadir snapshot, then we can give it a try, to confirm what happens here.

Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM, just some nits and open questions.

Copy link
Member

Choose a reason for hiding this comment

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

we need to revisit this when we add L2 Pectra support, cc @danyalprout

miner/miner_test.go Show resolved Hide resolved
core/genesis.go Show resolved Hide resolved
@protolambda
Copy link
Collaborator Author

Closing, we're going forward with the single-merge-commit PR #480

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants