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

feat(protocol): Add parent's metaHash to assignment #15498

Merged
merged 3 commits into from
Jan 18, 2024

Conversation

adaki2004
Copy link
Contributor

As per SigmaP.:

Proving may vary significantly based on the initial state (including storage variables, nonces, account balances, execution flow paths etc). So it could be worth including the parentHash field for assignments if they want to know transaction execution before agreeing to an assignment. It depends on whether the prover wants / needs to know the execution of a set of transaction before agreeing to an assignment.

+1: We don't need this storage var:

TaikoData.Block storage parent =
            state.blocks[(b.numBlocks - 1) % config.blockRingBufferSize];

- remove Block struct storage var
- add parentMetaHash to assignment
@adaki2004 adaki2004 changed the base branch from main to alpha-6 January 15, 2024 07:23
Copy link

vercel bot commented Jan 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
bridge-ui-v2-a5 ✅ Ready (Inspect) Visit Preview Jan 18, 2024 7:13am
bridge-ui-v2-a6 ✅ Ready (Inspect) Visit Preview Jan 18, 2024 7:13am
bridge-ui-v2-internal ✅ Ready (Inspect) Visit Preview Jan 18, 2024 7:13am

@davidtaikocha
Copy link
Member

davidtaikocha commented Jan 15, 2024

we have optional maxBlockId , proposedIn, they are almost the same? If a proposer wants to propose based on a known parent, he can simply use assignment.maxBlockId right?

@adaki2004
Copy link
Contributor Author

I think if we require this, then our proposers won't be able to include multiple L2 blocks in one L1 block anymore (then TPS issue)? And we have optional maxBlockId , proposedIn, they are almost the same? If a proposer wants to propose based on a known parent, he can simply use assignment.maxBlockId right?

We do not introduce new functionality, we just adding the parentMetaHash in the assigment - which can be still 0.
This is for "conditional" transactions - if any. The maxBlockIn and proposedIn is for L1 - and to prevent that a proposer can submit blocks maybe 10 days in the future, so a different purpose.

@adaki2004
Copy link
Contributor Author

Adding here the related convo from TG:

Kirk from SigP.:
proving may vary significantly based on the initial state (including storage variables, nonces, account balances, execution flow paths etc). So it could be worth including the parentHash field for assignments if they want to know transaction execution before agreeing to an assignment. It depends on whether the prover wants / needs to know the execution of a set of transaction before agreeing to an assignment.

Dani:
parentMetaHash is part of the BlockMetadata. (And we agreed within the team we keep the metaHash inside the asignment, even if it could be removed ATM but maybe future hooks needs it.. and also because of this reason for example you jsut mentioned :) ) So:
// Check if parent block has the right meta hash
if (params.parentMetaHash != 0 && parent.metaHash != params.parentMetaHash) {
revert L1_UNEXPECTED_PARENT();
}
So if parentMetaHash is set arbitrarily, it will not match up the (simulated and) signed metaHash so assignment is invalid.

Kirk:
_for parentMetaHash vs metaHash, it's very challenging to know the current meta hash when creating an assignment since you'd need to know the block.number in which the proposal is created, along with some other fields that are not easily determined. So I don't believe using assignment.metaHash is feasible for the assignment hook.

For adding the parentMetaHash in the assignment it allows the prover to know exactly what the state transition occur in the block and this can only be known if you know the initial state. The inital state is "fixed" once the parentMetaHash is set. You're welcome to debate whether the prover wants to know the exact state transition before they begin proving. The main two cases I think of are if there is an unprovable block or if there are certain efficiency gains for smaller executions which would making proving cheaper and therefore they can ask a lower fee.

If you were to set it, it would be the same logic as the remaining fields where we only consider the non zero inputs e.g._

        if (
            block.timestamp > assignment.expiry
                || assignment.metaHash != 0 && blk.metaHash != assignment.metaHash
                || assignment.parentMetaHash != 0 && blk.parentMetaHash != assignment.parentMetaHash // if parentMetaHash is set check it
                || assignment.maxBlockId != 0 && meta.id > assignment.maxBlockId
                || assignment.maxProposedIn != 0 && block.number > assignment.maxProposedIn
        ) {
            revert HOOK_ASSIGNMENT_EXPIRED();
        }

@dantaik @Brechtpd just tagging you as you need to call for action in this case. I do not see it a bug or critical, but i find it a useful feature.

@dantaik
Copy link
Contributor

dantaik commented Jan 15, 2024

After reading the comments, I feel like we should remove checking metaHash from onBlockProposed and instead checking the following data in the metadata:

  • blobHash
  • coinbase
  • gasLimit
  • txListByteOffset
  • txListByteSize
  • minTier,
  • blobUsed
  • parentMetaHash

But these change will involve upgrading the current A6 testnet, so we need to create a second AssignmentHook implementation and keep the old one as-is we also need to define a new ProverAssignment object in the new impl.

@Brechtpd
Copy link
Contributor

Copying my comments from telegram here:

a)

There's still the question here about why would a prover want to cancel an assignment? The impact here is low as a proposer wastes some gas and prover doesn't receive the assignment. I don't know of any significant concerns here.

I think there's a possible attack vector. Let's say there's some very attractive MEV in an L2 block. If the promise of the prover is easily revertible out of the proposer's control, there's an easy way to make sure you win the game to propose a block:

  1. Create the block with a lot of MEV
  2. Spin up a fake prover that promises to prove blocks for the lowest price, which will ensure all other proposers will try to propose their block with this "fake" promise
  3. Propose the block for a reasonable but lower than normally necessary price so it gets picked up by the L1 builder/L1 validator
  4. At the last time possible, cancel the promise in any way, too late for competing proposers to switch to another prover
  5. All competing proposers' blocks won't get proposed because the tx reverts (which won't waste any gas for the proposer when only paying the fee through PBS)
  6. Your block proposal now gets submitted first and you collect the MEV.

b)

For adding the parentMetaHash in the assignment it allows the prover to know exactly what the state
transition occur in the block and this can only be known if you know the initial state. The inital state is "fixed" once the parentMetaHash is set.

The parentMetaHash currently does not completely make the L2 block deterministic. That's because inputs like l1Hash, l1Height timestamp and difficulty also directly impact how the L2 block will execute as they are made available immediately to the L2 block and will be accessible directly from the EVM in that block.

So I think using metaHash is really the only way to make it possible for the prover to accurately predict provers costs. Even more, it's also the only way for the proposer to know how much fees it will collect from the block and so the value being known, while challenging on testnet, is almost inevitably a precondition for proposers to even propose the block (otherwise they risk losing money, unless they have some heuristics to calculate the risk depending on the block and if those values changing could impact the profitability of the block). On mainnet however, in a very competitive environment and with PBS available, I assume within each L1 block time new L2 blocks will be created and proposed for this reason, and so I would assume it would be be fine to depend on this value.

@kirk-baird
Copy link
Contributor

The parentMetaHash currently does not completely make the L2 block deterministic. That's because inputs like l1Hash, l1Height timestamp and difficulty also directly impact how the L2 block will execute as they are made available immediately to the L2 block and will be accessible directly from the EVM in that block.

This is a very good point these fields can also significantly effect proving, especially since they are available in TaikoL2.sol so smart contracts can create different branches using these variables. So I'll conceed that parent meta hash is not sufficient for deterministic execution you'll need the current block's meta hash.

@Brechtpd
Copy link
Contributor

Hmm not sure why this was merged in. What is the benefit of having the parentMetaHash check? Not sure I follow.

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

Successfully merging this pull request may close these issues.

5 participants