Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

POC Signature aggregation API #368

Merged
merged 55 commits into from
Aug 8, 2024
Merged
Show file tree
Hide file tree
Changes from 50 commits
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
5a4b307
Implement POC of Signature Aggregator API
iansuvak Jul 15, 2024
2fbaa7a
Add SigningSubnetID to the interface
iansuvak Jul 17, 2024
3ea7b58
compiles but doesn't run
iansuvak Jul 18, 2024
09083e7
basic test passes
iansuvak Jul 18, 2024
af458cf
Add new constructors for signature-aggregator configs from base configs
iansuvak Jul 18, 2024
58a0747
Get subnetID from Pchain if not available
iansuvak Jul 19, 2024
7ead3ab
Hook up relayer to sig aggregator -- tests not passing yet
iansuvak Jul 19, 2024
eb90426
Merge remote-tracking branch 'origin/main' into signature-aggregation…
iansuvak Jul 22, 2024
b6231a5
Apply suggestions from code review
iansuvak Jul 22, 2024
90252ef
Move back to using existing implementation for now
iansuvak Jul 22, 2024
138316c
Rip out sourceblockchain configs
iansuvak Jul 22, 2024
3ea861b
remove deleted file
iansuvak Jul 22, 2024
a0a6fb8
Fix existing tests =)
iansuvak Jul 23, 2024
d6c0aae
remove apprequest code from relayer
iansuvak Jul 23, 2024
2356886
remove locks and requestID counter from relayer
iansuvak Jul 23, 2024
055e983
Move aggregator to signature-aggregator directory
iansuvak Jul 23, 2024
16be2c7
Add small README
iansuvak Jul 23, 2024
f7097ee
merge peer code
iansuvak Jul 24, 2024
80f7f57
re-add pre-connect functionality + trackedSubnets for relayer
iansuvak Jul 24, 2024
120b9ce
fix lint issues
iansuvak Jul 24, 2024
b1690ef
SigAggRawReq::UnsignedMessage: hex str, not bytes
feuGeneA Jul 29, 2024
c61239e
fix log error message
feuGeneA Jul 29, 2024
22d6380
deduplicate log message
feuGeneA Jul 29, 2024
68b0ee2
SigAgResponse: hex encoded, not bytes
feuGeneA Jul 29, 2024
05b1d59
set response type to json
feuGeneA Jul 30, 2024
079ff03
errors: don't expose internals; always JSON
feuGeneA Jul 30, 2024
90f4005
avoid a pointer to int
feuGeneA Jul 30, 2024
97b926c
avoid a pointer to ids.ID
feuGeneA Jul 30, 2024
e90f8d2
avoid another pointer to ids.ID
feuGeneA Jul 30, 2024
da09584
streamline quorum handling
feuGeneA Jul 30, 2024
15045f0
Merge pull request #393 from ava-labs/signature-aggregation-api-issue…
iansuvak Jul 31, 2024
cc28367
remove dead interface, update README, add sample config.json
iansuvak Jul 26, 2024
9ece3be
rename requests and resources
feuGeneA Aug 1, 2024
9519013
Merge remote-tracking branch 'origin/main' into signature-aggregation…
feuGeneA Aug 1, 2024
3d970e6
fixup README after resource renames
feuGeneA Aug 1, 2024
2cc9280
put all scripts in `/scripts`
feuGeneA Aug 1, 2024
bf9201d
fixup put all scripts in `/scripts`
feuGeneA Aug 1, 2024
f12dcea
wrap errors with descriptive messages
feuGeneA Aug 1, 2024
82ef662
avoid writing undefined response data
feuGeneA Aug 1, 2024
ea3ade8
Merge remote-tracking branch 'origin/main' into signature-aggregation…
iansuvak Aug 2, 2024
15029ba
remove no longer relevant TODO
iansuvak Aug 2, 2024
44ff6ed
Apply suggestions from code review
iansuvak Aug 6, 2024
7dcb2f0
Apply suggestions from code review
iansuvak Aug 6, 2024
3b01ef6
address review feedback
iansuvak Aug 6, 2024
bbe3032
Apply suggestions from code review
iansuvak Aug 6, 2024
6802470
Address review feedback
iansuvak Aug 6, 2024
96e8262
Merge branch 'main' into signature-aggregation-api
iansuvak Aug 6, 2024
73089d4
address review feedback
iansuvak Aug 7, 2024
c73cf66
fix typos
iansuvak Aug 7, 2024
f8130ce
Merge remote-tracking branch 'origin/main' into signature-aggregation…
iansuvak Aug 7, 2024
6f971c3
Merge branch 'main' into signature-aggregation-api
iansuvak Aug 7, 2024
eadb326
Update signature-aggregator/README.md
iansuvak Aug 8, 2024
79dca35
Update signature-aggregator/README.md
iansuvak Aug 8, 2024
8b12597
address review feedback
iansuvak Aug 8, 2024
1477133
invert utils.CheckStakeWeightPercentageExceedsThreshold
iansuvak Aug 8, 2024
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
10 changes: 10 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -246,3 +246,13 @@ func (c *Config) GetWarpQuorum(blockchainID ids.ID) (WarpQuorum, error) {
}
return WarpQuorum{}, errFailedToGetWarpQuorum
}

