Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ require (
github.com/stretchr/testify v1.10.0
go.uber.org/goleak v1.3.0
go.uber.org/zap v1.26.0
golang.org/x/sys v0.45.0
)

require (
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ go.uber.org/multierr v1.10.0 h1:S0h4aNzvfcFsC3dRF1jLoaov7oRaKqRGC/pUEJ2yvPQ=
go.uber.org/multierr v1.10.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y=
go.uber.org/zap v1.26.0 h1:sI7k6L95XOKS281NhVKOFCUNIvv9e0w4BF8N3u+tCRo=
go.uber.org/zap v1.26.0/go.mod h1:dtElttAiwGvoJ/vj4IwHBS/gXsEu/pZ50mUIRWuG0so=
golang.org/x/sys v0.45.0 h1:dO4czNzziLiiXplLQgBCEpCvXQ3dnkn0SdaZSYdQ+FY=
golang.org/x/sys v0.45.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127 h1:qIbj1fsPNlZgppZ+VLlY7N33q108Sa+fhmuc+sWQYwY=
gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
Expand Down
155 changes: 155 additions & 0 deletions msm/approvals.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
// Copyright (C) 2019-2025, Ava Labs, Inc. All rights reserved.
// See the file LICENSE for licensing terms.

package metadata

import (
"encoding/asn1"
"encoding/binary"
"fmt"

"github.com/ava-labs/simplex"
"go.uber.org/zap"
)

type approvalsByPChainHeight map[uint64]*approvalAndTimestamp

type approvalAndTimestamp struct {
ValidatorSetApproval
Timestamp uint64
}

type ApprovalStore struct {
Comment thread
samliok marked this conversation as resolved.
signatureVerifier SignatureVerifier
validators NodeBLSMappings
logger simplex.Logger
pkByNodeID map[nodeID][]byte
approvalsByNodes map[nodeID]approvalsByPChainHeight
storedCount int
}

func NewApprovalStore(signatureVerifier SignatureVerifier, validators NodeBLSMappings, logger simplex.Logger) *ApprovalStore {
pkByNodeID := make(map[nodeID][]byte)
for _, vdr := range validators {
pkByNodeID[vdr.NodeID] = vdr.BLSKey
}

approvalsByNodes := make(map[nodeID]approvalsByPChainHeight, len(validators))
for _, vdr := range validators {
approvalsByNodes[vdr.NodeID] = make(approvalsByPChainHeight)
}

return &ApprovalStore{
signatureVerifier: signatureVerifier,
validators: validators,
pkByNodeID: pkByNodeID,
logger: logger,
approvalsByNodes: approvalsByNodes,
}
}

func (as *ApprovalStore) Approvals() ValidatorSetApprovals {
approvals := make(ValidatorSetApprovals, 0, as.storedCount)
for _, approvalsByHeight := range as.approvalsByNodes {
for _, approval := range approvalsByHeight {
approvals = append(approvals, (*approval).ValidatorSetApproval)
}
}
return approvals
}

func (as *ApprovalStore) HandleApproval(approval *ValidatorSetApproval, timestamp uint64) error {
// First thing we check is if the node that sent this approval is a validator.
pk, exists := as.getPKOfNode(approval.NodeID)
if !exists {
as.logger.Debug("Received an approval from a node that is not a validator", zap.String("nodeID",
fmt.Sprintf("%x", approval.NodeID)), zap.Uint64("pChainHeight", approval.PChainHeight))
return nil
}

// Second thing we check is if we already have an approval for this height from this node.
if as.approvalExistsAndUpToDate(approval, timestamp) {
as.logger.Debug("Already have an approval from the node", zap.String("nodeID",
fmt.Sprintf("%x", approval.NodeID)), zap.Uint64("pChainHeight", approval.PChainHeight))
return nil
}

// Third thing we check is if the signature of the approval is valid.
// We need it to be valid in order for nodes to be able to aggregate it later on along with other approvals.
if err := as.checkApprovalSignature(approval, pk); err != nil {
as.logger.Debug("Received an approval with an invalid signature", zap.String("nodeID",
fmt.Sprintf("%x", approval.NodeID)), zap.Uint64("pChainHeight", approval.PChainHeight))
return nil
}

// Store the approval.
oldApproval := as.approvalsByNodes[approval.NodeID][approval.PChainHeight]
as.approvalsByNodes[approval.NodeID][approval.PChainHeight] = &approvalAndTimestamp{
ValidatorSetApproval: *approval,
Timestamp: timestamp,
}

if oldApproval == nil {
as.storedCount++
}

// We only store the last |as.validators| of approvals for each node,
// so we need to delete old approvals if we have more than |as.validators| approvals stored for this node.
as.maybePruneOldApprovals(approval)

return nil
}

func (as *ApprovalStore) maybePruneOldApprovals(approval *ValidatorSetApproval) {
if len(as.approvalsByNodes[approval.NodeID]) <= len(as.validators) {
return
}
// Find the oldest approval and delete it.
var oldestApproval *approvalAndTimestamp
for _, approval := range as.approvalsByNodes[approval.NodeID] {
if oldestApproval == nil || approval.Timestamp < oldestApproval.Timestamp {
oldestApproval = approval
}
}

if oldestApproval != nil {
as.logger.Debug("Deleting old approval from node",
zap.String("nodeID", fmt.Sprintf("%x", oldestApproval.NodeID)),
zap.String("oldestApprovalPChainHeight",
fmt.Sprintf("%d", oldestApproval.PChainHeight)), zap.Uint64("oldestApprovalTimestamp", oldestApproval.Timestamp))
delete(as.approvalsByNodes[approval.NodeID], oldestApproval.PChainHeight)
as.storedCount--
}
}

func (as *ApprovalStore) checkApprovalSignature(approval *ValidatorSetApproval, pk []byte) error {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

hmm is it not a problem that we verify the signature is valid over just the pChainHeight here, but in the MSM code we verify it with the context + asn1 encoding? shouldn't there be one canonical way of verifying/signing that is shared for both of them?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, because when I implemented this, I still haven't done that domain separation in the MSM.

Will fix 👍

Copy link
Copy Markdown
Collaborator Author

@yacovm yacovm Jun 1, 2026

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is it still not a problem though that we don't check the approval signature against the timestamp? we also don't pass in who sent the approval so someone could send an approval with a timestamp = MaxUint64 and say it was from another node. This would remove all other approvals from that node in maybePruneOldApprovals.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think you have answered your own question.

This only works if the dissemination of the approvals is point to point, over authenticated channels, and not via gossip.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Also recall that:

type NextEpochApprovals struct {
	NodeIDs   []byte `canoto:"bytes,1"`
	Signature []byte `canoto:"bytes,2"`

	canotoData canotoData_NextEpochApprovals
}

This doesn't include timestamps, and we naturally cannot expect all signatures to match a single timestamp.

The timestamp is just there to give us liveness when a node crashes and restarts or picks a different P-chain height to sign over. We need some way of disposing of "old" in-memory values.

We could have the timestamp be even computed locally inside of HandleApproval and not sent over the wire at all.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We could have the timestamp be even computed locally inside of HandleApproval and not sent over the wire at all

might be a nice change, why send it if we don't need to.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

pChainHeight := approval.PChainHeight
pChainHeightBuff := make([]byte, 8)
binary.BigEndian.PutUint64(pChainHeightBuff, pChainHeight)

signedMsg := simplex.SignedMessage{Payload: pChainHeightBuff, Context: signatureContext}
toBeSigned, err := asn1.Marshal(signedMsg)
if err != nil {
return err
}

// We check if the signature is valid before we store the approval.
return as.signatureVerifier.VerifySignature(approval.Signature, toBeSigned, pk)
}

func (as *ApprovalStore) getPKOfNode(nodeID nodeID) ([]byte, bool) {
pk, exists := as.pkByNodeID[nodeID]
return pk, exists
}

func (as *ApprovalStore) approvalExistsAndUpToDate(approval *ValidatorSetApproval, timestamp uint64) bool {
if as.approvalsByNodes[approval.NodeID] == nil {
return false
}
existingApproval := as.approvalsByNodes[approval.NodeID][approval.PChainHeight]
if existingApproval == nil {
return false
}

return existingApproval.Timestamp >= timestamp
}
Loading
Loading