SCP Gherkin Tests #10

MatheusFranco99 · 2025-11-28T16:23:24Z

Overview

This PR adds gherkin tests files for the SCP protocol.

GalRogozinski

Good job!

I like sequencer_2 better because it is more readable simply.

I think we can have maybe tags like: sequencer, publisher, contracts, or integration (we need to decide with Ayaz if we should have integration tests as spec tests)

GalRogozinski · 2025-12-01T15:38:31Z

gherkin/scp/sequencer_2.feature

+    Given there is a chain "1" with sequencer "A"
+    And there is a chain "2" with sequencer "B"
+
+  @start-instance


My thoughts were actually to divide the sequencer into files: start_instance.feature, timeout.feature, and etc.

Then we in the Feature section we have a place to document each feature.
So the tests become easy way to learn how each feature of the spec works.

GalRogozinski · 2025-12-01T15:46:02Z

gherkin/scp/sequencer_2.feature

+Feature: Sequencer
+  Once the sequencer receives a StartInstance message it filters its own transactions, sets a timer,
+  and begins simulating with a mailbox tracer. It exchanges messages with peer sequencers and,
+  after the simulation completes, it votes and waits for a decision.


note it is also possible to put tags here

GalRogozinski · 2025-12-01T15:49:24Z

gherkin/scp/sequencer_2.feature

+      | returns success                         | true  |
+      | returns an error other than "Read miss" | false |


nit: maybe the word "returns" is redundant

GalRogozinski · 2025-12-01T15:54:04Z

gherkin/scp/sequencer_2.feature

+      | returns success                         | true  |
+      | returns an error other than "Read miss" | false |
+
+  @mailbox @simulation


you are mocking the simulation here, right?
So I disagree with this tag

GalRogozinski

Also, maybe we should use error codes?
Or maybe it is fine if we use meaningful constants but not enforce text

…msgs

Copilot

Pull request overview

This PR introduces comprehensive Gherkin test specifications for the Shared Consensus Protocol (SCP), covering both sequencer and publisher behavior across multiple scenarios including instance lifecycle, voting, timeouts, simulations, and decision handling.

Key Changes:

Added 7 Gherkin feature files documenting SCP protocol behavior
Defined test scenarios for sequencer operations (start instance, timeout, simulation, decision)
Defined test scenarios for publisher operations (start instance, votes, timeout)

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
gherkin/scp/sequencer/timeout.feature	Defines sequencer timeout behavior for voting and rejection scenarios
gherkin/scp/sequencer/start_instance.feature	Specifies sequencer instance initialization, transaction filtering, and timer startup
gherkin/scp/sequencer/simulation.feature	Covers transaction simulation outcomes, mailbox message handling, and read miss scenarios
gherkin/scp/sequencer/decision.feature	Documents instance finalization, decision processing, and invalid state handling
gherkin/scp/publisher/votes.feature	Specifies vote collection, validation, and consensus decision logic
gherkin/scp/publisher/timeout.feature	Defines publisher timeout behavior for instance rejection
gherkin/scp/publisher/start_instance.feature	Documents publisher instance initialization and StartInstance message broadcasting

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-05T16:46:26Z

gherkin/scp/sequencer/decision.feature

+      | instance_id | 0x1   |
+      | chain_id    | 1     |
+      | vote        | true  |
+    When sequencer "A" receives Decided for instance "0x1" with decision <decision>


The <decision> placeholder in the When clause is not properly formatted as a quoted value. Unlike other Scenario Outlines in these feature files, the decision value should be enclosed in quotes for consistency. Change to: When sequencer "A" receives Decided for instance "0x1" with decision "<decision>"

Suggested change

When sequencer "A" receives Decided for instance "0x1" with decision <decision>

When sequencer "A" receives Decided for instance "0x1" with decision "<decision>"

GalRogozinski

Very nice!

I think maybe we should have happy-flow (or sanity) tags

GalRogozinski · 2025-12-08T13:26:55Z

gherkin/scp/publisher/timeout.feature

+
+  @publisher @scp @timeout
+  Scenario: Rejects instance when the timer expires before all votes are received
+    Given the shared publisher "SP" started an instance:


Should be same as above, unless you want this to be glued to different code?

Suggested change

Given the shared publisher "SP" started an instance:

Given the shared publisher "SP" starts an instance:

GalRogozinski · 2025-12-08T13:27:27Z

gherkin/scp/publisher/timeout.feature

+
+  @publisher @scp @timeout
+  Scenario Outline: Ignores timer expiry after a decision has been published
+    Given the shared publisher "SP" started an instance:


Suggested change

Given the shared publisher "SP" started an instance:

Given the shared publisher "SP" starts an instance:

GalRogozinski · 2025-12-08T13:29:37Z

gherkin/scp/publisher/timeout.feature

+    And there is a chain "1" with sequencer "A"
+    And there is a chain "2" with sequencer "B"
+
+  @publisher @scp @timeout


is it redundant to have @timeout if you can just run a single feature file?
Maybe it isn't if you will have timeout tests outside the file

You can argue that it adds clutter but adds convenience...
We can ask @ayaz-ssvlabs if he likes the @timeout for cli commands