// Config implements the peers.Config interface
func (c *Config) GetPChainAPI() *APIConfig {
return c.PChainAPI
}

// Config implements the peers.Config interface
func (c *Config) GetInfoAPI() *APIConfig {
return c.InfoAPI
}
26 changes: 21 additions & 5 deletions main/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/ava-labs/avalanchego/message"
"github.com/ava-labs/avalanchego/utils/constants"
"github.com/ava-labs/avalanchego/utils/logging"
"github.com/ava-labs/avalanchego/utils/set"
"github.com/ava-labs/awm-relayer/api"
"github.com/ava-labs/awm-relayer/config"
"github.com/ava-labs/awm-relayer/database"
Expand All @@ -26,6 +27,7 @@ import (
"github.com/ava-labs/awm-relayer/peers"
"github.com/ava-labs/awm-relayer/relayer"
"github.com/ava-labs/awm-relayer/relayer/checkpoint"
"github.com/ava-labs/awm-relayer/signature-aggregator/aggregator"
"github.com/ava-labs/awm-relayer/utils"
"github.com/ava-labs/awm-relayer/vms"
"github.com/ava-labs/subnet-evm/ethclient"
Expand Down Expand Up @@ -134,11 +136,23 @@ func main() {
if logLevel <= logging.Debug {
networkLogLevel = logLevel
}
var trackedSubnets set.Set[ids.ID]
iansuvak marked this conversation as resolved.
Show resolved Hide resolved
// trackedSubnets is no longer strictly required but keeping it here for now
// to keep full parity with existing AWM relayer for now
// TODO: remove this from here once trackedSubnets are no longer referenced
// by ping messages in avalanchego
for _, sourceBlockchain := range cfg.SourceBlockchains {
trackedSubnets.Add(sourceBlockchain.GetSubnetID())
}

network, err := peers.NewNetwork(
networkLogLevel,
prometheus.DefaultRegisterer,
trackedSubnets,
&cfg,
)
network.InitializeConnectionsAndCheckStake(&cfg)

Choose a reason for hiding this comment

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

does the config nee to be passed by reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not strictly no, since it gets used as interface but from what I've seen we normally pass configs as references and I've done that in the past as well. Not a big deal in general but in applications where configs get passed around a lot doing this can save on allocs and garbage collection.

Choose a reason for hiding this comment

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

can we remove the pass by ref here, want to avoid it being unintentionally altered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW configs are being passed by reference everywhere in this codebase. As mentioned in previous comment we can clean this up but I'd like to punt on this until later

Copy link

@minghinmatthewlam minghinmatthewlam Aug 8, 2024

Choose a reason for hiding this comment

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

same as above, good on punting but can you create an issue to track and link here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated: #409


if err != nil {
logger.Fatal("Failed to create app request network", zap.Error(err))
panic(err)
Expand Down Expand Up @@ -197,17 +211,19 @@ func main() {
panic(err)
}

signatureAggregator := aggregator.NewSignatureAggregator(network, logger, messageCreator)

applicationRelayers, minHeights, err := createApplicationRelayers(
context.Background(),
logger,
relayerMetrics,
db,
ticker,
network,
messageCreator,
&cfg,
sourceClients,
destinationClients,
signatureAggregator,
)
if err != nil {
logger.Fatal("Failed to create application relayers", zap.Error(err))
Expand Down Expand Up @@ -332,10 +348,10 @@ func createApplicationRelayers(
db database.RelayerDatabase,
ticker *utils.Ticker,
network *peers.AppRequestNetwork,
messageCreator message.Creator,
cfg *config.Config,
sourceClients map[ids.ID]ethclient.Client,
destinationClients map[ids.ID]vms.DestinationClient,
signatureAggregator *aggregator.SignatureAggregator,
) (map[common.Hash]*relayer.ApplicationRelayer, map[ids.ID]uint64, error) {
applicationRelayers := make(map[common.Hash]*relayer.ApplicationRelayer)
minHeights := make(map[ids.ID]uint64)
Expand All @@ -355,10 +371,10 @@ func createApplicationRelayers(
ticker,
*sourceBlockchain,
network,
messageCreator,
cfg,
currentHeight,
destinationClients,
signatureAggregator,
)
if err != nil {
logger.Error(
Expand Down Expand Up @@ -391,10 +407,10 @@ func createApplicationRelayersForSourceChain(
ticker *utils.Ticker,
sourceBlockchain config.SourceBlockchain,
network *peers.AppRequestNetwork,
messageCreator message.Creator,
cfg *config.Config,
currentHeight uint64,
destinationClients map[ids.ID]vms.DestinationClient,
signatureAggregator *aggregator.SignatureAggregator,
) (map[common.Hash]*relayer.ApplicationRelayer, uint64, error) {
// Create the ApplicationRelayers
logger.Info(
Expand Down Expand Up @@ -438,12 +454,12 @@ func createApplicationRelayersForSourceChain(
logger,
metrics,
network,
messageCreator,
iansuvak marked this conversation as resolved.
Show resolved Hide resolved
relayerID,
destinationClients[relayerID.DestinationBlockchainID],
sourceBlockchain,
checkpointManager,
cfg,
signatureAggregator,
)
if err != nil {
logger.Error(
Expand Down
78 changes: 53 additions & 25 deletions peers/app_request_network.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,18 @@ package peers

import (
"context"
"fmt"
"math/big"
"os"
"sync"
"time"

"github.com/ava-labs/avalanchego/ids"
"github.com/ava-labs/avalanchego/message"
"github.com/ava-labs/avalanchego/network"
avagoCommon "github.com/ava-labs/avalanchego/snow/engine/common"
snowVdrs "github.com/ava-labs/avalanchego/snow/validators"
"github.com/ava-labs/avalanchego/subnets"
"github.com/ava-labs/avalanchego/utils/constants"
"github.com/ava-labs/avalanchego/utils/logging"
"github.com/ava-labs/avalanchego/utils/set"
Expand All @@ -30,8 +34,8 @@ const (
)

type AppRequestNetwork struct {
Network network.Network
Handler *RelayerExternalHandler
network network.Network
handler *RelayerExternalHandler
infoAPI *InfoAPI
logger logging.Logger
lock *sync.Mutex
Expand All @@ -42,7 +46,8 @@ type AppRequestNetwork struct {
func NewNetwork(
logLevel logging.Level,
registerer prometheus.Registerer,
cfg *config.Config,
trackedSubnets set.Set[ids.ID],
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may want to wait until it's included in a release, but we can remove this now that ava-labs/avalanchego#3258 is merged right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we definitely need to keep it for now and I want to save bumping different versions of all our dependencies for a later PR. I am sort of considering keeping it for now as well though since there are other differences/places where trackedSubnets are used in the networking code.

Specifically uptime tracking via ping messages. Not having trackedSubnets set makes peer nodes spew more logs about uptime tracking. This uptime tracking via ping messages should go away soon as well but it hasn't yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good. A more general comment, there's a lot of miscellania that's rightly being deferred in this PR. Do you mind enumerating them in a followup ticket so that they don't get lost in the chaff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup I will enter those issues and link them in the description for easy reference before we are ok to merge this one.

cfg Config,
feuGeneA marked this conversation as resolved.
Show resolved Hide resolved
) (*AppRequestNetwork, error) {
logger := logging.NewLogger(
"awm-relayer-p2p",
Expand All @@ -63,7 +68,7 @@ func NewNetwork(
return nil, err
}

infoAPI, err := NewInfoAPI(cfg.InfoAPI)
infoAPI, err := NewInfoAPI(cfg.GetInfoAPI())
if err != nil {
logger.Error(
"Failed to create info API",
Expand All @@ -80,11 +85,6 @@ func NewNetwork(
return nil, err
}

var trackedSubnets set.Set[ids.ID]
for _, sourceBlockchain := range cfg.SourceBlockchains {
trackedSubnets.Add(sourceBlockchain.GetSubnetID())
}

testNetwork, err := network.NewTestNetwork(logger, networkID, snowVdrs.NewManager(), trackedSubnets, handler)
if err != nil {
logger.Error(
Expand All @@ -94,38 +94,47 @@ func NewNetwork(
return nil, err
}

validatorClient := validators.NewCanonicalValidatorClient(logger, cfg.PChainAPI)
validatorClient := validators.NewCanonicalValidatorClient(logger, cfg.GetPChainAPI())

arNetwork := &AppRequestNetwork{
Network: testNetwork,
Handler: handler,
network: testNetwork,
handler: handler,
infoAPI: infoAPI,
logger: logger,
lock: new(sync.Mutex),
validatorClient: validatorClient,
}
go logger.RecoverAndPanic(func() {
testNetwork.Dispatch()

Choose a reason for hiding this comment

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

do we need to watch the error returned from Dispatch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that RecoverAndPanic should cover our needs here but could be wrong

})

return arNetwork, nil
}

// TODO: remove dependence on Relayer specific config since this is meant to be a generic AppRequestNetwork file
func (n *AppRequestNetwork) InitializeConnectionsAndCheckStake(cfg *config.Config) error {
// Manually connect to the validators of each of the source subnets.
// We return an error if we are unable to connect to sufficient stake on any of the subnets.
// Sufficient stake is determined by the Warp quora of the configured supported destinations,
// or if the subnet supports all destinations, by the quora of all configured destinations.
for _, sourceBlockchain := range cfg.SourceBlockchains {
if sourceBlockchain.GetSubnetID() == constants.PrimaryNetworkID {
if err := arNetwork.connectToPrimaryNetworkPeers(cfg, sourceBlockchain); err != nil {
return nil, err
if err := n.connectToPrimaryNetworkPeers(cfg, sourceBlockchain); err != nil {
return fmt.Errorf(
"failed to connect to primary network peers: %w",
err,
)
}
} else {
if err := arNetwork.connectToNonPrimaryNetworkPeers(cfg, sourceBlockchain); err != nil {
return nil, err
if err := n.connectToNonPrimaryNetworkPeers(cfg, sourceBlockchain); err != nil {
return fmt.Errorf(
"failed to connect to non-primary network peers: %w",
err,
)
}
}
}

go logger.RecoverAndPanic(func() {
testNetwork.Dispatch()
})

return arNetwork, nil
return nil
}

// ConnectPeers connects the network to peers with the given nodeIDs.
Expand All @@ -135,7 +144,7 @@ func (n *AppRequestNetwork) ConnectPeers(nodeIDs set.Set[ids.NodeID]) set.Set[id
defer n.lock.Unlock()

// First, check if we are already connected to all the peers
connectedPeers := n.Network.PeerInfo(nodeIDs.List())
connectedPeers := n.network.PeerInfo(nodeIDs.List())
if len(connectedPeers) == nodeIDs.Len() {
return nodeIDs
}
Expand All @@ -160,7 +169,7 @@ func (n *AppRequestNetwork) ConnectPeers(nodeIDs set.Set[ids.NodeID]) set.Set[id
for _, peer := range peers {
if nodeIDs.Contains(peer.ID) {
trackedNodes.Add(peer.ID)
n.Network.ManuallyTrack(peer.ID, peer.PublicIP)
n.network.ManuallyTrack(peer.ID, peer.PublicIP)
if len(trackedNodes) == nodeIDs.Len() {
return trackedNodes
}
Expand All @@ -182,7 +191,7 @@ func (n *AppRequestNetwork) ConnectPeers(nodeIDs set.Set[ids.NodeID]) set.Set[id
)
} else {
trackedNodes.Add(apiNodeID)
n.Network.ManuallyTrack(apiNodeID, apiNodeIPPort)
n.network.ManuallyTrack(apiNodeID, apiNodeIPPort)
}
}

Expand Down Expand Up @@ -245,6 +254,25 @@ func (n *AppRequestNetwork) ConnectToCanonicalValidators(subnetID ids.ID) (*Conn
}, nil
}

func (n *AppRequestNetwork) Send(
msg message.OutboundMessage,
nodeIDs set.Set[ids.NodeID],
subnetID ids.ID,
allower subnets.Allower,

Choose a reason for hiding this comment

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

do we need to set allower here? Or can we just set as the noop allower

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to either. In general I like to keep these wrapper interface methods as close as possible in the signature to the original but this is already broken here since we use the legacy VDR set instead of the new SendConfig. I'm fine with either:
1️⃣ assuming our usage, since we only call send from here and do the conversion to SendConfig here and explicitly set the noopAllower here as well
2️⃣ keep the interface of Send same as network.Send and do the conversion and allower setting in the application layer.

I will defer on decision here to whatever the majority preference is

) set.Set[ids.NodeID] {
return n.network.Send(msg, avagoCommon.SendConfig{NodeIDs: nodeIDs}, subnetID, allower)
}

func (n *AppRequestNetwork) RegisterAppRequest(requestID ids.RequestID) {
n.handler.RegisterAppRequest(requestID)
}
func (n *AppRequestNetwork) RegisterRequestID(requestID uint32, numExpectedResponse int) chan message.InboundMessage {
return n.handler.RegisterRequestID(requestID, numExpectedResponse)
}
func (n *AppRequestNetwork) GetSubnetID(blockchainID ids.ID) (ids.ID, error) {
return n.validatorClient.GetSubnetID(context.Background(), blockchainID)
}

// Private helpers

// Connect to the validators of the source blockchain. For each destination blockchain,
Expand Down
12 changes: 12 additions & 0 deletions peers/config.go
Copy link
Collaborator

Choose a reason for hiding this comment

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

This interface type seems a bit strange to me. I see that it's used to provide access to NewNetwork in both the awm-relayer and signature-aggregator code. However, the interface methods are 1) very limited in scope and 2) seem to reflect happenstance commonality between the two application configurations, rather than properties that all services that act as peers must implement. Lastly, it's odd that the configuration interface returns a type specified by a package that implements the interface.

I think it would be cleaner to simply pass in the corresponding *config.APIConfig to NewNetwork,

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively, renaming to AggregatorConfig and moving it to signature-aggregator/aggregator would make more sense from a code organization perspective.

Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Copyright (C) 2024, Ava Labs, Inc. All rights reserved.
// See the file LICENSE for licensing terms.

package peers

import "github.com/ava-labs/awm-relayer/config"

// Config defines a common interface necessary for standing up an AppRequestNetwork.
type Config interface {
GetInfoAPI() *config.APIConfig
GetPChainAPI() *config.APIConfig
}
Loading
Loading