Skip to content

Documentation for qbft, ssv, and types.#605

Open
jose-ssv-labs wants to merge 8 commits into
mainfrom
documentation
Open

Documentation for qbft, ssv, and types.#605
jose-ssv-labs wants to merge 8 commits into
mainfrom
documentation

Conversation

@jose-ssv-labs
Copy link
Copy Markdown

@jose-ssv-labs jose-ssv-labs commented Mar 12, 2026

Replaces #496

MatheusFranco99 and others added 8 commits January 1, 2026 11:34
* include epoch in round robin proposer calculation

* align tests

* add explanation comment on epoch variability
* add boole domain type

* add alan fork epoch

* use max uint64 as fork placeholder
* draft

* fix aggregator committee - sync committee contribution only test

* bug fix

* bug fix

* add 20-validator test for sync committee aggregator

* add test for aggregator and sync committee contribution duties

* lint

* merge with main

* missing tests

* support fulu in GetAggregateAndProofs

* fix fulu aggregate and proof

* fix missing fulu cases

* fix leftovers

* fix passing slot in contributionProofMsg

* Revert "fix passing slot in contributionProofMsg"

This reverts commit 226659d.

* Aggregator Committee - Drop previous runners and align tests (#592)

* remove aggregatorCommittee from BeaconRole

* remove aggregator and scc roles

* remove agg and scc from ValidatorConsensusData

* generate ssz encoding

* align testingutils to remove reference to agg and scc alone

* add agg committee consensus data tests (and remove agg and scc from validator consensus data tests)

* generate types JSON tests

* drop agg and scc runners; fix agg committee runner issue

* align testingutils for agg committee tests

* value check tests

* preconsensus tests

* post consensus tests

* duties tests

* runner construction tests

* consensus tests

* happy flow test

* dutyexe tests

* add test docs; fix msg processing test

* add all tests

* generate JSON tests

* apply suggestions

* Aggregator Committee Mixed Duties Tests (#593)

* remove aggregatorCommittee from BeaconRole

* remove aggregator and scc roles

* remove agg and scc from ValidatorConsensusData

* generate ssz encoding

* align testingutils to remove reference to agg and scc alone

* add agg committee consensus data tests (and remove agg and scc from validator consensus data tests)

* generate types JSON tests

* drop agg and scc runners; fix agg committee runner issue

* align testingutils for agg committee tests

* value check tests

* preconsensus tests

* post consensus tests

* duties tests

* runner construction tests

* consensus tests

* happy flow test

* dutyexe tests

* add test docs; fix msg processing test

* add all tests

* generate JSON tests

* drop weird json tests in unintended directory

* add mixed agg+scc pre-consensus tests

* generate JSON tests

* add error code

* increase number of Contributors

* add  post-consensus mixed agg committee tests

* generate JSON tests

* fixed remaining mixed tests

* generate JSON tests

* fix lint (remove unused functions)

* avoid in-place sorting

* update ssz hash tags

* apply suggestions

* solve TODOs

* revert deleted Alan runner roles

* revert deleted ValidatorConsensusData methods

* generate tests

* Revert "generate tests"

This reverts commit 441a53a.

* Revert "revert deleted ValidatorConsensusData methods"

This reverts commit cb04d43.

* Revert "revert deleted Alan runner roles"

This reverts commit 69a6a65.

* Agg comm improvements (#594)

* remove aggregatorCommittee from BeaconRole

* remove aggregator and scc roles

* remove agg and scc from ValidatorConsensusData

* generate ssz encoding

* align testingutils to remove reference to agg and scc alone

* add agg committee consensus data tests (and remove agg and scc from validator consensus data tests)

* generate types JSON tests

* drop agg and scc runners; fix agg committee runner issue

* align testingutils for agg committee tests

* value check tests

* preconsensus tests

* post consensus tests

* duties tests

* runner construction tests

* consensus tests

* happy flow test

* dutyexe tests

* add test docs; fix msg processing test

* add all tests

* generate JSON tests

* drop weird json tests in unintended directory

* add mixed agg+scc pre-consensus tests

* generate JSON tests

* add error code

* increase number of Contributors

* add  post-consensus mixed agg committee tests

* generate JSON tests

* add agg committee duty validation; add psgi msg sorting;

* add sorting and duty validation tests

* generate JSON tests

* fixed remaining mixed tests

* generate JSON tests

* fix lint (remove unused functions)

* change AggregatorCommitteeConsensusData to reduce duplicated data overhead

* align tests

* generate JSON tests

* fix maximum ssz sizes

* avoid in-place sorting

* add test docs

* remove unused test docs

* fix maximum-size tests

* generate JSON tests

* fix test: duty with diff slots

* add max size test for aggCommCD

* add size tests for phase0 and electra attestations; fix ssz max size for attestation in AggCommCD

* fix lint issues

* fix test dir (no multiple duty)

* fix versions data

* maximum duty possible test

* fix lint

* apply suggestions (remove sorting feature; remove unused errors; use subnet for contribution)

* clarify validator sync committee index usage

* change subnetID computation to avoid errors

* generate JSON tests

* tests for: invalid quorum; invalid quorum then valid quorum;

* generate JSON tests

* generate JSON tests with new error numbers

* remove deprecated partial signature types

* generate JSON tests

* generate SSZ files

* value check att decoding check

* generate JSON tests

* apply suggestions

* Rename ValidatorConsensusData to ProposerConsensusData (#596)

* remove aggregatorCommittee from BeaconRole

* remove aggregator and scc roles

* remove agg and scc from ValidatorConsensusData

* generate ssz encoding

* align testingutils to remove reference to agg and scc alone

* add agg committee consensus data tests (and remove agg and scc from validator consensus data tests)

* generate types JSON tests

* drop agg and scc runners; fix agg committee runner issue

* align testingutils for agg committee tests

* value check tests

* preconsensus tests

* post consensus tests

* duties tests

* runner construction tests

* consensus tests

* happy flow test

* dutyexe tests

* add test docs; fix msg processing test

* add all tests

* generate JSON tests

* drop weird json tests in unintended directory

* add mixed agg+scc pre-consensus tests

* generate JSON tests

* add error code

* increase number of Contributors

* add  post-consensus mixed agg committee tests

* generate JSON tests

* add agg committee duty validation; add psgi msg sorting;

* add sorting and duty validation tests

* generate JSON tests

* fixed remaining mixed tests

* generate JSON tests

* fix lint (remove unused functions)

* change AggregatorCommitteeConsensusData to reduce duplicated data overhead

* align tests

* generate JSON tests

* fix maximum ssz sizes

* avoid in-place sorting

* add test docs

* remove unused test docs

* fix maximum-size tests

* generate JSON tests

* fix test: duty with diff slots

* add max size test for aggCommCD

* add size tests for phase0 and electra attestations; fix ssz max size for attestation in AggCommCD

* fix lint issues

* fix test dir (no multiple duty)

* fix versions data

* maximum duty possible test

* fix lint

* apply suggestions (remove sorting feature; remove unused errors; use subnet for contribution)

* clarify validator sync committee index usage

* change subnetID computation to avoid errors

* generate JSON tests

* tests for: invalid quorum; invalid quorum then valid quorum;

* generate JSON tests

* generate JSON tests with new error numbers

* rename ValidatorConsensusData -> ProposerConsensusData

* align tests

* generate JSON tests

* fix renaming on merge

* make runner role explicit

* generate JSON tests

* Aggregator Committee - Fix committee runners management (#597)

* remove aggregatorCommittee from BeaconRole

* remove aggregator and scc roles

* remove agg and scc from ValidatorConsensusData

* generate ssz encoding

* align testingutils to remove reference to agg and scc alone

* add agg committee consensus data tests (and remove agg and scc from validator consensus data tests)

* generate types JSON tests

* drop agg and scc runners; fix agg committee runner issue

* align testingutils for agg committee tests

* value check tests

* preconsensus tests

* post consensus tests

* duties tests

* runner construction tests

* consensus tests

* happy flow test

* dutyexe tests

* add test docs; fix msg processing test

* add all tests

* generate JSON tests

* drop weird json tests in unintended directory

* add mixed agg+scc pre-consensus tests

* generate JSON tests

* add error code

* increase number of Contributors

* add  post-consensus mixed agg committee tests

* generate JSON tests

* add agg committee duty validation; add psgi msg sorting;

* add sorting and duty validation tests

* generate JSON tests

* fixed remaining mixed tests

* generate JSON tests

* fix lint (remove unused functions)

* change AggregatorCommitteeConsensusData to reduce duplicated data overhead

* align tests

* generate JSON tests

* fix maximum ssz sizes

* avoid in-place sorting

* add test docs

* remove unused test docs

* fix maximum-size tests

* generate JSON tests

* fix test: duty with diff slots

* add max size test for aggCommCD

* add size tests for phase0 and electra attestations; fix ssz max size for attestation in AggCommCD

* fix lint issues

* fix test dir (no multiple duty)

* fix versions data

* maximum duty possible test

* fix lint

* apply suggestions (remove sorting feature; remove unused errors; use subnet for contribution)

* clarify validator sync committee index usage

* change subnetID computation to avoid errors

* generate JSON tests

* tests for: invalid quorum; invalid quorum then valid quorum;

* generate JSON tests

* generate JSON tests with new error numbers

* remove deprecated partial signature types

* generate JSON tests

* generate SSZ files

* fix committee to have agg and comm runners

* align testing utils. Fix Committee constructor to a common one, and align tests execution

* add test for comm + agg comm duties in the same slot

* generate JSON tests

* remove unused function

* add tests for error cases in committee

* add test for mixed duties for multiple slots

* remove unused parameter

* set fork-persistent values for psig types

* generate JSON tests

* set max ssz sizes exportable

---------

Co-authored-by: Alan <alan@ssvlabs.io>
Co-authored-by: Nikita Kryuchkov <nkryuchkov10@gmail.com>
Co-authored-by: MatheusFranco99 <48058141+MatheusFranco99@users.noreply.github.com>
* sign once per validator subnet

* add test for duplicated subnets

* generate JSON tests
…vement (#604)

* fix: let agg comm runner wait for more messages in pre-consensus when no aggregator is selected

* tests: add and align tests (not selected, selected, and all msgs received)

* generate JSON tests

* Merge branch 'boole' into agg-comm-pre-consensus-termination

* refactor: rename preConsensusSelectionsTested to preConsensusDutiesCheckedForSelection

* test: ensure duty termination when all duties are checked for selection

* feat: ignore pre-consensus msg if already advanced to consensus

* test: add ignore msg test and align other tests

* generate JSON tests

* test: extend msg processing spectest with expected QBFT proposal

* test: generate JSON tests

* fix: improve condition checks, test checks

* fix: "doens't" -> "does not"

* fix: do not return error on pre-consensus msg processing after consensus started

* fix: return error if any
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 12, 2026

Greptile Summary

This PR adds comprehensive documentation for the qbft, ssv, and types modules, including new README.md files with architectural overviews, component descriptions, code snippets, and supporting diagrams (class diagrams, BPMN flow, duty flow). The documentation is a valuable addition for understanding the codebase, but several issues need to be addressed before merging.

Key findings:

  • Factual error in types/docs/README.md: the section "ValidatorConsensusData and BeaconVote" incorrectly refers to the type as `BeaconDuty` (line 111) instead of the correct `BeaconVote`, which could mislead readers looking for that type.
  • Incorrect description in types/docs/README.md: `OperatorSigner` is described as a "structure" when it is an interface, inconsistent with how `BeaconSigner` is described in the same section.
  • Missing word in types/docs/README.md: "the Data should an encoded" is missing "be".
  • Typo in section heading in types/docs/README.md: MessagID should be MessageID.
  • Multiple typos in ssv/README.md: "thhe", "Pre-Consenus", "require a the", "Luckly", "over an a prior".
  • Grammar errors in qbft/docs/README.md: "All of this fields" and duplicate "the the QBFT".

Confidence Score: 4/5

  • This is a documentation-only PR with no code changes; safe to merge after fixing the identified typos and the one factual error (BeaconDutyBeaconVote).
  • No production code is modified. The content is generally accurate and well-structured. Score is 4 rather than 5 due to a factual error that names a non-existent type (BeaconDuty) and an incorrect interface/structure characterisation for OperatorSigner, both of which could mislead developers using the docs as a reference.
  • types/docs/README.md requires the most attention due to the factual type-name error and the incorrect interface description.

Important Files Changed

Filename Overview
types/docs/README.md New documentation for the types module — contains a factual error (refers to BeaconDuty instead of BeaconVote), a missing word ("should an encoded"), a section heading typo (MessagID), and incorrectly describes OperatorSigner as a "structure" rather than an interface.
ssv/README.md New top-level SSV module documentation — good overview of the system model and duty execution flow, but contains several typos: "thhe", "Pre-Consenus", "require a the", "Luckly", and "over an a prior".
qbft/README.md Updated QBFT README with a Mermaid flowchart overview and BPMN diagram appendix — well-structured but minor HTML attribute comma syntax issue in the img tag.
qbft/docs/README.md New QBFT module documentation — clear descriptions of key structs and their relationships; two minor textual errors ("All of this fields" and duplicate "the the") found.
ssv/docs/README.md New SSV module documentation covering Network, BeaconNode, Runner, DutyRunners, Validator, and Committee — content is accurate and well-written with no notable issues.

Sequence Diagram

sequenceDiagram
    participant O as Operator
    participant R as Runner
    participant BN as BeaconNode
    participant QBFT as QBFT Controller
    participant N as P2P Network

    O->>R: StartDuty()
    alt Has Pre-Consensus phase (Proposer / Aggregator / SyncCommitteeContribution)
        R->>N: Broadcast PartialSignatureMessage (Pre-Consensus)
        N-->>R: Receive threshold of PartialSignatureMessages
        R->>BN: Fetch duty data (e.g. BeaconBlock, AggregateAndProof)
    end

    R->>QBFT: StartInstance(height, value)
    QBFT->>N: Broadcast Proposal (if leader)
    N-->>QBFT: Receive Proposal
    QBFT->>N: Broadcast Prepare
    N-->>QBFT: Receive quorum of Prepares
    QBFT->>N: Broadcast Commit
    N-->>QBFT: Receive quorum of Commits
    QBFT->>N: Broadcast Decided (aggregated Commits)

    R->>N: Broadcast PartialSignatureMessage (Post-Consensus)
    N-->>R: Receive threshold of PartialSignatureMessages
    R->>BN: Submit reconstructed validator signature
Loading

Last reviewed commit: 4ab1145

Comment thread types/docs/README.md

## MessageID

`MessagID` is an identifier that is composed by:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Typo in section heading: MessagID should be MessageID

The heading uses MessagID (missing the e) instead of the correct MessageID.

Suggested change
`MessagID` is an identifier that is composed by:
## MessageID

Comment thread types/docs/README.md

## ValidatorConsensusData and BeaconVote

But what objects do the operators propose in the consensus protocol? It can be a `ValidatorConsensusData` or a `BeaconDuty`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Incorrect type name: BeaconDuty should be BeaconVote

This section is titled "ValidatorConsensusData and BeaconVote", but line 111 refers to the type as BeaconDuty — a type that does not exist. The correct name is BeaconVote.

Suggested change
But what objects do the operators propose in the consensus protocol? It can be a `ValidatorConsensusData` or a `BeaconDuty`.
The `ValidatorConsensusData` holds the associated duty, a blockchain version (e.g. as Phase0, Altair, Bellatrix, Capella, and Deneb), and an encoded object related to the duty (e.g. an AttestationData, a BeaconBlock). This structure is used for all except the `CommitteeDuty`.
The `BeaconVote` contains a blockchain head vote (LMD GHOST vote) and a source and target votes (Casper FFG). It's used for the `CommitteeDuty`.

Comment thread types/docs/README.md
}
```

If the `MsgType` is of consensus type, then the `Data` should an encoded `qbft.Message` object. Else (if it's of partial signature type), it should be an encoded [`PartialSignatureMessages`](#partialsignaturemessages) object.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing word: "should be an encoded"

The sentence reads "the Data should an encoded qbft.Message object" — it is missing the word "be".

Suggested change
If the `MsgType` is of consensus type, then the `Data` should an encoded `qbft.Message` object. Else (if it's of partial signature type), it should be an encoded [`PartialSignatureMessages`](#partialsignaturemessages) object.
If the `MsgType` is of consensus type, then the `Data` should be an encoded `qbft.Message` object. Else (if it's of partial signature type), it should be an encoded [`PartialSignatureMessages`](#partialsignaturemessages) object.

Comment thread types/docs/README.md

## BeaconSigner and OperatorSigner

`BeaconSigner` is an interface that provides methods for creating beacon signatures. Apart from signing, it servers as a slashing protection by providing a method to check if an attestation is slashable.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Incorrect description: OperatorSigner is an interface, not a structure

The previous paragraph correctly describes BeaconSigner as an "interface". For consistency and accuracy, OperatorSigner should also be described as an "interface", not a "structure".

Suggested change
`BeaconSigner` is an interface that provides methods for creating beacon signatures. Apart from signing, it servers as a slashing protection by providing a method to check if an attestation is slashable.
The `OperatorSigner` is a similar interface but performs signing for network messages.

Comment thread ssv/README.md
Comment on lines +31 to +44
The Ethereum duties that require a the Pre-Consenus phase are the Proposer, Aggregation and Sync Committee Contribution duties. The other duties (Attestation and Sync Committee) start from step 2 of the above diagram (without needing to provide any signature). In the Pre-Consensus phase:
- the Proposer duty should construct the validator's signature over the `Epoch` number to build the `RANDAO` object.
- the Aggregator duty should construct the validator's signature over the `Slot` number to submit an `AggregateSelectionProof`.
- the Sync Committee Contributor duty should, for each committee index, construct the validator's signature over the `SyncAggregatorSelectionData` object to check if it's an aggregator.

To reconstruct the validators signature, at minimum a *threshold* amount of signatures is needed. In our case, this *threshold* is $\lfloor \frac{N+f}{2} \rfloor + 1$ (or $2f+1$ in case $N = 3f+1$).

### Duty start time

Within the duty's slot, thhe time at which the operators should start the duty depends on the duty type.

- For the Proposer duty, the protocol should be executed as soon as the slot starts (so no delay).
- For the Attestation and Sync Committee duties, the protocol should be executed after one third (1/3) of the slot duration, i.e. 4 seconds. This helps maximizing the chance of voting for the most recent proposed block.
- For the Aggregation and Sync Committee Contribution duties, the protocol should be executed after two thirds (2/3) of the slot duration, i.e. 8 seconds. This helps maximizing the collection of Attestation and Sync Committee votes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Multiple typos in this section

Several typos are present:

  • Line 31: "require a the Pre-Consenus" → should be "require the Pre-Consensus"
  • Line 40: "thhe time" → should be "the time"
  • Line 43: "maximizing the chance of voting" (minor, acceptable)
Suggested change
The Ethereum duties that require a the Pre-Consenus phase are the Proposer, Aggregation and Sync Committee Contribution duties. The other duties (Attestation and Sync Committee) start from step 2 of the above diagram (without needing to provide any signature). In the Pre-Consensus phase:
- the Proposer duty should construct the validator's signature over the `Epoch` number to build the `RANDAO` object.
- the Aggregator duty should construct the validator's signature over the `Slot` number to submit an `AggregateSelectionProof`.
- the Sync Committee Contributor duty should, for each committee index, construct the validator's signature over the `SyncAggregatorSelectionData` object to check if it's an aggregator.
To reconstruct the validators signature, at minimum a *threshold* amount of signatures is needed. In our case, this *threshold* is $\lfloor \frac{N+f}{2} \rfloor + 1$ (or $2f+1$ in case $N = 3f+1$).
### Duty start time
Within the duty's slot, thhe time at which the operators should start the duty depends on the duty type.
- For the Proposer duty, the protocol should be executed as soon as the slot starts (so no delay).
- For the Attestation and Sync Committee duties, the protocol should be executed after one third (1/3) of the slot duration, i.e. 4 seconds. This helps maximizing the chance of voting for the most recent proposed block.
- For the Aggregation and Sync Committee Contribution duties, the protocol should be executed after two thirds (2/3) of the slot duration, i.e. 8 seconds. This helps maximizing the collection of Attestation and Sync Committee votes.
The Ethereum duties that require the Pre-Consensus phase are the Proposer, Aggregation and Sync Committee Contribution duties. The other duties (Attestation and Sync Committee) start from step 2 of the above diagram (without needing to provide any signature). In the Pre-Consensus phase:
- the Proposer duty should construct the validator's signature over the `Epoch` number to build the `RANDAO` object.
- the Aggregator duty should construct the validator's signature over the `Slot` number to submit an `AggregateSelectionProof`.
- the Sync Committee Contributor duty should, for each committee index, construct the validator's signature over the `SyncAggregatorSelectionData` object to check if it's an aggregator.
To reconstruct the validators signature, at minimum a *threshold* amount of signatures is needed. In our case, this *threshold* is $\lfloor \frac{N+f}{2} \rfloor + 1$ (or $2f+1$ in case $N = 3f+1$).
### Duty start time
Within the duty's slot, the time at which the operators should start the duty depends on the duty type.
- For the Proposer duty, the protocol should be executed as soon as the slot starts (so no delay).
- For the Attestation and Sync Committee duties, the protocol should be executed after one third (1/3) of the slot duration, i.e. 4 seconds. This helps maximizing the chance of voting for the most recent proposed block.
- For the Aggregation and Sync Committee Contribution duties, the protocol should be executed after two thirds (2/3) of the slot duration, i.e. 8 seconds. This helps maximizing the collection of Attestation and Sync Committee votes.

Comment thread ssv/README.md
Comment on lines +52 to +57
Both duties are only composed by the Pre-Consensus phase since only a signature construction is needed over an a prior known data.

Regarding the duties' start time, both should start at the beginning of the slot.

## The Committee duty
Since the committee may manage several validators, many Attestation duties may need to be performed in the same slot. This represents a workload concern. Luckly, we have that all consensus executions for each attestation duty agree on the same data (the Casper FFG and LMD GHOST votes). Therefore, we can perform a single consensus execution for all validators assigned in the same slot (though a unique Post-Consensus phase is required for each).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Multiple typos in this section

  • Line 52: "over an a prior known data" → remove the extra article: "over a prior known data"
  • Line 57: "Luckly" → should be "Luckily"
Suggested change
Both duties are only composed by the Pre-Consensus phase since only a signature construction is needed over an a prior known data.
Regarding the duties' start time, both should start at the beginning of the slot.
## The Committee duty
Since the committee may manage several validators, many Attestation duties may need to be performed in the same slot. This represents a workload concern. Luckly, we have that all consensus executions for each attestation duty agree on the same data (the Casper FFG and LMD GHOST votes). Therefore, we can perform a single consensus execution for all validators assigned in the same slot (though a unique Post-Consensus phase is required for each).
Both duties are only composed by the Pre-Consensus phase since only a signature construction is needed over a prior known data.
Regarding the duties' start time, both should start at the beginning of the slot.
## The Committee duty
Since the committee may manage several validators, many Attestation duties may need to be performed in the same slot. This represents a workload concern. Luckily, we have that all consensus executions for each attestation duty agree on the same data (the Casper FFG and LMD GHOST votes). Therefore, we can perform a single consensus execution for all validators assigned in the same slot (though a unique Post-Consensus phase is required for each).

Comment thread qbft/docs/README.md
- `RoundChangeJustification`: used for the Proposal and Round-Change messages. For the Proposal message, it contains a list of Round-Change messages. For the Round-Change message, it contains a list of Prepare messages.
- `PrepareJustification`: used for the Proposal message. Includes a list of Prepare messages.

Notice that there's no field for the sender (`OperatorID`), the signature or the proposed data (that must be included in the Proposal message and may be included in the Round-Change message). All of this fields are included in the upper wrapper (`types.SignedSSVMessage`). This is useful to detach the whole proposed data from the signature. Notice, however, that the signature is still binded to the proposed data by the field `Root` which is the hash of the data.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Grammatical error: "All of this fields" should be "All of these fields"

Suggested change
Notice that there's no field for the sender (`OperatorID`), the signature or the proposed data (that must be included in the Proposal message and may be included in the Round-Change message). All of this fields are included in the upper wrapper (`types.SignedSSVMessage`). This is useful to detach the whole proposed data from the signature. Notice, however, that the signature is still binded to the proposed data by the field `Root` which is the hash of the data.
Notice that there's no field for the sender (`OperatorID`), the signature or the proposed data (that must be included in the Proposal message and may be included in the Round-Change message). All of these fields are included in the upper wrapper (`types.SignedSSVMessage`). This is useful to detach the whole proposed data from the signature. Notice, however, that the signature is still binded to the proposed data by the field `Root` which is the hash of the data.

Comment thread qbft/docs/README.md

`Instance` is an structure that represents a single QBFT execution. It holds a [`State`](#state), a [`IConfig`](#iconfig), and its start value (to be proposed if possible in case it's the leader).

It has methods to start the the QBFT protocol and to process a message. Once processing a message, it also performs a message validation as described in the formal specification.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Duplicate word: "the the QBFT protocol"

The phrase "to start the the QBFT protocol" contains a duplicated "the".

Suggested change
It has methods to start the the QBFT protocol and to process a message. Once processing a message, it also performs a message validation as described in the formal specification.
It has methods to start the QBFT protocol and to process a message. Once processing a message, it also performs a message validation as described in the formal specification.

Copy link
Copy Markdown
Contributor

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

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

Also maybe move some contents from the main README to the docs (not in this PR), and add links to the docs.

The main README should be brief with an explanation on how to run the tests

Comment thread qbft/docs/README.md

It contains:
- `MsgType`: that represents one of the possible types of messages (Proposal, Prepare, Commit, or Round-Change).
- `Height`: the height of the associated QBFT execution.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is actually the slot number

Comment thread qbft/docs/README.md
- `Round`: the message round.
- `Identifier`: the identifier related to the message, similar to the one hold by the [`Controller`](#controller).
- `Root`: the hash of the proposed data, if necessary.
- `DataRound`: the round related to the proposed data, for Round-Change messages.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this isn't clear

Comment thread qbft/docs/README.md
- `Identifier`: the identifier related to the message, similar to the one hold by the [`Controller`](#controller).
- `Root`: the hash of the proposed data, if necessary.
- `DataRound`: the round related to the proposed data, for Round-Change messages.
- `RoundChangeJustification`: used for the Proposal and Round-Change messages. For the Proposal message, it contains a list of Round-Change messages. For the Round-Change message, it contains a list of Prepare messages.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

make this clear

Comment thread qbft/docs/README.md
The main entry point of this module is the `Controller` structure. Its main goal is to allow the operator to manage sequential executions of the QBFT protocol (QBFT instances).

The controller includes:
- `Identifier`: to uniquely identify the QBFT controller. In this module, it's abstracted as a sequence of bytes but, in practice, it's a `types.MessageID` object that uniquely identifies a validator (or a committee for committee duties) and a duty type. This identifier is replicated into the [`Instance`](#instance)'s [`State`](#state) and into the QBFT messages.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe explain what identifier is made of?

Comment thread qbft/docs/README.md
The controller includes:
- `Identifier`: to uniquely identify the QBFT controller. In this module, it's abstracted as a sequence of bytes but, in practice, it's a `types.MessageID` object that uniquely identifies a validator (or a committee for committee duties) and a duty type. This identifier is replicated into the [`Instance`](#instance)'s [`State`](#state) and into the QBFT messages.
- `Height`: the current execution's height. Note that the QBFT executions are sequential so no execution run in parallel.
- `StoredInstances`: a container to save the history of QBFT instances.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this field is a historic relic, not sure it is needed anymore...

Comment thread qbft/README.md

Notice that there are some divergencies from the paper, namely:
- Proposal is an equivalent name for Pre-prepare.
- We have an extra message named Decided. This message is an aggregation of a quorum of Commit messages plus the proposal data. Its main reason of existence is that, if there is a late replica that didn't receive the quorum of Commits yet, as soon as it receives the Decided message it has all the necessary information that allows it to finish the consensus execution.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is in the paper as optional, no?

Comment thread qbft/README.md
- [//] Unified test suite, compatible with the formal verification spec
- [//] Align according to spec and [Roberto's comments](./roberto_comments)
- [ ] Remove round check from upon commit as it can be for any round?
- [ ] Use data hashes instead of full data in msgs to save space in justifications No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need to check if this task was done

Comment thread ssv/README.md

### Duty start time

Within the duty's slot, thhe time at which the operators should start the duty depends on the duty type.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Within the duty's slot, thhe time at which the operators should start the duty depends on the duty type.
Within the duty's slot, the time at which the operators should start the duty depends on the duty type.

Base automatically changed from boole to main March 26, 2026 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants