Skip to content

Conversation

samliok
Copy link
Contributor

@samliok samliok commented Jul 16, 2025

Why this should be merged

Implements the simplex QuorumCertificate, QCDeserializer and SignatureAggregator interfaces. This allows simplex to parse, aggregate and verify quorum certificates(ex. finalizations and notarizations) during execution.

How this works

  • Builds on top of the BLSVerifier to handle BLS signatures and public keys.
  • The bytes of a QC are serialized with the Codec

How this was tested

Added unit tests to qc_test.go.

Need to be documented in RELEASES.md?

no

@Copilot Copilot AI review requested due to automatic review settings July 16, 2025 13:52
@samliok samliok requested a review from StephenButtolph as a code owner July 16, 2025 13:52
Copilot

This comment was marked as outdated.

yacovm
yacovm previously approved these changes Jul 16, 2025
Copy link
Contributor

@StephenButtolph StephenButtolph left a comment

Choose a reason for hiding this comment

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

I didn't review the tests yet, will look though the tests after these are addressed.

Comment on lines 45 to 51
// Verify checks if the quorum certificate is valid by verifying the aggregated signature against the signers' public keys.
func (qc *QC) Verify(msg []byte) error {
pks := make([]*bls.PublicKey, 0, len(qc.signers))
quorum := simplex.Quorum(len(qc.verifier.nodeID2PK))
if len(qc.signers) != quorum {
return fmt.Errorf("%w: expected %d signers but got %d", errUnexpectedSigners, quorum, len(qc.signers))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there checks somewhere else that verify that the same signer isn't included multiple times?

If so we should document that assumption here. If not we need to add that.

Copy link
Contributor

@yacovm yacovm Jul 21, 2025

Choose a reason for hiding this comment

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

Simplex checks for:

  1. Finalization
  2. Notarization
  3. Empty notarization

During the regular Simplex admission path.

Though now when I quickly skimmed through the code, I think we can process an un-verified notarization through the replication path, oops... @samliok can you confirm?

I think we should be prudent and double check this here in the avalanchego side as well.

Copy link
Contributor Author

@samliok samliok Jul 22, 2025

Choose a reason for hiding this comment

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

So a couple notes after looking a bit deeper.

  • We do check if multiple nodes have signed twice in simplex see here. Although it's probably a better idea to do it here in Verify(and potentially remove the check in simplex)?
  • We don't verify notarizations through the replication path. Created an issue
  • Noticed we don't add re-add replication task if block verification fails. Issue
  • We may possible add a signature for the wrong digest when creating a notarization. Issue

I'll tackle these issues on this simplex side, plus added checking for double signers in avalanchego.

Copy link
Contributor

Choose a reason for hiding this comment

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

But it seems to me that we call verifyQuorumRound whenever we process a replication response, which calls IsFinalizationValid and it performs the duplicity checks.

@yacovm yacovm dismissed their stale review July 21, 2025 21:44

Let's double check the QC is valid (no double signing, enough signers) here as well.

@samliok samliok self-assigned this Jul 24, 2025
Copy link
Contributor

@StephenButtolph StephenButtolph left a comment

Choose a reason for hiding this comment

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

trivial nit and lgtm

@StephenButtolph StephenButtolph added this pull request to the merge queue Jul 24, 2025
Merged via the queue into master with commit 44b1e6c Jul 24, 2025
29 checks passed
@StephenButtolph StephenButtolph deleted the simplex-quorum branch July 24, 2025 20:16
@github-project-automation github-project-automation bot moved this to Done 🎉 in avalanchego Jul 24, 2025
joshua-kim added a commit that referenced this pull request Jul 30, 2025
commit 45df9e6
Author: Jonathan Oppenheimer <jonathan.oppenheimer@avalabs.org>
Date:   Tue Jul 29 15:44:08 2025 -0400

    chore: Update header year to 2025 (#4140)

commit dca17fd
Author: Stephen Buttolph <stephen@avalabs.org>
Date:   Tue Jul 29 15:41:14 2025 -0400

    Remove gitignore line that ignores the `database/dbtest` package (#4142)

commit 6e56650
Author: aaronbuchwald <aaron.buchwald56@gmail.com>
Date:   Tue Jul 29 14:29:42 2025 -0400

    Remove flaky dial throttler tests (#4139)

commit 577820e
Author: aaronbuchwald <aaron.buchwald56@gmail.com>
Date:   Tue Jul 29 13:25:27 2025 -0400

    Add runner input to run c-chain reexecution benchmark on arbitrary target (#4121)

    Signed-off-by: aaronbuchwald <aaron.buchwald56@gmail.com>
    Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

commit d5cdc50
Author: Stephen Buttolph <stephen@avalabs.org>
Date:   Tue Jul 29 13:24:13 2025 -0400

    With golangci-lint v2.2.2 using http.NewRequest is discouraged (#4136)

commit d9d6f59
Author: aaronbuchwald <aaron.buchwald56@gmail.com>
Date:   Tue Jul 29 11:49:26 2025 -0400

    Remove external-data-json-path from benchmark push step (#4134)

commit 0c62370
Author: aaronbuchwald <aaron.buchwald56@gmail.com>
Date:   Mon Jul 28 14:12:02 2025 -0400

    Split action benchmark comparison and push to gh-pages (#4130)

commit b1433fd
Author: Geoff Stuart <geoff.vball@gmail.com>
Date:   Fri Jul 25 14:50:16 2025 -0400

    Enable Cubist Signer integration (#3965)

    Signed-off-by: Geoff Stuart <geoff.vball@gmail.com>
    Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
    Co-authored-by: Richard Pringle <richard.pringle@avalabs.org>
    Co-authored-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
    Co-authored-by: Stephen Buttolph <stephen@avalabs.org>

commit 44c79a8
Author: rodrigo <77309055+RodrigoVillar@users.noreply.github.com>
Date:   Fri Jul 25 10:52:17 2025 -0400

    refactor: remove load 1.0  (#4112)

commit e658cf4
Author: jishudashu <979260390@qq.com>
Date:   Fri Jul 25 22:50:43 2025 +0800

    refactor: use maps.Copy for cleaner map handling (#4119)

    Signed-off-by: jishudashu <979260390@qq.com>

commit 0980edf
Author: aaronbuchwald <aaron.buchwald56@gmail.com>
Date:   Thu Jul 24 16:13:30 2025 -0400

    Update codeowners of reexecution changes (#4116)

commit 44b1e6c
Author: Sam Liokumovich <65994425+samliok@users.noreply.github.com>
Date:   Thu Jul 24 16:00:45 2025 -0400

    Simplex QuorumCertificate and BLS aggregator (#4091)

commit 5af69e6
Author: rodrigo <77309055+RodrigoVillar@users.noreply.github.com>
Date:   Thu Jul 24 13:17:48 2025 -0400

    ci: remove load 1.0 (#4106)

commit 12b75d4
Author: aaronbuchwald <aaron.buchwald56@gmail.com>
Date:   Wed Jul 23 13:01:42 2025 -0400

    Add step to push benchmark results to gh-pages (#4103)

    Signed-off-by: aaronbuchwald <aaron.buchwald56@gmail.com>

commit c712437
Author: Sam Liokumovich <65994425+samliok@users.noreply.github.com>
Date:   Wed Jul 23 11:33:29 2025 -0400

    Remove Stale References of the toEngine Channel (#4101)

Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants