SBCP Gherkin Tests #12
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces comprehensive Gherkin test specifications for the SBCP (Superblock Consensus Protocol), defining behavioral requirements for both sequencer and publisher components. The tests cover critical protocol operations including settlement events, rollback handling, period management, instance scheduling, and block lifecycle management.
Key Changes
- Adds sequencer test scenarios covering L1 settlement events, rollback handling, period transitions, instance management, and block policy enforcement
- Adds publisher test scenarios for settlement proof aggregation, period management, and cross-chain instance scheduling
- Defines expected error conditions and state transitions for the SBCP protocol
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| gherkin/sbcp/sequencer/settlement_events.feature | Defines sequencer behavior for processing L1 finalized superblock events |
| gherkin/sbcp/sequencer/rollback.feature | Specifies rollback acceptance/rejection logic and state cleanup procedures |
| gherkin/sbcp/sequencer/periods.feature | Tests period transitions and settlement pipeline triggering conditions |
| gherkin/sbcp/sequencer/instances.feature | Validates composability instance lifecycle and transaction locking behavior |
| gherkin/sbcp/sequencer/block_policy.feature | Ensures sequential block building and instance interaction constraints |
| gherkin/sbcp/publisher/proofs.feature | Tests proof collection, aggregation, and superblock publication workflow |
| gherkin/sbcp/publisher/periods.feature | Validates period management and proof window constraint enforcement |
| gherkin/sbcp/publisher/instances.feature | Specifies instance scheduling with disjoint participant sets and sequence numbering |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Feature: Publisher Settlement Proofs | ||
| Once a period ends, sequencers produce zk proofs for their respective chains and | ||
| submit them to the SP | ||
| The SP collects the proofs a sequential manner, respecting the order of superblocks. |
There was a problem hiding this comment.
Missing article: "a sequential manner" should be "in a sequential manner". The preposition "in" is required for grammatical correctness.
| The SP collects the proofs a sequential manner, respecting the order of superblocks. | |
| The SP collects the proofs in a sequential manner, respecting the order of superblocks. |
| The SP initiates multiple composability instances during a period. | ||
| It should maintain a queue with incoming XTRequests from users | ||
| (either sent directly from the user, or relayed by sequencers). | ||
| It follows the constrain that no chain can participate in more than one |
There was a problem hiding this comment.
Grammar error: "It follows the constrain" should be "It follows the constraint". The word "constraint" is a noun, while "constrain" is a verb.
| It follows the constrain that no chain can participate in more than one | |
| It follows the constraint that no chain can participate in more than one |
| """ | ||
| Then the attempt should fail with error: | ||
| """ | ||
| can not start any instance |
There was a problem hiding this comment.
The error message "can not start any instance" is unclear. Consider a more descriptive error message like "cannot start instance: chain already participating in active instance" or "overlapping active participants" to better explain why the operation failed.
| can not start any instance | |
| cannot start instance: overlapping active participants |
| """ | ||
| Then the attempt should fail with error: | ||
| """ | ||
| invalid request |
There was a problem hiding this comment.
The error message "invalid request" is too generic and unhelpful. It should specify what makes the request invalid, e.g., "XTRequest must span at least two chains" to provide actionable feedback to developers.
| invalid request | |
| XTRequest must span at least two chains |
| """ | ||
| Then the attempt should fail with error: | ||
| """ | ||
| can not start any instance |
There was a problem hiding this comment.
Inconsistent terminology: "can not" should be "cannot" (one word). This is the standard spelling in formal writing and matches programming error message conventions.
| can not start any instance | |
| cannot start any instance |
| | period_id | 21 | | ||
| | target_superblock | 12 | | ||
| Then no proof request should be sent to the prover | ||
| And the sequencer "A" should send its latest proof to the SP |
There was a problem hiding this comment.
Incomplete documentation: The scenario description states "Includes no header when no block was sealed in the previous period" but the expected behavior (line 79-80) is ambiguous. It's unclear what "send its latest proof to the SP" means when there's no sealed block. Consider clarifying what proof is sent or if this means sending an empty/null proof.
| And the sequencer "A" should send its latest proof to the SP | |
| And no proof should be sent to the SP |
| When a new period starts, the sequencer updates those counters and immediately starts the settlement | ||
| pipeline for the previous period if no block is pending. Otherwise, it waits until the pending block is sealed | ||
| before requesting proofs and forwarding them to the publisher. | ||
| Thus, the general rule is that: once a the last block for a previous period is sealed, the settlement pipeline |
There was a problem hiding this comment.
Grammar error: "once a the" should be "once the". Remove the extra article "a".
| Thus, the general rule is that: once a the last block for a previous period is sealed, the settlement pipeline | |
| Thus, the general rule is that: once the last block for a previous period is sealed, the settlement pipeline |
GalRogozinski
left a comment
GalRogozinski
left a comment
There was a problem hiding this comment.
Very good!
Also apply tag suggestion from previous PR
|
|
||
| @publisher @sbcp @instances | ||
| Scenario: Rejects XTRequests that overlap with active participants | ||
| Given chains "1,2" are active |
There was a problem hiding this comment.
maybe it is best to not have the shortcut and activate for a better test?
| In case a StartPeriod is called but a proof window exceedance is detected, | ||
| the publisher broadcasts a Rollback, resetting the network state to the last finalized superblock, |
There was a problem hiding this comment.
we need to discuss how much slack we give the prover
| @publisher @sbcp @proofs | ||
| Scenario: Duplicate proofs from the same chain are ignored | ||
| Given SP received Proof from chain "1" for: | ||
| | field | value | | ||
| | period_id | 9 | | ||
| | superblock_number | 6 | | ||
| | proof_data | 0xaa | | ||
| When SP receives Proof from chain "1" for: | ||
| | field | value | | ||
| | period_id | 9 | | ||
| | superblock_number | 6 | | ||
| | proof_data | 0xbb | | ||
| Then the proof should be ignored |
There was a problem hiding this comment.
hmmm
don't understand the value of this test
| | superblock | 5 | | ||
| | hash | 0x999 | | ||
| And chains "1,2" should be marked as inactive | ||
| And the target superblock should reset to "6" |
There was a problem hiding this comment.
wait... I am confused
the target superblock is what we are trying to prove?
Then nothing makes sense... why currently we target 7?
| Then the attempt should fail with error: | ||
| """ | ||
| local transactions are disabled while an instance is active | ||
| """ |
There was a problem hiding this comment.
shouldn't it be queued?
| @sequencer @sbcp @periods | ||
| Scenario: Triggers settlement immediately when StartPeriod arrives and no block is pending | ||
| Given the sequencer "A" is at period ID "20" targeting superblock "11" | ||
| And there is no open block for sequencer "A" |
| for that period is triggered. | ||
|
|
||
| Background: | ||
| Given there is a chain "1" with sequencer "A" |
There was a problem hiding this comment.
aren't we missing current period and superblock?
the first test needs it, no?
| | state_root | 0x222 | | ||
|
|
||
| @sequencer @sbcp @periods | ||
| Scenario: Includes no header when no block was sealed in the previous period |
| And the sequencer "A" sealed blocks "105,106" for period "10" targeting superblock "5" | ||
| And the sequencer "A" has an open block "107" for period "11" targeting superblock "6" | ||
| And the sequencer "A" has an active instance "0xdef" | ||
| When the sequencer "A" receives Rollback: |
There was a problem hiding this comment.
| When the sequencer "A" receives Rollback: | |
| When the sequencer "A" receives Rollback from publisher: |
| @@ -0,0 +1,59 @@ | |||
| Feature: Publisher Period Management | |||
There was a problem hiding this comment.
publisher is sending rollback messages, right?
Maybe we need tio add tests for this
Overview
This PR adds gherkin tests for the SBCP protocol.