-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Plasma: Abstract CommitmentData to Interface #10458
Conversation
This is a generic commitment to be used by the op-node & op-batcher.
Semgrep found 1 Service 'op_stack_go_builder' is running with a writable root filesystem. This may allow malicious applications to download and run additional payloads, or modify container files. If an application inside a container has to save something temporarily consider using a tmpfs. Add 'read_only: true' to this service to prevent this. Ignore this finding from writable-filesystem-service.Semgrep found 1 Service 'op_stack_go_builder' allows for privilege escalation via setuid or setgid binaries. Add 'no-new-privileges:true' in 'security_opt' to prevent this. Ignore this finding from no-new-privileges. |
d9b70b2
to
c89f6ed
Compare
c89f6ed
to
25cf8f2
Compare
473643f
to
ba3c0cd
Compare
WalkthroughWalkthroughThe recent updates across various files involve transitioning from using Changes
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (2)
Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The design looks really good. I just found some small things
906f8da
to
28f1c5a
Compare
* op-node: Generic Commitment This is a generic commitment to be used by the op-node & op-batcher. * abstract commitments to CommitmentData interface * correct byte-stripping ; add tests ; finish wiring * make GenericCommitment always verify * correct action tests * PR comments * fix unit test * remove fmt.Println --------- Co-authored-by: Joshua Gutow <jgutow@oplabs.co>
What
Abstracts Keccak and Generic Commitment types to a
CommitmentData
, and wires it throughout the projectWhy
op-node
must support different modes of operation for Generic and Keccak commitments, which are signaled by a type byte at the front of the data.How
Now, in all places where a commitment is decoded, the package function
DecodeCommitmentData
is used. This handles the type check, does some basic validation, and then calls down to the implementation-specific decoder.And, in all places where a commitment is encoded,
NewCommitmentData
is used. This function takes theCommitmentType
to decide which concrete type to use.The
CommitmentData
interface replaces all reference to concrete types in existing Plasma/DA behaviors.Outside of these activities, all access to the
CommitmentData
happens through interface-defined functions which Keccak and Generic already supported --Encode, Verify, TxData
Omitting
Not featured in this PR is any feature-flagging from the node for the expected Commitment Type. @trianglesphere has a plan in mind to group the config details, so that will be wired in then. The DA Client similarly is set to only produce Keccak commitments.
Testing
Tests were written for the CommitmentData interface to confirm it creates the right type of commitment and handles errors as expected.
DA server tests are fixed to type-assert Keccak type Commitments, as the server components are designed to only use those. When flags are wired into the
Next
andSetInput
functions, the test can be multiplied by both Commitment Types. The Mock DA Server is already set to handle that with this PR, you can just overload the Commitment Type the mock uses.