Add completed FraudVerifier and StateTransitioner contracts#184
Add completed FraudVerifier and StateTransitioner contracts#184smartcontracts merged 46 commits intomasterfrom
Conversation
…po into YAS-467/rollup-contracts/refactor
…mism/optimism-monorepo into YAS-202/FraudVerifier/proofs
ben-chain
left a comment
ben-chain
left a comment
There was a problem hiding this comment.
WOOT WOOT this is looking 🔥
Left a bunch of comments inline, main high level question I have is around nuance of proving storage vs. contracts. Seems like we should only prove contract updates once all storage is updated, which I'm not positive is being enforced right now. Additionally, if the same slot is updated multiple times, we should ignore all but the last update to that slot. Is that being handled right now? Not sure if I'm missing something but I don't see it.
packages/contracts/contracts/optimistic-ethereum/ovm/FraudVerifier.sol
Outdated
Show resolved
Hide resolved
packages/contracts/contracts/optimistic-ethereum/ovm/FraudVerifier.sol
Outdated
Show resolved
Hide resolved
packages/contracts/contracts/optimistic-ethereum/ovm/FraudVerifier.sol
Outdated
Show resolved
Hide resolved
| * @dev Likely to be changed (if not moved to another contract). Currently | ||
| * remaining here as to avoid modifying CanonicalTransactionChain. Unclear |
There was a problem hiding this comment.
👍👍👍
Note that will be made actionable once #183 gets merged as it has that encoding. Worth spawning a ticket if you wanna cover in a future PR.
| interface OVMTransactionData { | ||
| timestamp: number | ||
| queueOrigin: number | ||
| ovmEntrypoint: string | ||
| callBytes: string | ||
| fromAddress: string | ||
| l1MsgSenderAddress: string | ||
| allowRevert: boolean | ||
| } |
There was a problem hiding this comment.
Same as above WRT consolidating types
packages/contracts/test/contracts/ovm/StateTransitioner.spec.ts
Outdated
Show resolved
Hide resolved
|
|
||
| expect(await stateManager.updatedStorageSlotCounter()).to.equal(3) | ||
|
|
||
| const newStateTrieRoot = await proveAllStorageUpdates( |
There was a problem hiding this comment.
Would it be possible to explicitly assert that the newStateTrieRoot has the final storage value set? (3 in this case I believe)
There was a problem hiding this comment.
Certainly possible. Checking newStateTrieRoot generated by the test helpers against the one inside the contract should be good enough (as that root is known to be correct).
| it('does not fail if all the state is supplied', async () => { | ||
| await prepareStateForTransactionExecution() | ||
| await stateTransitioner.applyTransaction() | ||
| describe('proveUpdatedContract(...)', async () => { |
There was a problem hiding this comment.
In addition to the contract creation tests here, I think the two other cases to watch out for would be:
- EOA nonce updates
- Contract nonce updates (but maybe this is covered already?)
There was a problem hiding this comment.
Yeah the nonce updates are covered - I actually don't have any code here for EOA accounts. Wasn't in the original sketch. I'll talk to Karl about this later today.
There was a problem hiding this comment.
@kfichter oh yeah so EOA accounts are actually (funny enough) covered by the transaction decoding logic. There are two ways to decode transactions (only one should be valid at a time):
- EOA transaction -- this encoding should have a
vrandswhich will be recoverable against the rest of the transaction. This is I believe similar to the tx decoding you sketched out in this PR. In this case, we set thefromaddress equal to the recovered signature value. - Non-EOA transaction -- this encoding is used when there is no
vrandspresent. In this case we will set thefromaddress toZERO_ADDRESS(and in the case that this is an L1->L2 transaction, we set thel1MessageSenderas well)
Note that EOAs are literally just treated as contracts. Maybe we should ensure that there is no EOA address which == some contract address... But I've been operating under the assumption that we shouldn't find a conflict like that
packages/contracts/test/contracts/ovm/StateTransitioner.spec.ts
Outdated
Show resolved
Hide resolved
|
@ben-chain wrt
It turns out we actually don't need to enforce this because the way the data structure works is we don't actually store the intermediate values, so the worst thing that can happen here is that you waste gas re-proving the same slot multiple times. That said, I think @kfichter brought this issue up before & may have actually added some logic which prevents redundant proofs to save on gas. |
|
@ben-chain this is actually something I need to change more generally. Same issue applies to contract nonces being changed. The existing PartialStateManager code will correctly assign the new state for each for pending update but still duplicate the updates (i.e., if a nonce goes 0 -> 1 -> 2 then we update the nonce to 2 three times). Should be easily addressable by tracking updates on a per-contract/per-slot basis. |
|
Unfortunately we have another circular dependency issue w.r.t. |
|
Another thought: Do we have code contract association logic? I see that we pass in the code hash to |
Yes -- the association occurs when the contract is created. Although that reminds me that I need to make sure it only happens during |
|
Ok @ben-chain I just resolved all of the suggestions you made (besides some of the type consolidation). |
Not sure exactly what you mean here -- code contracts need to be associated during both right? For both Also -- there's a non-creation code contract association which needs to occur too--previously deployed contracts need to have code contracts associated based on the |
smartcontracts
changed the title
…po into YAS-202/FraudVerifier/proofs
…m-v1.7.2 Merge upstream v1.7.2
* Revert to using time.Now() for migration block Instead of simply adding 5 to the parent block time. We really do need a deterministic time for the migration block so that all parties that run the migration arrive at the same migration block but the problem is that op-geth requires that the L2 migration block (aka l2 origin) occurs after the l1 origin (I guess the point where you deploy the bridge contracts to the l1). When we migrate a partially synced datadir the block before the transition block will be very old, up to 4 years old! So of course it occurs before the l1 origin. So a fix just to get things working is to use time.Now(), but probably we should make this a configurable parameter. * add flag to specify timestamp * Update op-chain-ops/cmd/celo-migrate/main.go --------- Co-authored-by: piersy <pierspowlesland@gmail.com>
* Revert to using time.Now() for migration block Instead of simply adding 5 to the parent block time. We really do need a deterministic time for the migration block so that all parties that run the migration arrive at the same migration block but the problem is that op-geth requires that the L2 migration block (aka l2 origin) occurs after the l1 origin (I guess the point where you deploy the bridge contracts to the l1). When we migrate a partially synced datadir the block before the transition block will be very old, up to 4 years old! So of course it occurs before the l1 origin. So a fix just to get things working is to use time.Now(), but probably we should make this a configurable parameter. * add flag to specify timestamp * Update op-chain-ops/cmd/celo-migrate/main.go --------- Co-authored-by: piersy <pierspowlesland@gmail.com>
* feat(preimage): Async server components Makes the server components in `kona-preimage` async to allow for remote data fetching in the `host` program. * feat(host): Host program scaffold Introduces the scaffolding and initial implementation of the `kona-host` program. * feat(host): Add L1 txs, L1 receipts, & L1 precompile hint routes
### Description Releases `0.5.1`. Unblocks @clabby from refactoring `kona-host` to remove the `debug_*` endpoints since alloy-rs/op-alloy#183 was merged.
3 participants
Description
This PR implements the
FraudVerifierandStateTransitionercontracts in full. Includes tests.Metadata
Fixes
Contributing Agreement