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 preconfirmation support [helder] #17654

Draft
wants to merge 49 commits into
base: helder
Choose a base branch
from

Conversation

Brechtpd
Copy link
Contributor

@Brechtpd Brechtpd commented Jun 23, 2024

For now these changes are only to support the upcoming preconfirmation testnet!

Tried to minimize changes with how ISequencerRegistry is implemented using a callback inside the propose function, otherwise I would probably add an additional contract in front of TaikoL1 but that seems like a bigger change for multiple reasons.

Untested.

Copy link

openzeppelin-code bot commented Jun 23, 2024

feat(protocol): add preconfirmation support [helder]

Generated at commit: 915f88852aea88e828a94181b07148dd9c753f28

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
2
2
0
8
42
54
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

@dantaik dantaik requested a review from YoGhurt111 June 24, 2024 13:16
@dantaik
Copy link
Contributor

dantaik commented Jun 25, 2024

Brecht, my understanding is that if we have two preconfers on slot 5 and 31, only the first perconfer can propose blocks in slot 0 to slot 5, and only the second preconfer can propose block in slot 6 to 31 (this is Justin's original proposal I believe). The l1StateBlockNumber in your PR is to help these preconfers to propose blocks with new restrictions so that L2 blocks proposed by a preconfer will not be included in other out-of-range L1 blocks unexpected.

In the implementation of isEligibleSigner, should we also check if the msg.sender can propose block for block.number % 32?

@Brechtpd
Copy link
Contributor Author

Two more changes in latest version:

  • Also allow the L2 block's timestamp to be chosen by the proposer with similar constraints as the l1StateBlockNumber. This is to solve potential problems if there are missed slots which made the previous calculation not deterministic (thanks gd for noticing this!)
  • Saw that the difficulty calculation still contained a dependency on block.number which also made it not deterministic. It is calculated like this: difficulty = keccak256(abi.encodePacked(blockhash(params.l1StateBlockNumber), b.numBlocks));

@davidtaikocha @dantaik @YoGhurt111

@Brechtpd Brechtpd changed the base branch from main to helder June 27, 2024 19:49
# Conflicts:
#	packages/protocol/contracts/L1/libs/LibProposing.sol
@Brechtpd Brechtpd marked this pull request as ready for review June 27, 2024 21:02
@Brechtpd Brechtpd changed the title feat(protocol): add preconfirmation support feat(protocol): add preconfirmation support [helder] Jun 27, 2024
}
}

require(_params.length == _txLists.length, "mismatched params length");
Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to think, it might be better like this below if we wanna support calldata efficiently.

        if(txLists.length != 0) {
            require(params.length == _txLists.length, "mismatched params length"));
        }

So currently protocol can handle both, but with the above change, we would have to create empty _txLists -matched to the length of _params, even if we use blob OR some empty if we use blob + calldata both, separately, paralel with one L1 TXN.

I dont think it is practical to use different data avail. layer for multiple L2 blocks but one L1 TXN. (Maybe i'm wrong in here, and in this case inefficiency is i guess negligible).

davidtaikocha and others added 10 commits July 16, 2024 09:35
)

Co-authored-by: gavin <gavin@taiko.xyz>
Co-authored-by: YoGhurt111 <YoGhurt111@users.noreply.github.com>
Co-authored-by: Jeffery Walsh <cyberhorsey@gmail.com>
Co-authored-by: jeff <113397187+cyberhorsey@users.noreply.github.com>
Co-authored-by: cyberhorsey <cyberhorsey@users.noreply.github.com>
@dantaik dantaik marked this pull request as draft August 16, 2024 02:06
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.

6 participants