GalRogozinski · 2025-12-08T13:33:12Z

gherkin/scp/publisher/timeout.feature

+    And there is a chain "2" with sequencer "B"
+
+  @publisher @scp @timeout
+  Scenario: Rejects instance when the timer expires before all votes are received


maybe also add "before any votes" scenario

GalRogozinski · 2025-12-08T13:43:45Z

gherkin/scp/publisher/votes.feature

+      | instance_id | 0x3   |
+      | chain_id    | 2     |
+      | vote        | true  |
+    Then the shared publisher "SP" should publish Decided with:


Suggested change

Then the shared publisher "SP" should publish Decided with:

Then the shared publisher "SP" publishes Decided with:

You don't have to take this.
I just think it reads better in case the step will be also used as an AND step.
But only some humans might care

GalRogozinski · 2025-12-08T14:36:21Z

gherkin/scp/sequencer/simulation.feature

+    Then sequencer "A" should publish Vote with:
+      | field       | value  |
+      | instance_id | 0x1    |
+      | chain_id    | 1      |
+      | vote        | <vote> |
+
+    Examples:
+      | result                                  | vote  |
+      | returns success                         | true  |
+      | returns an error other than "Read miss" | false |


Good test
Maybe add in the end "AND no additional MailboxMessage should be forwarded"

GalRogozinski · 2025-12-08T14:37:48Z

gherkin/scp/sequencer/simulation.feature

+      | source            | 0xaaa      |
+      | receiver          | 0xbbb      |
+      | session_id        | 0x777      |
+      | label             | TRANSFER   |


do we have this label?

GalRogozinski · 2025-12-08T14:44:07Z

gherkin/scp/sequencer/simulation.feature

+        1: [tx1]
+        2: [tx2]
+      """
+    And sequencer "A" has already forwarded a mailbox message with:


This is less reusable...
consult with @ayaz-ssvlabs if he wants the clutter

GalRogozinski · 2025-12-08T14:56:19Z

gherkin/scp/sequencer/start_instance.feature

+  that belong to its own chain for simulation.
+  In case there are no transactions for its own chain, the sequencer should reject the instance immediately
+  as it shouldn't have received the message and this consists of an invalid protocol state.
+  After startup, the sequencer should immediately start simulating the transactions for the instance.


Suggested change

After startup, the sequencer should immediately start simulating the transactions for the instance.

After instance start, the sequencer should immediately start simulating the transactions for the instance.

GalRogozinski · 2025-12-08T15:02:03Z

gherkin/scp/sequencer/simulation.feature

+      | expected_state     | storage_result                                                                                                 | simulation_effect                               |
+      | has not stored     | the message is appended to the pending mailbox queue for instance "0x1"                                      | sequencer "A" should not start a new simulation |
+      | has already stored | the message is removed from the expected set and pending queue, then inserted into the inbox and a mailbox.putInbox transaction is added | sequencer "A" should start a new simulation     |


MatheusFranco99 added 2 commits November 28, 2025 16:08

add gherkin for SCP sequencer

5e17c8f

fix sender typo, "should" keyword, "_" vote and no vote assertion

6e1bf64

MatheusFranco99 requested a review from GalRogozinski November 28, 2025 16:23

MatheusFranco99 self-assigned this Nov 28, 2025

add 2nd file with different style for comparison

767f18c

GalRogozinski reviewed Dec 1, 2025

View reviewed changes

MatheusFranco99 added 5 commits December 5, 2025 16:09

split up in several feature files

16a55f4

fix tags, feature names, and typos

6e030f5

add test case for already sent written msg, and for multiple written …

c41fd02

…msgs

add scp and sequencer tags

fea6de7

add publisher tests

f4c8d01

MatheusFranco99 requested a review from GalRogozinski December 5, 2025 16:43

MatheusFranco99 marked this pull request as ready for review December 5, 2025 16:43

Copilot AI review requested due to automatic review settings December 5, 2025 16:43

Copilot started reviewing on behalf of MatheusFranco99 December 5, 2025 16:44 View session

Copilot finished reviewing on behalf of MatheusFranco99 December 5, 2025 16:45

Copilot AI reviewed Dec 5, 2025

View reviewed changes

GalRogozinski reviewed Dec 8, 2025

View reviewed changes

		\| returns success \| true \|
		\| returns an error other than "Read miss" \| false \|

	When sequencer "A" receives Decided for instance "0x1" with decision <decision>
	When sequencer "A" receives Decided for instance "0x1" with decision "<decision>"

	Given the shared publisher "SP" started an instance:
	Given the shared publisher "SP" starts an instance:

	Then the shared publisher "SP" should publish Decided with:
	Then the shared publisher "SP" publishes Decided with:

	After startup, the sequencer should immediately start simulating the transactions for the instance.
	After instance start, the sequencer should immediately start simulating the transactions for the instance.

SCP Gherkin Tests #10

Are you sure you want to change the base?

SCP Gherkin Tests #10

Uh oh!

Conversation

MatheusFranco99 commented Nov 28, 2025

Overview

Uh oh!

GalRogozinski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GalRogozinski left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

GalRogozinski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants