fix: EVM emulation integration – upgrade workflow fixes #3593
Conversation
| // Speculative VM version for the next protocol version to be used in the upgrade integration test etc. | ||
| // TODO: Must be changed when the protocol version is actually implemented! | ||
| ProtocolVersionId::Version28 => VmVersion::VmGateway, |
There was a problem hiding this comment.
Is this the correct way to do things? I don't think the upgrade integration test would work otherwise, since it's supposed to run with the next protocol version after the upgrade.
There was a problem hiding this comment.
Shouldn't we use the new subversion (smth like VmEvmEmulator) here?
There was a problem hiding this comment.
Once we determine how to deal with the new VM version in multivm, I'd imagine that yes we'd have VmVersion:: VmEvmEmulator for protocol versions v27 and v28.
| match Self::decode_from_token(schema, token) { | ||
| Ok(decoded) => { | ||
| if let Some(upgrade) = &upgrade { | ||
| anyhow::bail!("Ambiguous upgrade: can be decoded as either {decoded:?} or {upgrade:?}"); |
There was a problem hiding this comment.
I don't like this approach since there's no guarantee that this error won't trigger. In practice, the new upgrade payloads decode successfully using either the post-gateway or the post-EVM schema, and the only thing stopping the post-gateway decoding from being correct is a bogus protocol version (0). Ideally, I'd imagine that the protocol version (or maybe the schema identifier itself?) would be encoded at the known offset of the payload.
There was a problem hiding this comment.
This is indeed unfortunate. Most likely changing encoding schema in L1 contracts is not possible at this point without re-audit. As a more stable workaround you can pass DecentralizedUpgradesEventProcessor::last_seen_protocol_version + 1 down to decode method as a hint to choose the schema
There was a problem hiding this comment.
It could prevent some ambiguous decodings, but not all of them, IIUC: the protocol version may stay the same with an upgrade, which means that an ambiguous decoding I've described above would still occur if last_seen_protocol_version.minor == ProtocolVersionId::Version26 (since the upgrade may be either a patch bump or a v27 upgrade). Also, just for my understanding: Is it possible for an upgrade to skip a ProtocolVersionId (e.g., a v25 -> v27 upgrade)?
There was a problem hiding this comment.
Agree, it's not perfect either but no upgrade between v26 and v27 is planned. It's possible in theory, on practice it should only happen if we upgraded some devnet, found an issue, and want to keep the devnet live, then devnet will be upgraded this way x->x+1->x+2, and others x->x+2
There was a problem hiding this comment.
I've tried this approach with passing the min expected version, but it doesn't work straightforwardly because IIUC the watcher component may receive old upgrade events; here's the corresponding filter. We most probably want to distinguish between valid old upgrades that can be skipped and upgrades that the node doesn't understand, which makes your proposal not quite straightforward. So if you don't mind, I'd leave the logic as-is.
There was a problem hiding this comment.
no upgrade between v26 and v27 is planned
We had some discussions about v26.1
| @@ -265,12 +266,18 @@ describe('Upgrade test', function () { | |||
| 'contracts/system-contracts/artifacts-zk/contracts-preprocessed/DefaultAccount.sol/DefaultAccount.json' | |||
| ); | |||
|
|
|||
| const evmEmulatorCode = readCode( | |||
| 'contracts/system-contracts/zkout/EvmEmulator.yul/contracts-preprocessed/EvmEmulator.yul.json' | |||
There was a problem hiding this comment.
Output path from compiler was changed several times recently, so we have some ugly functions to read bytecode. Iiuc, it isn't guaranteed that this hardcoded path will remain valid in the future (but I hope so)
What ❔
Fixes the upgrade workflow with new L1 contracts.
Why ❔
The existing workflow doesn't work because of changes in
ProposedUpgrade(+ some stubs in the codebase).Checklist
zkstack dev fmtandzkstack dev lint.