-
Notifications
You must be signed in to change notification settings - Fork 0
CDCP Core Lib #9
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
base: main
Are you sure you want to change the base?
Conversation
- Simulation returns written messages. - sbcp/sequencer sealed block var renaming. - Proof timeout config logic - Proof timeout resets seq number to 0
This reverts commit c064476.
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.
Pull request overview
This PR introduces the Cross-Domain Composability Protocol (CDCP) for coordinating atomic execution between Compose native rollups and external rollups. The implementation provides three main components: Publisher (orchestrates consensus), Native Sequencer (wraps existing SCP sequencer), and Wrapped Sequencer (handles external rollup interactions with mailbox-aware simulations).
Key Changes
- Implements CDCP protocol with Publisher, Native Sequencer, and Wrapped Sequencer roles
- Adds comprehensive unit tests covering state machine transitions, error handling, and edge cases
- Includes detailed documentation with sequence diagrams and interface specifications
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| compose/cdcp/README.md | Protocol documentation with interface descriptions and sequence diagrams |
| compose/cdcp/publisher.go | Publisher implementation managing votes from native chains and decisions from wrapped sequencer |
| compose/cdcp/publisher_test.go | Comprehensive test suite for publisher state machine and edge cases |
| compose/cdcp/wrapped_sequencer.go | Wrapped sequencer implementation handling mailbox-aware simulations and ER client interaction |
| compose/cdcp/wrapped_sequencer_test.go | Test suite covering simulation flows, mailbox message handling, and timeout behavior |
| compose/cdcp/native_sequencer.go | Thin wrapper delegating to SCP sequencer for native chain behavior |
| compose/cdcp/helpers_test.go | Test utilities including fake implementations and data cloning helpers |
| compose/README.md | Updated module listing to include CDCP |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| compose.StateRoot{}, | ||
| testLogger(), | ||
| ) | ||
| // Ensure an error is returned chain has no transaction. |
Copilot
AI
Nov 28, 2025
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.
Comment says "Ensure an error is returned chain has no transaction" but should say "Ensure an error is returned when chain has no transaction". The word "when" is missing.
| // Ensure an error is returned chain has no transaction. | |
| // Ensure an error is returned when chain has no transaction. |
| chainReq(2, []byte("b")), | ||
| ) | ||
| _, err = NewPublisherInstance(instance, &fakePublisherNetwork{}, compose.ChainID(3), testLogger()) | ||
| // creation errors if the ER chain has no transaction. |
Copilot
AI
Nov 28, 2025
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.
Comment says "creation errors if the ER chain has no transaction" but should say "creation errors if the ER chain has no transactions" (plural) to match the error name ErrERNotFound and the validation logic context.
| // creation errors if the ER chain has no transaction. | |
| // creation errors if the ER chain has no transactions. |
| } | ||
|
|
||
| // Else, sends successful decision, and terminates | ||
| ws.logger.Info().Err(err).Msg("ER call succeeded. Sending WSDecided as true.") |
Copilot
AI
Nov 28, 2025
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 log message "ER call succeeded. Sending WSDecided as true." logs the error with .Err(err), but at this point err is nil (the success case). This will log a nil error unnecessarily. The .Err(err) should be removed from line 305.
| ws.logger.Info().Err(err).Msg("ER call succeeded. Sending WSDecided as true.") | |
| ws.logger.Info().Msg("ER call succeeded. Sending WSDecided as true.") |
| - `Simulate(args)`: simulates `safe_execute` against the given snapshot and returns the result. | ||
| - `ReadMiss`: tracks the required mailbox header and keeps waiting for native messages, re-simulating once satisfied. | ||
| - `WriteMiss`: stages the missing outbox message into `PutOutboxMessages` and re-simulates. | ||
| - Error: sends `WSDecided(false)` and terminates. | ||
| - `ProcessMailboxMessage(msg)`: buffers incoming mailbox messages for simulation; when a pending read is satisfied it moves the message into `PutInboxMessages` and re-runs the simulation. | ||
| - `ProcessNativeDecidedMessage(decided)`: deduplicates the publisher’s native decision and, if `true`, triggers the external rollup submission; `false` cancels execution. | ||
| - `Timeout()`: while not already waiting for the ER response, aborts and reports `WSDecided(false)`. | ||
|
|
Copilot
AI
Nov 28, 2025
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 documentation describes a Simulate(args) method for the WrappedSequencerInstance interface (line 76), but this method doesn't exist in the actual interface. The Simulate method is part of the WSExecutionEngine interface, not WrappedSequencerInstance. The documentation should remove this method description or clarify that it refers to the execution engine, not the sequencer instance itself.
| - `Simulate(args)`: simulates `safe_execute` against the given snapshot and returns the result. | |
| - `ReadMiss`: tracks the required mailbox header and keeps waiting for native messages, re-simulating once satisfied. | |
| - `WriteMiss`: stages the missing outbox message into `PutOutboxMessages` and re-simulates. | |
| - Error: sends `WSDecided(false)` and terminates. | |
| - `ProcessMailboxMessage(msg)`: buffers incoming mailbox messages for simulation; when a pending read is satisfied it moves the message into `PutInboxMessages` and re-runs the simulation. | |
| - `ProcessNativeDecidedMessage(decided)`: deduplicates the publisher’s native decision and, if `true`, triggers the external rollup submission; `false` cancels execution. | |
| - `Timeout()`: while not already waiting for the ER response, aborts and reports `WSDecided(false)`. | |
| - `ProcessMailboxMessage(msg)`: buffers incoming mailbox messages for simulation; when a pending read is satisfied it moves the message into `PutInboxMessages` and re-runs the simulation. | |
| - `ProcessNativeDecidedMessage(decided)`: deduplicates the publisher’s native decision and, if `true`, triggers the external rollup submission; `false` cancels execution. | |
| - `Timeout()`: while not already waiting for the ER response, aborts and reports `WSDecided(false)`. | |
| > **Note:** Simulation of `safe_execute` is performed by the `WSExecutionEngine` dependency, not directly by the `WrappedSequencerInstance` interface. |
| // Look if it exists in received messages | ||
| for receivedMsgIdx, receivedMsg := range ws.pendingMessages { | ||
| if receivedMsg.MailboxMessageHeader.Equal(expectedMsg) { | ||
| // If found, add to mailboxOps |
Copilot
AI
Nov 28, 2025
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.
Comment says "add to mailboxOps" but the code adds to putInboxMessages, not mailboxOps. The comment should be updated to say "add to putInboxMessages" to match the actual code.
| // If found, add to mailboxOps | |
| // If found, add to putInboxMessages |
| } | ||
|
|
||
| // NewNativeSequencerInstance returns a new native-sequencer CDCP instance. | ||
| // It has the exact same behaviour as in SCP, and thus its constructor can used. |
Copilot
AI
Nov 28, 2025
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.
Comment says "its constructor can used" but should say "its constructor can be used". The word "be" is missing.
| // It has the exact same behaviour as in SCP, and thus its constructor can used. | |
| // It has the exact same behaviour as in SCP, and thus its constructor can be used. |
| // Processed votes | ||
| require.NoError(t, pub.ProcessVote(compose.ChainID(1), true)) | ||
| require.NoError(t, pub.ProcessVote(compose.ChainID(2), true)) | ||
| // NativeDecided shoudl be sent with result true |
Copilot
AI
Nov 28, 2025
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.
Typo in comment: "shoudl" should be "should".
| // NativeDecided shoudl be sent with result true | |
| // NativeDecided should be sent with result true |
| require.Equal(t, 1, len(exec.requests)) | ||
|
|
||
| msg := makeMailboxMsg(1, 4, "late", []byte("payload")) | ||
| // ProcessMailboxMessage don't trigger any simulation requests. |
Copilot
AI
Nov 28, 2025
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.
Comment says "ProcessMailboxMessage don't trigger" but should say "ProcessMailboxMessage doesn't trigger". The verb should agree with the singular subject.
| // ProcessMailboxMessage don't trigger any simulation requests. | |
| // ProcessMailboxMessage doesn't trigger any simulation requests. |
| return r.instance | ||
| } | ||
|
|
||
| // Run performs launches the instance by sending a message to all participants. |
Copilot
AI
Nov 28, 2025
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.
Comment says "Run performs launches the instance" which is grammatically incorrect. It should either be "Run launches the instance" or "Run performs the instance launch".
| // Run performs launches the instance by sending a message to all participants. | |
| // Run launches the instance by sending a message to all participants. |
| func (ws *wsInstance) sendWriteMessages(messages []scp.MailboxMessage) { | ||
| for _, msg := range messages { | ||
| // Check if belongs to cache | ||
| alreadySent := false | ||
| for _, cachedMsg := range ws.writtenMessagesCache { | ||
| if cachedMsg.Equal(msg) { | ||
| alreadySent = true | ||
| break | ||
| } | ||
| } | ||
| if alreadySent { | ||
| continue | ||
| } | ||
|
|
||
| // Send and add to cache if new message | ||
| ws.network.SendMailboxMessage(msg.MailboxMessageHeader.DestChainID, msg) | ||
| ws.writtenMessagesCache = append(ws.writtenMessagesCache, msg) | ||
| } | ||
| } |
Copilot
AI
Nov 28, 2025
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 sendWriteMessages method is called while holding the mutex lock in Run() (line 203), but the method itself doesn't acquire or expect the lock to be held. It accesses shared state like ws.writtenMessagesCache without synchronization. Either the method should document that it must be called with the lock held, or it should acquire the lock internally. Given that it's only called from within Run() while the lock is held, adding a comment documenting this requirement would clarify the locking contract.
| } | ||
|
|
||
| // validateChains validates the instance and returns the set of native chains. | ||
| func validateChains(instance compose.Instance, erChainID compose.ChainID) (nativeChains map[compose.ChainID]struct{}, err error) { |
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.
| func validateChains(instance compose.Instance, erChainID compose.ChainID) (nativeChains map[compose.ChainID]struct{}, err error) { | |
| func validateNativeChains(instance compose.Instance, erChainID compose.ChainID) (nativeChains map[compose.ChainID]struct{}, err error) { |
Also maybe it is nice to to support multiple ERs?
But good enough for now
GalRogozinski
left a comment
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.
Good job!
For some of my comments you can see what Engineering thinks
| ) | ||
|
|
||
| type publisherInstance struct { | ||
| mu sync.Mutex |
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.
why aren't we using RW mutex from the get go?
| r.logger.Info(). | ||
| Uint64("chain_id", uint64(sender)). | ||
| Bool("vote", vote). | ||
| Msg("Ignoring vote because not waiting anymore") | ||
| return nil | ||
| } | ||
|
|
||
| // Ensure no duplicates | ||
| if _, exists := r.votes[sender]; exists { | ||
| r.logger.Info(). | ||
| Uint64("chain_id", uint64(sender)). | ||
| Bool("vote", vote). | ||
| Msg("Ignoring duplicated vote") | ||
| return ErrDuplicatedVote | ||
| } | ||
|
|
||
| // Ensure it's a native chain | ||
| if _, ok := r.nativeChains[sender]; !ok { | ||
| r.logger.Info(). | ||
| Uint64("chain_id", uint64(sender)). | ||
| Bool("vote", vote). | ||
| Msg("Ignoring vote from non-native chain") | ||
| return ErrVoteSenderNotNativeChain | ||
| } |
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.
just wondering if those shouldn't be WARN level because they are unaccepted behavior
| PutInboxMessages: append([]scp.MailboxMessage(nil), ws.putInboxMessages...), | ||
| PutOutboxMessages: append([]scp.MailboxMessage(nil), ws.writePrePopulationMessages...), | ||
| Transactions: ws.txs, |
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.
shouldn't you do all the reads inside a lock?
| func (ws *wsInstance) attemptERCall() { | ||
| ws.mu.Lock() | ||
|
|
||
| // If not waiting for native decided, return | ||
| if ws.state != WSStateWaitingNativeDecided { | ||
| ws.mu.Unlock() | ||
| return | ||
| } |
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.
why not use defer mu.unlock?
| ws.mu.Unlock() | ||
| if includedAny { |
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.
you don't use defer mu.unlock becuase of the log?
|
|
||
| // consumeReceivedMailboxMessagesAndSimulate checks if any expected read mailbox messages have been received | ||
| // If so, remove from the lists, and call run to simulate | ||
| func (ws *wsInstance) consumeReceivedMailboxMessagesAndSimulate() error { |
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.
I don't see you simulating here...
How will this be used as lib? Maybe pass a simulation function?
| ws.logger.Info(). | ||
| Bool("received_native_decided", decided). | ||
| Bool("stored_native_decision", *ws.nativeDecided). | ||
| Msg("Ignoring native decided because already received") | ||
| ws.mu.Unlock() | ||
| return ErrDuplicateNativeDecided | ||
| } | ||
|
|
||
| if ws.state == WSStateDone { | ||
| ws.logger.Info(). | ||
| Bool("received_native_decided", decided). | ||
| Str("stored_decision", ws.decisionState.String()). | ||
| Msg("Ignoring native decided message because already done") | ||
|
|
||
| ws.mu.Unlock() | ||
| return nil |
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.
Warn?
GalRogozinski
left a comment
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.
AI review:
Potential deadlocks/back-pressure while holding mutexes during outbound network calls. In several code paths the state lock stays held while invoking network/ER clients: publisher.ProcessVote sends SendDecided/SendNativeDecided under lock (compose/cdcp/publisher.go:174-185), publisher.ProcessWSDecided sends SendDecided under lock (compose/cdcp/publisher.go:245-265), and wsInstance sends mailbox/WSDecided messages while locked (compose/cdcp/wrapped_sequencer.go:194-198, 235-252, 294-309, 422-428). If the transport layer is slow or re-enters callbacks, the state mutex can block progress or deadlock. Consider releasing locks before making external calls or deferring them outside the critical section.
Duplicate native decisions are treated as hard errors even after the instance completes (compose/cdcp/wrapped_sequencer.go:381-388). Networks often retry messages; returning ErrDuplicateNativeDecided after finishing could surface spurious errors upstream instead of being idempotent. Consider ignoring duplicates once a final decision is reached.
Overview
This PR introduces the CDCP protocol for composing with external rollups.
It includes a markdown for explaining the protocol in prose, and a minimal spec implementation along with unit tests.