Improved Optimism Bedrock Compatibility #2713
Conversation
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Just dropping this link list here from some internal chat discussion so that it won't get lost:
(guess most of this should be retrievable by the now excellently structured official docs though as well) Will also mention @kchojn here since he might be interested in this work (we had a chat exchange on Discord). |
|
Thanks @holgerd77. Let me know if I should open an issue, mainly because these problems are more related to Optimism and not Bedrock in detail. Before Berlin Hardfork:EIIP-2929Berlin Hardfork Block Number: 3950000; In general, a minimal version of EIP-2929 is needed:
for reprocessing purposes, I added my JSON file with eip2929-minimal, this allowed the blocks to be reprocessed Btw, before the Berlin hardfork, basically everything was treated as a new storage slot. L1BLOCK_NUMBERThis opcode is missing for Optimism. After Berlin Hardfork:GasRefund
0 addressOptimism has some system transactions that don't quite work like legacy transactions. The transaction is a system transaction, but it has
const v = (rpcTransaction as any).v === "0x0" ? undefined : (rpcTransaction as any).v;
const r = (rpcTransaction as any).r === "0x0" ? undefined : (rpcTransaction as any).r;
const s = (rpcTransaction as any).s === "0x0" ? undefined : (rpcTransaction as any).s;
isSigned();
{
// const { v, r, s } = this;
// if (v === undefined || r === undefined || s === undefined) {
// return false;
// }
// else {
// return true;
// }
return true;
}Sometimes When does a transaction have a zero address as the sender, it To fix it, we need:
getSenderPublicKey();
{
const isOptimismNetwork = this.common.chainName().toLowerCase().includes("optimistic");
const msgHash = this.getMessageToVerifySignature();
const { v, r, s } = this;
this._validateHighS();
try {
return (0, util_1.ecrecover)(msgHash, v, (0, util_1.bigIntToUnpaddedBuffer)(r), (0, util_1.bigIntToUnpaddedBuffer)(s), this.supports(types_1.Capability.EIP155ReplayProtection) ? this.common.chainId() : undefined);
} catch (e) {
const msg = this._errorMsg("Invalid Signature");
// Optimism network zero address handler
if (isOptimismNetwork) {
return null;
}
throw new Error(msg);
}
}In this case, we added a check for the Optimism network and if it is, then we return null (not an error).
getSenderAddress();
{
const senderPublicKey = this.getSenderPublicKey();
if (!senderPublicKey) {
return new util_1.Address(Buffer.alloc(20));
}
return new util_1.Address((0, util_1.publicToAddress)(this.getSenderPublicKey()));
}In this case, we added a check if the senderPublicKey is null, then we return the zero address.
await this._emit("beforeTx", tx);
let caller = tx.getSenderAddress();
if (caller.equals(util_1.Address.zero()) && this._common.chainName().toLowerCase().includes("optimistic")) {
const senderAddr = "36bde71c97b33cc4729cf772ae268934f7ab70b2";
caller = new util_1.Address(Buffer.from(senderAddr, "hex"));
}In this case, we added a check if the caller is the zero address and if it is, then we replace it with the special CALLgas.js const warmStorageRead = common.param("gasPrices", "warmstorageread");
const coldAccountAccess = common.param("gasPrices", "coldaccountaccess");
const coldCost = coldAccountAccess - warmStorageRead;
I Had to do some changes in gas calculation in SummaryMost of these changes are temporary and some kind of hack. But it allowed me to reprocess more than 99.0% of blocks in |
|
Hi @kchojn Karol, My current tendency would be to "only" make EthereumJS Bedrock compatible and do not focus too much on the old blocks (and e.g. not add the Doesn't need to be decided ad hoc though, also not against to take the older stuff in. We just also do not have to decide now but can also slowly expand later on if needed. For the system txs I guess it's likely worth to implement this as a dedicated tx type, not sure if would want to start with this now, but in general. For now I might also just skip them or something, or side-use some hacks. One question on your technical setup: did you process the Optimism blocks with the EthersStateManager? 🤔 Or some other setup? And did you use our next-v7-releases code from the Anyhow: again, thanks a lot!! 😀 ❤️ 🙏 |
|
Update: ah, I will just start with implementing the Optimism system tx type today, then we'll have this clean and do not always need to route around this or do some hacky stuff and then we can just simply run the full blocks with e.g. I will also introduce a high-level |
holgerd77
force-pushed
the
optimism-bedrock-compatibility
branch
from
0d49f20
to
33e8a26
Compare
|
@holgerd77 |
|
@kchojn Ah, just read your system tx section, I will be able to do this a lot cleaner with a dedicated tx type, your comments are super helpful though, can just go along one by one (so there are for sure (?) no Optimism 1559 system tx, right?). I have also added the Update: Ah, just re-read and realized that you meant "Dedicated Types" for "Transaction Types", yeah, this also needs to remain selective. For Optimism though this very much seems justified to me, this is at the end only one type and this system tx type is very prevalent. And the switches are there anyhow, I think it will rather be a bit cleaner with a type inheriting from I guess
🤯 That's totally impressive! So we on our side will work towards having the EthersStateManager ready to also be used towards Optimism, you can there pass in your Optimism-provider backed Ethers instance and then request state on the fly. Both EthersStateManager/StateManager and generally all the EVM/VM state related stuff is under heavy rework right now for v7 (so this indicates our breaking release round for all EthereumJS libraries), but in the finishing stages, #2702 from @jochem-brouwer is finalizing the EVM/VM state refactoring, likely to be merged in the current or next week.
Bedrock changes will be for v7, v6 is in maintenance mode now. We are relatively close, so there will be - minimally - 2 pre-releases during June and then likely the final releases in July this year! 😀 |
|
@holgerd77
By the final release, I will have time to test your changes for Bedrock. 👀 Still fully focused on the history, |
holgerd77
force-pushed
the
optimism-bedrock-compatibility
branch
2 times, most recently
from
bc6da15
to
f37bde3
Compare
|
Squashed some commits here and rebased and retargeted towards #2702 (EEI/EVM refactor work from Jochem) |
holgerd77
force-pushed
the
optimism-bedrock-compatibility
branch
from
48c61b2
to
8d0e93d
Compare
|
Cherry-picked this on top of |
holgerd77
force-pushed
the
optimism-bedrock-compatibility
branch
from
8d0e93d
to
16fa479
Compare
holgerd77
changed the title
holgerd77
force-pushed
the
optimism-bedrock-compatibility
branch
from
16fa479
to
f77d4e4
Compare
…) -> Block.fromRPC())
…m -> OptimismEthereum, add OptimismGoerli custom chain
…F block to Common
…m RPC example to README
holgerd77
force-pushed
the
optimism-bedrock-compatibility
branch
from
f77d4e4
to
db919ab
Compare
There was a problem hiding this comment.
Left a number of comments. The biggest concern I have is hardcoding non-mainnet hardforks in our code. That feels like we're entering a path of committing to support every L2 that comes along and wants a custom configuration (or else having to pick which ones are "worth" maintaining a configuration for).
| transactions.push(tx) | ||
| let skip = false | ||
| if (opts?.skipTxTypes !== undefined && txData.type !== undefined) { | ||
| const txType = Number(bytesToInt(toBytes(txData.type))) |
There was a problem hiding this comment.
Why is this cast to Number if bytesToInt returns a Number?
| @@ -51,6 +53,7 @@ export class Block { | |||
| public readonly withdrawals?: Withdrawal[] | |||
| public readonly txTrie = new Trie() | |||
| public readonly _common: Common | |||
| public readonly _skipTxTypes: number[] | |||
There was a problem hiding this comment.
Would it be slightly more efficient to use Bigint here so you don't need to conver the tx.type value to bytes and then to number down below?
| @@ -153,6 +165,9 @@ export class Block { | |||
| * @param opts | |||
| */ | |||
| public static fromValuesArray(values: BlockBytes, opts?: BlockOptions) { | |||
| if (opts?.skipTxTypes !== undefined) { | |||
| throw new Error('constructor usage not possible in combination with skipTxTypes option') | |||
There was a problem hiding this comment.
| throw new Error('constructor usage not possible in combination with skipTxTypes option') | |
| throw new Error('cannot use fromValuesArray in combination with skipTxTypes option, use fromBlockData instead') |
| @@ -479,6 +502,9 @@ export class Block { | |||
| validateTransactions(stringError: false): boolean | |||
| validateTransactions(stringError: true): string[] | |||
| validateTransactions(stringError = false) { | |||
| if (this._skipTxTypes.length > 0) { | |||
| throw new Error('tx validation not possible when skipTxTypes option is used') | |||
There was a problem hiding this comment.
Why can't we validate transactions here? Couldn't you just skip validating the specific transaction down below in the loop where it validates each transaction?
| * - Skip Optimism system txs (type: 126) | ||
| * | ||
| * Note that functionality of this library is limited when this option is used | ||
| * and validation- and serilization-related methods throw when called. |
There was a problem hiding this comment.
| * and validation- and serilization-related methods throw when called. | |
| * so validation and serialization-related methods throw when called. |
| @@ -175,6 +175,9 @@ export class EVM implements EVMInterface { | |||
| Hardfork.Paris, | |||
| Hardfork.Shanghai, | |||
| Hardfork.Cancun, | |||
| // Optimism (Bedrock) hardforks | |||
| Hardfork.Bedrock, | |||
| Hardfork.Regolith, | |||
There was a problem hiding this comment.
Per my comments above, rather than this hardcoded list of supported hardforks, I'd prefer to see a path where we take in a list of supported hardforks from common (and let people add arbitrary ones there). That way, it's not on us to maintain the list of hardforks for whatever L2 is popular next.
| skip = true | ||
| } | ||
| } | ||
| if (!skip) { |
There was a problem hiding this comment.
Is it possible to include the system transaction in the block's transaction list and just not validate that specific transaction or are there further complications if you include a system transaction in a block unvalidated?
| block.raw() | ||
| st.fail('should throw') | ||
| } catch (e) { | ||
| st.pass('should throw on raw() call') |
There was a problem hiding this comment.
| st.pass('should throw on raw() call') | |
| st.ok(e.message.includes('raw() method call not allowed when skipTxTypes option is used'), 'should throw on raw() call') |
| block.validateTransactions() | ||
| st.fail('should throw') | ||
| } catch (e) { | ||
| st.pass('should throw on validateTransactions() call') |
There was a problem hiding this comment.
As above, I like it when we check for the specific error message if we're checking to validate that a specific error is thrown (especially where we create the error message)
| await block.validateTransactionsTrie() | ||
| st.fail('should throw') | ||
| } catch (e) { | ||
| st.pass('should throw on validateTransactionsTrie() call') |
There was a problem hiding this comment.
As above, check the error message to verify it's throwing the correct error as an additional safety check.
|
Gm, |
I think it's worth trying. I know @holgerd77 tested it with a couple of Optimism Bedrock blocks from goerli a while back. We haven't rebased it recently so not sure if anything is missing or needs changes but let us know your results! |
|
@holgerd77 just started working on it. Is there any quick way to do a rebuild of this branch/pr in my project? I tried to do it manually, but tsc throws me errors every now and then Edit: nvm, I found a way, thx! |
|
@holgerd77, |
| @@ -0,0 +1,11 @@ | |||
| { | |||
| "name": "regolith", | |||
There was a problem hiding this comment.
In general maybe we should explicitly mark these hardforks as optimism? So we would use optimism/regolith as name (or something similar)?
There was a problem hiding this comment.
And maybe also in Hardfork.Optimism.Regolith as enum?
There was a problem hiding this comment.
Yeah, that makes likely sense, we can give these kind of things a bit more thinking now that we have more time on this.
There was a problem hiding this comment.
(the namespaced version might already be a good choice, good idea)
Yeah, this work here turned out to get very generic, so the new flag is basically just a universal tool just for these kind of tx-type-unknown situation. 🙂 This will likely be helpful for various situations, but I already thought that this will likely not fit your use case. It would be totally great if you would want to add broader Optimism support! ❤️ Did a quick consultation on this in our internal chat to confirm. We can promise you continued support on this with a close feedback loop! So what we will short-term do is just to extract the What you can cherry-pick from here - if you want - is likely the deep Yes, and then I would think the system tx type is the main thing. And for our "library thinking" we always want to have the EVM/VM included, so it would be good if this gets to a point (again) where an Optimism block can be instantiated and then run in the VM with `VM.runBlock(). Not sure if the VM is doing anything on system txs? (maybe change some state?). So I guess that would be what we would call "library-compatible". If we can run this with VM.runBlock() and the state would be matching afterwards. Does this help? Really curious to see what will come out of this! 🤩 |
|
@holgerd77 small update, As soon as I update
I guess, it will support Edit: I know it took a long time, but I'm working on a lot of things 😵💫 |
|
This is amazing! I work at OP labs so let me know if I can help get this over finish line |
|
@kchojn Yes, can only agree: this is amazing! 🤩 Exciting to hear! ❤️ |
Hey, thanks for the awesome work! Any updates on the PR? I'd be interested to get involved and can start after any updates have been pushed if there are any. |
This PR aims to improve compatibility with the upcoming Optimism Bedrock release, using the Bedrock Goerli deployment as a testbed.