diff --git a/Dockerfile b/Dockerfile index 20131da25e..c9cc3b819a 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,5 +1,5 @@ # Container for building Go binary. -FROM golang:1.26.5-trixie AS builder +FROM golang:1.26.6-trixie AS builder # Install dependencies RUN apt-get update && apt-get install -y --no-install-recommends build-essential git diff --git a/app/app.go b/app/app.go index 12d1f208e7..87e6bd3653 100644 --- a/app/app.go +++ b/app/app.go @@ -716,7 +716,7 @@ func wireCoreWorkflow(ctx context.Context, life *lifecycle.Manager, conf Config, // Priority protocol always uses QBFTv2. isync, err := wirePrioritise(ctx, conf, life, p2pNode, peerIDs, lock.Threshold, sender.SendReceive, defaultConsensus, sched, p2pKey, deadlineFunc, - consensusController, lock.ConsensusProtocol) + consensusController, lock.ConsensusProtocol, gaterFunc) if err != nil { return err } @@ -773,6 +773,7 @@ func wirePrioritise(ctx context.Context, conf Config, life *lifecycle.Manager, p peers []peer.ID, threshold int, sendFunc p2p.SendReceiveFunc, coreCons core.Consensus, sched core.Scheduler, p2pKey *k1.PrivateKey, deadlineFunc func(duty core.Duty) (time.Time, bool), consensusController core.ConsensusController, clusterPreferredProtocol string, + gaterFunc core.DutyGaterFunc, ) (*infosync.Component, error) { cons, ok := coreCons.(*qbft.Consensus) if !ok { @@ -785,7 +786,7 @@ func wirePrioritise(ctx context.Context, conf Config, life *lifecycle.Manager, p const exchangeTimeout = time.Second * 6 prio, err := priority.NewComponent(ctx, p2pNode, peers, threshold, - sendFunc, p2p.RegisterHandler, cons, exchangeTimeout, p2pKey, deadlineFunc) + sendFunc, p2p.RegisterHandler, cons, exchangeTimeout, p2pKey, deadlineFunc, gaterFunc) if err != nil { return nil, err } diff --git a/app/log/config.go b/app/log/config.go index bdd2636fb8..2f7ddbfc71 100644 --- a/app/log/config.go +++ b/app/log/config.go @@ -264,39 +264,51 @@ func NewConsoleForT(_ *testing.T, ws zapcore.WriteSyncer, opts ...func(*zapcore. } // InitConsoleForT initialises a global console logger for testing purposes. +// The previous global logger is restored on test cleanup. func InitConsoleForT(t *testing.T, ws zapcore.WriteSyncer, opts ...func(*zapcore.EncoderConfig)) { t.Helper() - - initMu.Lock() - defer initMu.Unlock() - - logger = NewConsoleForT(t, ws, opts...) + setLoggerForT(t, NewConsoleForT(t, zapcore.Lock(ws), opts...)) } // InitJSONForT initialises a json logger for testing purposes. +// The previous global logger is restored on test cleanup. func InitJSONForT(t *testing.T, ws zapcore.WriteSyncer, opts ...func(*zapcore.EncoderConfig)) { t.Helper() - initMu.Lock() - defer initMu.Unlock() - - var err error - - logger, err = newStructuredLogger("json", zapcore.DebugLevel, true, ws, defaultCallerSkip, opts...) + l, err := newStructuredLogger("json", zapcore.DebugLevel, true, zapcore.Lock(ws), defaultCallerSkip, opts...) require.NoError(t, err) + + setLoggerForT(t, l) } // InitLogfmtForT initialises a logfmt logger for testing purposes. +// The previous global logger is restored on test cleanup. func InitLogfmtForT(t *testing.T, ws zapcore.WriteSyncer, opts ...func(*zapcore.EncoderConfig)) { t.Helper() + l, err := newStructuredLogger("logfmt", zapcore.DebugLevel, false, zapcore.Lock(ws), defaultCallerSkip, opts...) + require.NoError(t, err) + + setLoggerForT(t, l) +} + +// setLoggerForT sets the global logger and restores the previous logger on test cleanup. +// Note the write syncer must be safe for concurrent use since logging may happen from multiple goroutines. +func setLoggerForT(t *testing.T, l zapLogger) { + t.Helper() + initMu.Lock() defer initMu.Unlock() - var err error + prev := logger + logger = l - logger, err = newStructuredLogger("logfmt", zapcore.DebugLevel, false, ws, defaultCallerSkip, opts...) - require.NoError(t, err) + t.Cleanup(func() { + initMu.Lock() + defer initMu.Unlock() + + logger = prev + }) } // Stop stops all log processors. diff --git a/app/log/config_internal_test.go b/app/log/config_internal_test.go index 9baf5c59c1..e7274e7f53 100644 --- a/app/log/config_internal_test.go +++ b/app/log/config_internal_test.go @@ -9,10 +9,13 @@ import ( "net/http" "net/http/httptest" "strconv" + "sync" "testing" "github.com/golang/snappy" "github.com/stretchr/testify/require" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest" "google.golang.org/protobuf/proto" pbv1 "github.com/obolnetwork/charon/app/log/loki/lokipb/v1" @@ -68,6 +71,56 @@ func TestLokiCaller(t *testing.T) { <-done } +var initForTFuncs = map[string]func(*testing.T, zapcore.WriteSyncer, ...func(*zapcore.EncoderConfig)){ + "console": InitConsoleForT, + "logfmt": InitLogfmtForT, + "json": InitJSONForT, +} + +func TestInitForTRestoresLogger(t *testing.T) { + for name, initFunc := range initForTFuncs { + t.Run(name, func(t *testing.T) { + var buf zaptest.Buffer + + t.Run("install", func(t *testing.T) { + initFunc(t, &buf) + Debug(context.Background(), "inside test") + require.Contains(t, buf.String(), "inside test") + }) + + // The previous logger must be restored on test cleanup, + // so this log must not be written to the buffer. + lenBefore := buf.Len() + + Debug(context.Background(), "after test") + require.Equal(t, lenBefore, buf.Len()) + }) + } +} + +func TestInitForTConcurrentLogging(t *testing.T) { + for name, initFunc := range initForTFuncs { + t.Run(name, func(t *testing.T) { + // zaptest.Buffer is not safe for concurrent use, the + // initialisers must synchronise writes to it. + var buf zaptest.Buffer + + initFunc(t, &buf) + + var wg sync.WaitGroup + for range 8 { + wg.Go(func() { + for range 100 { + Debug(context.Background(), "concurrent log") + } + }) + } + + wg.Wait() + }) + } +} + func decode(t *testing.T, b []byte) *pbv1.PushRequest { t.Helper() diff --git a/app/log/loki/lokipb/v1/loki.pb.go b/app/log/loki/lokipb/v1/loki.pb.go index 1da54ca863..70714de0d7 100644 --- a/app/log/loki/lokipb/v1/loki.pb.go +++ b/app/log/loki/lokipb/v1/loki.pb.go @@ -1,6 +1,6 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.11 +// protoc-gen-go v1.36.12 // protoc (unknown) // source: app/log/loki/lokipb/v1/loki.proto diff --git a/app/log/slog.go b/app/log/slog.go index ea5879694a..fc7cc82578 100644 --- a/app/log/slog.go +++ b/app/log/slog.go @@ -1,9 +1,11 @@ // Copyright © 2022-2026 Obol Labs Inc. Licensed under the terms of a Business Source License 1.1 +//nolint:revive,nolintlint // somehow the nolintlint linter catches revive as unnecessary, while it is package log import ( "context" + "fmt" "log/slog" "os" "runtime" @@ -11,6 +13,9 @@ import ( "sync" "go.uber.org/zap/zapcore" + + "github.com/obolnetwork/charon/app/errors" + "github.com/obolnetwork/charon/app/z" ) // SlogHandler returns a slog.Handler that writes records to the global charon logger. @@ -43,6 +48,20 @@ func (h *slogHandler) Enabled(_ context.Context, level slog.Level) bool { } func (h *slogHandler) Handle(_ context.Context, rec slog.Record) error { + // Never let a logging panic crash the process. + defer func() { + if r := recover(); r != nil { + defer func() { + if r2 := recover(); r2 != nil { + fmt.Fprintf(os.Stderr, "slog handler panic (logging also failed): %v\n", r) + } + }() + + Error(context.Background(), "Libp2p slog handler panic, log line dropped", + errors.New("slog handler panic", z.Str("panic", fmt.Sprint(r)))) + } + }() + entry := zapcore.Entry{ Level: toZapLevel(rec.Level), Time: rec.Time, @@ -105,13 +124,15 @@ func (h *slogHandler) clone() *slogHandler { } // toZapField converts a slog attribute to a zap field, prefixing open group names. +// All values are stringified to avoid zapcore.ReflectType, which panics in the +// logfmt encoder on named types (e.g. protocol.ID). func (h *slogHandler) toZapField(a slog.Attr) zapcore.Field { key := a.Key if len(h.groups) > 0 { key = strings.Join(h.groups, ".") + "." + key } - return zapcore.Field{Key: key, Type: zapcore.ReflectType, Interface: a.Value.Resolve().Any()} + return zapcore.Field{Key: key, Type: zapcore.StringType, String: fmt.Sprint(a.Value.Resolve().Any())} } // toZapLevel maps a slog level to the closest zap level. diff --git a/app/log/slog_internal_test.go b/app/log/slog_internal_test.go index 03910a9066..07eed4a5d3 100644 --- a/app/log/slog_internal_test.go +++ b/app/log/slog_internal_test.go @@ -83,3 +83,41 @@ func TestSlogHandler(t *testing.T) { other.Error("failed to listen", "err", "address in use") require.Contains(t, buf.String(), "failed to listen") } + +// namedString is a named string type like protocol.ID that is not plain string. +type namedString string + +func TestSlogHandlerNamedTypes(t *testing.T) { + var buf bytes.Buffer + + InitLogfmtForT(t, zapcore.AddSync(&buf)) + + levels := parseSlogLevels("identify=debug") + h := slog.Handler(&slogHandler{levels: levels, level: levels.fallback}) + identify := slog.New(h.WithAttrs([]slog.Attr{slog.String("logger", "identify")})) + + // Logging a slice of named-string types (like []protocol.ID) previously + // panicked because logfmt's AppendReflected asserts interface{} to string. + protocols := []namedString{"/proto/1.0", "/proto/2.0"} + + require.NotPanics(t, func() { + identify.Debug("sending identify", "protocols", protocols) + }) + + require.Contains(t, buf.String(), "sending identify") + require.Contains(t, buf.String(), "/proto/1.0") + + // Float, int, and bool values must also survive logfmt encoding. + buf.Reset() + require.NotPanics(t, func() { + identify.Debug("peer stats", + "score", 3.14, + "conns", 42, + "relay", true, + ) + }) + + require.Contains(t, buf.String(), "3.14") + require.Contains(t, buf.String(), "42") + require.Contains(t, buf.String(), "true") +} diff --git a/app/peerinfo/peerinfopb/v1/peerinfo.pb.go b/app/peerinfo/peerinfopb/v1/peerinfo.pb.go index 4c14dd26f4..9e81604f00 100644 --- a/app/peerinfo/peerinfopb/v1/peerinfo.pb.go +++ b/app/peerinfo/peerinfopb/v1/peerinfo.pb.go @@ -1,6 +1,6 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.11 +// protoc-gen-go v1.36.12 // protoc (unknown) // source: app/peerinfo/peerinfopb/v1/peerinfo.proto diff --git a/app/protonil/testdata/v1/test.pb.go b/app/protonil/testdata/v1/test.pb.go index 2942a60eb7..34916e2a44 100644 --- a/app/protonil/testdata/v1/test.pb.go +++ b/app/protonil/testdata/v1/test.pb.go @@ -1,6 +1,6 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.11 +// protoc-gen-go v1.36.12 // protoc (unknown) // source: app/protonil/testdata/v1/test.proto diff --git a/cluster/cluster_test.go b/cluster/cluster_test.go index d7142a9b2d..3e935a1e46 100644 --- a/cluster/cluster_test.go +++ b/cluster/cluster_test.go @@ -11,10 +11,12 @@ import ( "strings" "testing" + k1 "github.com/decred/dcrd/dcrec/secp256k1/v4" "github.com/stretchr/testify/require" "github.com/obolnetwork/charon/cluster" "github.com/obolnetwork/charon/eth2util" + "github.com/obolnetwork/charon/eth2util/enr" "github.com/obolnetwork/charon/testutil" ) @@ -317,6 +319,135 @@ func TestDefinitionPeers(t *testing.T) { } } +func TestUnmarshalDefinitionDepositAmounts(t *testing.T) { + defJSON := func(version, depositAmounts, compounding string) string { + return `{ + "version": "` + version + `", + "num_validators": 1, + "validators": [{"fee_recipient_address": "", "withdrawal_address": ""}], + "operators": [], + "deposit_amounts": ` + depositAmounts + `, + "compounding": ` + compounding + `}` + } + + const ( + oneGwei = `["1"]` // Below 1ETH minimum. + thirtyTwoEth = `["16000000000","16000000000"]` + fortyEightEth = `["48000000000"]` // Valid only for compounding validators. + ) + + tests := []struct { + name string + json string + errMsg string + }{ + { + name: "v1.8 invalid amounts", + json: defJSON(v1_8, oneGwei, "false"), + errMsg: "invalid deposit amounts", + }, + { + name: "v1.9 invalid amounts", + json: defJSON(v1_9, oneGwei, "false"), + errMsg: "invalid deposit amounts", + }, + { + name: "v1.10 invalid amounts", + json: defJSON(v1_10, oneGwei, "false"), + errMsg: "invalid deposit amounts", + }, + { + name: "v1.11 invalid amounts", + json: defJSON(v1_11, oneGwei, "false"), + errMsg: "invalid deposit amounts", + }, + { + name: "v1.11 amount too large without compounding", + json: defJSON(v1_11, fortyEightEth, "false"), + errMsg: "invalid deposit amounts", + }, + { + name: "v1.11 large amount valid with compounding", + json: defJSON(v1_11, fortyEightEth, "true"), + }, + { + name: "v1.8 valid amounts", + json: defJSON(v1_8, thirtyTwoEth, "false"), + }, + { + name: "v1.8 no amounts", + json: defJSON(v1_8, "[]", "false"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var def cluster.Definition + + err := json.Unmarshal([]byte(tt.json), &def) + if tt.errMsg != "" { + require.ErrorContains(t, err, tt.errMsg) + } else { + require.NoError(t, err) + } + }) + } +} + +func TestDefinitionPeersDuplicatePeerID(t *testing.T) { + newENR := func(key *k1.PrivateKey, opts ...enr.Option) string { + record, err := enr.New(key, opts...) + require.NoError(t, err) + + return record.String() + } + + dupKey, err := k1.GeneratePrivateKey() + require.NoError(t, err) + + // Two distinct ENR strings encoding the same public key, so the same peer ID. + dupENR1 := newENR(dupKey) + dupENR2 := newENR(dupKey, enr.WithTCP(3610)) + require.NotEqual(t, dupENR1, dupENR2) + + uniqueENR := func() string { + key, err := k1.GeneratePrivateKey() + require.NoError(t, err) + + return newENR(key) + } + + tests := []struct { + name string + enrs []string + }{ + { + name: "duplicate peer ids with distinct enrs", + enrs: []string{uniqueENR(), dupENR1, dupENR2, uniqueENR()}, + }, + { + name: "duplicate peer ids at tail", + enrs: []string{uniqueENR(), uniqueENR(), dupENR1, dupENR2}, + }, + { + name: "duplicate identical enrs", + enrs: []string{uniqueENR(), dupENR1, dupENR1, uniqueENR()}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var def cluster.Definition + for _, e := range tt.enrs { + def.Operators = append(def.Operators, cluster.Operator{ENR: e}) + } + + _, err := def.Peers() + require.ErrorContains(t, err, "definition contains duplicate peer ids") + }) + } +} + // TestV1x11SafeSignatures tests that v1.11 supports variable-length signatures (Safe multisig). func TestV1x11SafeSignatures(t *testing.T) { r := rand.New(rand.NewSource(1)) diff --git a/cluster/definition.go b/cluster/definition.go index afa6eaf9b1..3e98c81e69 100644 --- a/cluster/definition.go +++ b/cluster/definition.go @@ -386,14 +386,10 @@ func validateSignatureLength(version string, sig []byte, fieldName string) error func (d Definition) Peers() ([]p2p.Peer, error) { var resp []p2p.Peer - dedup := make(map[string]bool) - for i, operator := range d.Operators { - if dedup[operator.ENR] { - return nil, errors.New("definition contains duplicate peer enrs", z.Str("enr", operator.ENR)) - } - - dedup[operator.ENR] = true + // Dedup by peer ID (not ENR string) since distinct ENRs can encode the same public key. + dedup := make(map[peer.ID]bool) + for i, operator := range d.Operators { record, err := enr.Parse(operator.ENR) if err != nil { return nil, errors.Wrap(err, "decode enr", z.Str("enr", operator.ENR)) @@ -404,6 +400,12 @@ func (d Definition) Peers() ([]p2p.Peer, error) { return nil, err } + if dedup[p.ID] { + return nil, errors.New("definition contains duplicate peer ids", z.Str("enr", operator.ENR), z.Str("peer", p.Name)) + } + + dedup[p.ID] = true + resp = append(resp, p) } @@ -933,7 +935,8 @@ func unmarshalDefinitionV1x8(data []byte) (def Definition, err error) { return Definition{}, errors.New("num_validators does not match validators length") } - if err := deposit.VerifyDepositAmounts(def.DepositAmounts, def.Compounding); err != nil { + // Definition versions prior to v1.10 don't support compounding. + if err := deposit.VerifyDepositAmounts(defJSON.DepositAmounts, false); err != nil { return Definition{}, errors.Wrap(err, "invalid deposit amounts") } @@ -968,7 +971,8 @@ func unmarshalDefinitionV1x9(data []byte) (def Definition, err error) { return Definition{}, errors.New("num_validators does not match validators length") } - if err := deposit.VerifyDepositAmounts(def.DepositAmounts, def.Compounding); err != nil { + // Definition versions prior to v1.10 don't support compounding. + if err := deposit.VerifyDepositAmounts(defJSON.DepositAmounts, false); err != nil { return Definition{}, errors.Wrap(err, "invalid deposit amounts") } @@ -1004,7 +1008,7 @@ func unmarshalDefinitionV1x10to11(data []byte) (def Definition, err error) { return Definition{}, errors.New("num_validators does not match validators length") } - if err := deposit.VerifyDepositAmounts(def.DepositAmounts, def.Compounding); err != nil { + if err := deposit.VerifyDepositAmounts(defJSON.DepositAmounts, defJSON.Compounding); err != nil { return Definition{}, errors.Wrap(err, "invalid deposit amounts") } diff --git a/core/corepb/v1/consensus.pb.go b/core/corepb/v1/consensus.pb.go index ccf1e57e8a..13b2d7262e 100644 --- a/core/corepb/v1/consensus.pb.go +++ b/core/corepb/v1/consensus.pb.go @@ -1,6 +1,6 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.11 +// protoc-gen-go v1.36.12 // protoc (unknown) // source: core/corepb/v1/consensus.proto diff --git a/core/corepb/v1/core.pb.go b/core/corepb/v1/core.pb.go index d5468abfff..2c995e8c55 100644 --- a/core/corepb/v1/core.pb.go +++ b/core/corepb/v1/core.pb.go @@ -1,6 +1,6 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.11 +// protoc-gen-go v1.36.12 // protoc (unknown) // source: core/corepb/v1/core.proto diff --git a/core/corepb/v1/parsigex.pb.go b/core/corepb/v1/parsigex.pb.go index aba78c9ac4..26daecd026 100644 --- a/core/corepb/v1/parsigex.pb.go +++ b/core/corepb/v1/parsigex.pb.go @@ -1,6 +1,6 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.11 +// protoc-gen-go v1.36.12 // protoc (unknown) // source: core/corepb/v1/parsigex.proto diff --git a/core/corepb/v1/priority.pb.go b/core/corepb/v1/priority.pb.go index c3d1271298..5e5b1fc5e1 100644 --- a/core/corepb/v1/priority.pb.go +++ b/core/corepb/v1/priority.pb.go @@ -1,6 +1,6 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.11 +// protoc-gen-go v1.36.12 // protoc (unknown) // source: core/corepb/v1/priority.proto diff --git a/core/gater_test.go b/core/gater_test.go index 446331b69b..eabaf44ca5 100644 --- a/core/gater_test.go +++ b/core/gater_test.go @@ -54,3 +54,49 @@ func TestDutyGater(t *testing.T) { require.False(t, gater(core.Duty{Slot: 2, Type: 100})) require.False(t, gater(core.Duty{Slot: 3, Type: 1000})) } + +// TestDutyGaterInfoSync asserts the gater allows the info sync duties that infosync +// triggers in the last slot of each epoch. The priority protocol gates these duties on +// receipt, so rejecting them here would stall cluster wide priority resolution. +func TestDutyGaterInfoSync(t *testing.T) { + const ( + slotDuration = 12 * time.Second + slotsPerEpoch = 32 + epoch = 100 + ) + + genesis := time.Now() + + bmock, err := beaconmock.New( + t.Context(), + beaconmock.WithGenesisTime(genesis), + beaconmock.WithSlotDuration(slotDuration), + beaconmock.WithSlotsPerEpoch(slotsPerEpoch), + ) + require.NoError(t, err) + + // The slot infosync triggers on, being the last of its epoch. + triggerSlot := uint64(epoch*slotsPerEpoch + slotsPerEpoch - 1) + + tests := []struct { + name string + recvSlot uint64 + }{ + {name: "received in trigger slot", recvSlot: triggerSlot}, + // A peer lagging into the next epoch must still accept it, otherwise clock + // skew across the cluster would drop legitimate exchanges. + {name: "received in next epoch", recvSlot: triggerSlot + 1}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + now := genesis.Add(slotDuration * time.Duration(test.recvSlot)) + + gater, err := core.NewDutyGater(t.Context(), bmock, + core.WithDutyGaterForT(t, func() time.Time { return now }, 2)) + require.NoError(t, err) + + require.True(t, gater(core.NewInfoSyncDuty(triggerSlot))) + }) + } +} diff --git a/core/priority/component.go b/core/priority/component.go index 17c8d190b8..7a2e01b5cf 100644 --- a/core/priority/component.go +++ b/core/priority/component.go @@ -53,6 +53,7 @@ type ScoredPriority struct { func NewComponent(ctx context.Context, p2pNode host.Host, peers []peer.ID, minRequired int, sendFunc p2p.SendReceiveFunc, registerHandlerFunc p2p.RegisterHandlerFunc, consensus Consensus, exchangeTimeout time.Duration, privkey *k1.PrivateKey, deadlineFunc func(duty core.Duty) (time.Time, bool), + gaterFunc core.DutyGaterFunc, ) (*Component, error) { verifier, err := newMsgVerifier(peers) if err != nil { @@ -62,7 +63,7 @@ func NewComponent(ctx context.Context, p2pNode host.Host, peers []peer.ID, minRe deadliner := core.NewDeadliner(ctx, "priority", deadlineFunc) prioritiser := newInternal(p2pNode, peers, minRequired, sendFunc, registerHandlerFunc, - consensus, verifier, exchangeTimeout, deadliner) + consensus, verifier, exchangeTimeout, deadliner, gaterFunc) return &Component{ peerID: p2pNode.ID(), diff --git a/core/priority/prioritiser.go b/core/priority/prioritiser.go index 3be58117bc..b905241c7a 100644 --- a/core/priority/prioritiser.go +++ b/core/priority/prioritiser.go @@ -78,17 +78,17 @@ type request struct { func NewForT(_ *testing.T, p2pNode host.Host, peers []peer.ID, minRequired int, sendFunc p2p.SendReceiveFunc, registerHandlerFunc p2p.RegisterHandlerFunc, consensus Consensus, msgValidator msgValidator, exchangeTimeout time.Duration, - deadliner core.Deadliner, + deadliner core.Deadliner, gaterFunc core.DutyGaterFunc, ) *Prioritiser { return newInternal(p2pNode, peers, minRequired, sendFunc, registerHandlerFunc, - consensus, msgValidator, exchangeTimeout, deadliner) + consensus, msgValidator, exchangeTimeout, deadliner, gaterFunc) } // newInternal returns a new prioritiser, it is the constructor. func newInternal(p2pNode host.Host, peers []peer.ID, minRequired int, sendFunc p2p.SendReceiveFunc, registerHandlerFunc p2p.RegisterHandlerFunc, consensus Consensus, msgValidator msgValidator, - exchangeTimeout time.Duration, deadliner core.Deadliner, + exchangeTimeout time.Duration, deadliner core.Deadliner, gaterFunc core.DutyGaterFunc, ) *Prioritiser { // Create log filters noSupportFilters := make(map[peer.ID]z.Field) @@ -105,6 +105,7 @@ func newInternal(p2pNode host.Host, peers []peer.ID, minRequired int, msgValidator: msgValidator, exchangeTimeout: exchangeTimeout, deadliner: deadliner, + gaterFunc: gaterFunc, quit: make(chan struct{}), noSupportFilters: noSupportFilters, skipAllFilter: log.Filter(), @@ -163,6 +164,7 @@ type Prioritiser struct { peers []peer.ID consensus Consensus msgValidator msgValidator + gaterFunc core.DutyGaterFunc subs []subscriber noSupportFilters map[peer.ID]z.Field skipAllFilter z.Field @@ -222,18 +224,24 @@ func (p *Prioritiser) handleRequest(ctx context.Context, pID peer.ID, msg *pbv1. return nil, errors.Wrap(err, "invalid priority message") } - response := make(chan *pbv1.PriorityMsg, 1) // Ensure responding goroutine never blocks. - req := request{ - Msg: msg, - Response: response, - } - duty := core.DutyFromProto(msg.GetDuty()) + // Gate before any per-duty state is allocated below, otherwise a peer can retain + // unbounded deadliner and request buffer entries by varying the duty slot. + if !p.gaterFunc(duty) { + return nil, errors.New("invalid duty", z.Any("duty", duty)) + } + if status := p.deadliner.Add(duty); status == core.DeadlineExpired || status == core.DeadlineExempt { return nil, errors.New("duty expired or exempt", z.Any("duty", duty)) } + response := make(chan *pbv1.PriorityMsg, 1) // Ensure responding goroutine never blocks. + req := request{ + Msg: msg, + Response: response, + } + reqBuffer := p.getReqBuffer(duty) select { diff --git a/core/priority/prioritiser_internal_test.go b/core/priority/prioritiser_internal_test.go index bf575e2de3..f2b0c7069b 100644 --- a/core/priority/prioritiser_internal_test.go +++ b/core/priority/prioritiser_internal_test.go @@ -3,13 +3,23 @@ package priority import ( + "context" "encoding/hex" + "slices" + "sync" "testing" + "time" + "github.com/libp2p/go-libp2p/core/host" + "github.com/libp2p/go-libp2p/core/peer" + "github.com/libp2p/go-libp2p/core/protocol" "github.com/stretchr/testify/require" "google.golang.org/protobuf/proto" + "github.com/obolnetwork/charon/core" pbv1 "github.com/obolnetwork/charon/core/corepb/v1" + "github.com/obolnetwork/charon/p2p" + "github.com/obolnetwork/charon/testutil" ) func TestHashProto(t *testing.T) { @@ -63,3 +73,144 @@ func TestHashProto(t *testing.T) { }) } } + +// gaterSlot is the highest duty slot allowed by the gater used in the tests below. +const gaterSlot = 100 + +// TestHandleRequestGatesDuty asserts a gated duty is rejected before any per-duty +// state is allocated for it, while an allowed duty still reaches the deadliner and +// gets a request buffer. +func TestHandleRequestGatesDuty(t *testing.T) { + tests := []struct { + name string + slot uint64 + wantErr string + wantState bool + }{ + { + name: "gated far future duty", + slot: gaterSlot + 1, + wantErr: "invalid duty", + }, + { + name: "allowed duty", + slot: gaterSlot, + // No instance runs for an unsolicited request, so it blocks until the context expires. + wantErr: "timeout waiting for proposed priorities", + wantState: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + pID, deadliner, p := newGatedPrioritiser(t) + + ctx, cancel := context.WithTimeout(t.Context(), 100*time.Millisecond) + defer cancel() + + _, err := p.handleRequest(ctx, pID, infoSyncMsg(pID, test.slot)) + require.ErrorContains(t, err, test.wantErr) + + p.reqMu.Lock() + gotBuffers := len(p.reqBuffers) + p.reqMu.Unlock() + + if test.wantState { + require.Len(t, deadliner.Added(), 1) + require.Equal(t, 1, gotBuffers) + } else { + require.Empty(t, deadliner.Added(), "gated duty must not reach the deadliner") + require.Zero(t, gotBuffers, "gated duty must not retain a request buffer") + } + }) + } +} + +// TestHandleRequestFloodGated asserts a peer flooding distinct far-future duty slots +// retains no per-duty state. Ungated, each distinct slot leaked a deadliner entry that +// only expires at its (far future) deadline plus a request buffer keyed by that duty. +func TestHandleRequestFloodGated(t *testing.T) { + pID, deadliner, p := newGatedPrioritiser(t) + + for i := range uint64(100) { + // Gated requests return immediately. The timeout only bounds an ungated + // request, which blocks forever waiting on an instance that never runs. + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Millisecond) + + _, err := p.handleRequest(ctx, pID, infoSyncMsg(pID, gaterSlot+1+i)) + + cancel() + + require.ErrorContains(t, err, "invalid duty") + } + + p.reqMu.Lock() + defer p.reqMu.Unlock() + + require.Empty(t, p.reqBuffers) + require.Empty(t, deadliner.Added()) +} + +// newGatedPrioritiser returns a prioritiser gating duties above gaterSlot, along with +// the peer ID it accepts requests from and the deadliner it was wired with. +func newGatedPrioritiser(t *testing.T) (peer.ID, *recordingDeadliner, *Prioritiser) { + t.Helper() + + pID, err := p2p.PeerIDFromKey(testutil.GenerateInsecureK1Key(t, 0).PubKey()) + require.NoError(t, err) + + deadliner := new(recordingDeadliner) + + p := newInternal(nil, []peer.ID{pID}, 1, nil, nopRegisterHandler, nopConsensus{}, + func(*pbv1.PriorityMsg) error { return nil }, time.Hour, deadliner, + func(duty core.Duty) bool { return duty.Slot <= gaterSlot }) + + return pID, deadliner, p +} + +func infoSyncMsg(pID peer.ID, slot uint64) *pbv1.PriorityMsg { + return &pbv1.PriorityMsg{ + Duty: core.DutyToProto(core.NewInfoSyncDuty(slot)), + PeerId: pID.String(), + } +} + +// recordingDeadliner records the duties added to it. It implements core.Deadliner. +type recordingDeadliner struct { + mu sync.Mutex + added []core.Duty +} + +func (d *recordingDeadliner) Add(duty core.Duty) core.DeadlineStatus { + d.mu.Lock() + defer d.mu.Unlock() + + d.added = append(d.added, duty) + + return core.DeadlineScheduled +} + +func (*recordingDeadliner) C() <-chan core.Duty { return nil } + +func (d *recordingDeadliner) Added() []core.Duty { + d.mu.Lock() + defer d.mu.Unlock() + + return slices.Clone(d.added) +} + +// nopConsensus implements Consensus and does nothing. +type nopConsensus struct{} + +func (nopConsensus) ProposePriority(context.Context, core.Duty, *pbv1.PriorityResult) error { + return nil +} + +func (nopConsensus) SubscribePriority(func(context.Context, core.Duty, *pbv1.PriorityResult) error) { +} + +// nopRegisterHandler implements p2p.RegisterHandlerFunc and registers nothing. +func nopRegisterHandler(string, host.Host, protocol.ID, func() proto.Message, + p2p.HandlerFunc, ...p2p.SendRecvOption, +) { +} diff --git a/core/priority/prioritiser_test.go b/core/priority/prioritiser_test.go index 4f88a32c98..9d0ad7a74e 100644 --- a/core/priority/prioritiser_test.go +++ b/core/priority/prioritiser_test.go @@ -69,7 +69,7 @@ func TestPrioritiser(t *testing.T) { } prio := priority.NewForT(t, tcpNode, peers, n, p2p.SendReceive, p2p.RegisterHandler, - consensus, msgValidator, time.Hour, deadliner) + consensus, msgValidator, time.Hour, deadliner, allowAllDuties) prio.Subscribe(func(_ context.Context, duty core.Duty, result *pbv1.PriorityResult) error { require.Len(t, result.GetTopics(), 1) @@ -325,9 +325,12 @@ func newTestPrioritiser(t *testing.T, p2pNode host.Host, peers []peer.ID, send p t.Helper() return priority.NewForT(t, p2pNode, peers, len(peers), send, register, consensus, - func(*pbv1.PriorityMsg) error { return nil }, time.Hour, deadliner) + func(*pbv1.PriorityMsg) error { return nil }, time.Hour, deadliner, allowAllDuties) } +// allowAllDuties is a core.DutyGaterFunc that gates nothing. +func allowAllDuties(core.Duty) bool { return true } + // testConsensus is a mock consensus implementation that "decides" on the first proposal. // It also expects all proposals to be identical. type testConsensus struct { diff --git a/core/validatorapi/validatorapi.go b/core/validatorapi/validatorapi.go index 2587fdd376..63162d0d94 100644 --- a/core/validatorapi/validatorapi.go +++ b/core/validatorapi/validatorapi.go @@ -1050,7 +1050,10 @@ func (c Component) SyncCommitteeSelections(ctx context.Context, opts *eth2api.Sy // selections don't collide on the pubkey-keyed set. psigsBySlotSubcomm := make(map[slotSubcomm]core.ParSignedDataSet) - for _, selection := range opts.Selections { + // Resolve pubkeys upfront so the response can be built in request order. + pubkeys := make([]core.PubKey, len(opts.Selections)) + + for i, selection := range opts.Selections { eth2Pubkey, ok := vals[selection.ValidatorIndex] if !ok { return nil, errors.New("validator not found") @@ -1061,6 +1064,8 @@ func (c Component) SyncCommitteeSelections(ctx context.Context, opts *eth2api.Sy return nil, err } + pubkeys[i] = pubkey + parSigData := core.NewPartialSignedSyncCommitteeSelection(selection, c.shareIdx) // Verify selection proof. @@ -1087,24 +1092,25 @@ func (c Component) SyncCommitteeSelections(ctx context.Context, opts *eth2api.Sy } } - var resp []*eth2v1.SyncCommitteeSelection + // Build response in the same order as the request so index-matching VCs + // (e.g. prysm) associate aggregated proofs with the correct subcommittee. + resp := make([]*eth2v1.SyncCommitteeSelection, 0, len(opts.Selections)) - for key, data := range psigsBySlotSubcomm { - duty := core.NewPrepareSyncContributionDuty(uint64(key.Slot)) - for pk := range data { - // Query aggregated sync committee selection from aggsigdb for each duty, public key and subcommittee (this is blocking). - s, err := c.awaitAggSigDBFunc(ctx, duty, pk, key.SubcommIdx) - if err != nil { - return nil, err - } + for i, selection := range opts.Selections { + duty := core.NewPrepareSyncContributionDuty(uint64(selection.Slot)) + subcommIdx := core.SubcommitteeIndex(selection.SubcommitteeIndex) - sub, ok := s.(core.SyncCommitteeSelection) - if !ok { - return nil, errors.New("invalid sync committee selection") - } + s, err := c.awaitAggSigDBFunc(ctx, duty, pubkeys[i], subcommIdx) + if err != nil { + return nil, err + } - resp = append(resp, &sub.SyncCommitteeSelection) + sub, ok := s.(core.SyncCommitteeSelection) + if !ok { + return nil, errors.New("invalid sync committee selection") } + + resp = append(resp, &sub.SyncCommitteeSelection) } return wrapResponse(resp), nil diff --git a/core/validatorapi/validatorapi_test.go b/core/validatorapi/validatorapi_test.go index e347e6c455..a57018b8ea 100644 --- a/core/validatorapi/validatorapi_test.go +++ b/core/validatorapi/validatorapi_test.go @@ -2431,14 +2431,8 @@ func TestComponent_AggregateSyncCommitteeSelectionsVerify(t *testing.T) { } require.Equal(t, expect, merged) - got := eth2Resp.Data - - // Sort by VIdx before comparing. - sort.Slice(got, func(i, j int) bool { - return got[i].ValidatorIndex < got[j].ValidatorIndex - }) - - require.Equal(t, selections, got) + // Response must preserve request order (prysm matches by index). + require.Equal(t, selections, eth2Resp.Data) } // TestComponent_SyncCommitteeSelectionsMultiSubcommittee exercises the bug scenario: @@ -2527,7 +2521,82 @@ func TestComponent_SyncCommitteeSelectionsMultiSubcommittee(t *testing.T) { require.Contains(t, stored, uint64(subcommA)) require.Contains(t, stored, uint64(subcommB)) - require.ElementsMatch(t, selections, eth2Resp.Data) + // Response must preserve request order (prysm matches by index). + require.Equal(t, selections, eth2Resp.Data) +} + +// TestComponent_SyncCommitteeSelectionsResponseOrder verifies that the response +// preserves request order. Prysm matches response[i] to request[i] by index; +// Go map iteration randomises order, so the response must be built from the +// request slice, not from the internal map. +func TestComponent_SyncCommitteeSelectionsResponseOrder(t *testing.T) { + const ( + slot = 0 + shareIdx = 1 + vIdx = 1 + ) + + ctx := context.Background() + + valSet, err := beaconmock.ValidatorSetA.Clone() + require.NoError(t, err) + + secret, err := tbls.GenerateSecretKey() + require.NoError(t, err) + + pubkey, err := tbls.SecretToPublicKey(secret) + require.NoError(t, err) + + pk, err := core.PubKeyFromBytes(pubkey[:]) + require.NoError(t, err) + + valSet[vIdx].Validator.PublicKey = eth2p0.BLSPubKey(pubkey) + + bmock, err := beaconmock.New(t.Context(), beaconmock.WithValidatorSet(valSet)) + require.NoError(t, err) + + newSelection := func(subcommIdx uint64) *eth2v1.SyncCommitteeSelection { + sel := testutil.RandomSyncCommitteeSelection() + sel.ValidatorIndex = valSet[vIdx].Index + sel.Slot = slot + sel.SubcommitteeIndex = subcommIdx + sel.SelectionProof = syncCommSelectionProof(t, bmock, secret, slot, subcommIdx) + + return sel + } + + // Send subcommittees in REVERSE order (3,2,1,0) — any iteration over a + // map keyed by ascending subcommittee index would produce 0,1,2,3. + selections := []*eth2v1.SyncCommitteeSelection{ + newSelection(3), newSelection(2), newSelection(1), newSelection(0), + } + + allPubSharesByKey := map[core.PubKey]map[int]tbls.PublicKey{pk: {shareIdx: pubkey}} + + vapi, err := validatorapi.NewComponent(bmock, allPubSharesByKey, shareIdx, nil, false, 30000000) + require.NoError(t, err) + + vapi.RegisterAwaitAggSigDB(func(_ context.Context, duty core.Duty, gotPk core.PubKey, subcommIdx core.SubcommitteeIndex) (core.SignedData, error) { + for _, sel := range selections { + if sel.SubcommitteeIndex == uint64(subcommIdx) { + return core.NewSyncCommitteeSelection(sel), nil + } + } + + return nil, errors.New("selection not found") + }) + + vapi.Subscribe(func(context.Context, core.Duty, core.ParSignedDataSet) error { + return nil + }) + + eth2Resp, err := vapi.SyncCommitteeSelections(ctx, ð2api.SyncCommitteeSelectionsOpts{Selections: selections}) + require.NoError(t, err) + + for i, got := range eth2Resp.Data { + require.Equal(t, selections[i].SubcommitteeIndex, got.SubcommitteeIndex, + "response[%d]: expected subcommittee %d, got %d", i, selections[i].SubcommitteeIndex, got.SubcommitteeIndex) + } } // syncCommSelectionProof returns the selection_proof corresponding to the provided altair.ContributionAndProof. diff --git a/dkg/bcast/client.go b/dkg/bcast/client.go index 70b6dc118f..25c410faf8 100644 --- a/dkg/bcast/client.go +++ b/dkg/bcast/client.go @@ -51,7 +51,7 @@ func (c *client) Broadcast(ctx context.Context, msgID string, msg proto.Message) return errors.Wrap(err, "new any") } - hash, err := c.hashFunc(anyMsg) + hash, err := c.hashFunc(msgID, anyMsg) if err != nil { return errors.Wrap(err, "hash any") } diff --git a/dkg/bcast/helpers.go b/dkg/bcast/helpers.go index caeaf3a63b..ac6f4a1228 100644 --- a/dkg/bcast/helpers.go +++ b/dkg/bcast/helpers.go @@ -12,15 +12,18 @@ import ( ) const ( - protocolIDPrefix = "/charon/dkg/bcast/1.0.0" + // Note: v2.0.0 binds signed hashes to the session hash and message ID, + // the version bump makes mixed-version ceremonies fail at stream negotiation + // instead of at signature verification. + protocolIDPrefix = "/charon/dkg/bcast/2.0.0" protocolIDSig = protocolIDPrefix + "/sig" protocolIDMsg = protocolIDPrefix + "/msg" receiveTimeout = time.Minute // Allow for peers to be out of sync, with some sending messages much earlier and having to wait. sendTimeout = receiveTimeout + 2*time.Second // Allow for server to timeout first. ) -// hashFunc is a function that hashes a any-wrapped protobuf message. -type hashFunc func(*anypb.Any) ([]byte, error) +// hashFunc is a function that hashes a message ID and a any-wrapped protobuf message. +type hashFunc func(string, *anypb.Any) ([]byte, error) // Callback is a function that is called when a reliably-broadcast message was successfully received. type Callback func(ctx context.Context, peerID peer.ID, msgID string, msg proto.Message) error diff --git a/dkg/bcast/impl.go b/dkg/bcast/impl.go index d955e64f4e..bd74c7cfdb 100644 --- a/dkg/bcast/impl.go +++ b/dkg/bcast/impl.go @@ -5,6 +5,7 @@ package bcast import ( "context" "crypto/sha256" + "encoding/binary" "sync" k1 "github.com/decred/dcrd/dcrec/secp256k1/v4" @@ -55,31 +56,44 @@ func (c *Component) Broadcast(ctx context.Context, msgID string, msg proto.Messa } // New registers a new reliable-broadcast server and returns a reliable-broadcast client function. -func New(p2pNode host.Host, peers []peer.ID, secret *k1.PrivateKey) *Component { +// All messages are bound to sessionHash, so signatures from other sessions fail verification. +func New(p2pNode host.Host, peers []peer.ID, secret *k1.PrivateKey, sessionHash []byte) *Component { c := Component{ allowedMsgIDs: map[string]struct{}{}, secret: secret, peers: peers, } + hashFunc := newHashAny(sessionHash) signFunc := c.newK1Signer() - verifyFunc := c.newPeerK1Verifier() + verifyFunc := c.newPeerK1Verifier(hashFunc) - cl := newClient(p2pNode, peers, p2p.SendReceive, p2p.Send, hashAny, signFunc, verifyFunc) + cl := newClient(p2pNode, peers, p2p.SendReceive, p2p.Send, hashFunc, signFunc, verifyFunc) c.broadcastFunc = cl.Broadcast - c.srv = newServer(p2pNode, signFunc, hashAny, verifyFunc) + c.srv = newServer(p2pNode, signFunc, hashFunc, verifyFunc) return &c } -// hashAny is a function that hashes a any-wrapped protobuf message. -func hashAny(anyPB *anypb.Any) ([]byte, error) { - h := sha256.New() - _, _ = h.Write([]byte(anyPB.GetTypeUrl())) - _, _ = h.Write(anyPB.GetValue()) +// newHashAny returns a function that hashes a message ID and a any-wrapped protobuf +// message, binding them to the session hash. Fields are length-prefixed to +// avoid ambiguous concatenation. +func newHashAny(sessionHash []byte) hashFunc { + return func(msgID string, anyPB *anypb.Any) ([]byte, error) { + h := sha256.New() + for _, field := range [][]byte{sessionHash, []byte(msgID), []byte(anyPB.GetTypeUrl()), anyPB.GetValue()} { + if err := binary.Write(h, binary.BigEndian, uint64(len(field))); err != nil { + return nil, errors.Wrap(err, "write field length") + } - return h.Sum(nil), nil + if _, err := h.Write(field); err != nil { + return nil, errors.Wrap(err, "write field") + } + } + + return h.Sum(nil), nil + } } // newK1Signer returns a function that signs a hash using the given private key. @@ -94,7 +108,7 @@ func (c *Component) newK1Signer() func(string, []byte) ([]byte, error) { } // newPeerK1Verifier returns a function that verifies a hash using the given peer IDs (public keys). -func (c *Component) newPeerK1Verifier() func(string, *anypb.Any, [][]byte) error { +func (c *Component) newPeerK1Verifier(hashFunc hashFunc) func(string, *anypb.Any, [][]byte) error { return func(msgID string, anyPB *anypb.Any, sigs [][]byte) error { if len(sigs) != len(c.peers) { return errors.New("invalid number of signatures") @@ -104,7 +118,7 @@ func (c *Component) newPeerK1Verifier() func(string, *anypb.Any, [][]byte) error return errors.New("invalid message id") } - hash, err := hashAny(anyPB) + hash, err := hashFunc(msgID, anyPB) if err != nil { return errors.Wrap(err, "hash any") } diff --git a/dkg/bcast/impl_internal_test.go b/dkg/bcast/impl_internal_test.go new file mode 100644 index 0000000000..eb046efe7c --- /dev/null +++ b/dkg/bcast/impl_internal_test.go @@ -0,0 +1,42 @@ +// Copyright © 2022-2026 Obol Labs Inc. Licensed under the terms of a Business Source License 1.1 + +package bcast + +import ( + "testing" + + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/types/known/anypb" +) + +func TestNewHashAny(t *testing.T) { + anyPB := &anypb.Any{TypeUrl: "typeURL", Value: []byte("value")} + + hash := func(session []byte, msgID string, anyPB *anypb.Any) []byte { + h, err := newHashAny(session)(msgID, anyPB) + require.NoError(t, err) + + return h + } + + base := hash([]byte("session"), "msgID", anyPB) + + // Deterministic. + require.Equal(t, base, hash([]byte("session"), "msgID", anyPB)) + + // Sensitive to each field. + require.NotEqual(t, base, hash([]byte("other session"), "msgID", anyPB)) + require.NotEqual(t, base, hash([]byte("session"), "other msgID", anyPB)) + require.NotEqual(t, base, hash([]byte("session"), "msgID", &anypb.Any{TypeUrl: "other typeURL", Value: []byte("value")})) + require.NotEqual(t, base, hash([]byte("session"), "msgID", &anypb.Any{TypeUrl: "typeURL", Value: []byte("other value")})) + + // Length prefixes prevent ambiguous concatenation of adjacent fields. + require.NotEqual(t, + hash([]byte("sessionX"), "msgID", anyPB), + hash([]byte("session"), "XmsgID", anyPB), + ) + require.NotEqual(t, + hash([]byte("session"), "msgIDX", anyPB), + hash([]byte("session"), "msgID", &anypb.Any{TypeUrl: "XtypeURL", Value: []byte("value")}), + ) +} diff --git a/dkg/bcast/impl_test.go b/dkg/bcast/impl_test.go index a4cfefb6fa..f50b9d3531 100644 --- a/dkg/bcast/impl_test.go +++ b/dkg/bcast/impl_test.go @@ -84,7 +84,7 @@ func TestBCast(t *testing.T) { return nil } - bcastFunc := bcast.New(tcpNodes[i], peers, secrets[i]) + bcastFunc := bcast.New(tcpNodes[i], peers, secrets[i], []byte("session hash")) bcastFunc.RegisterMessageIDFuncs(msgID1, callback, checkMessage) bcastFunc.RegisterMessageIDFuncs(msgID2, callback, checkMessage) @@ -149,3 +149,61 @@ func TestBCast(t *testing.T) { require.NoError(t, err) assertResults(t, p0Result, peers[0]) } + +// TestBCastSessionHashMismatch ensures that messages signed in one session +// cannot be verified in another, binding broadcasts to the cluster session. +func TestBCastSessionHashMismatch(t *testing.T) { + const ( + n = 2 + msgID = "msgID" + ) + + var ( + ctx = context.Background() + secrets []*k1.PrivateKey + tcpNodes []host.Host + peers []peer.ID + bcasts []bcast.BroadcastFunc + ) + + for range n { + secret, err := k1.GeneratePrivateKey() + require.NoError(t, err) + + secrets = append(secrets, secret) + + tcpNode := testutil.CreateHostWithIdentity(t, testutil.AvailableAddr(t), secret) + tcpNodes = append(tcpNodes, tcpNode) + + peers = append(peers, tcpNode.ID()) + } + + for i := range n { + for j := range n { + tcpNodes[i].Peerstore().AddAddrs(tcpNodes[j].ID(), tcpNodes[j].Addrs(), peerstore.PermanentAddrTTL) + } + } + + callback := func(context.Context, peer.ID, string, proto.Message) error { + return nil + } + checkMessage := func(_ context.Context, _ peer.ID, msgAny *anypb.Any) error { + var ts timestamppb.Timestamp + if err := msgAny.UnmarshalTo(&ts); err != nil { + return errors.Wrap(err, "anypb error") + } + + return nil + } + + // Each peer runs with a different session hash. + for i := range n { + bcastFunc := bcast.New(tcpNodes[i], peers, secrets[i], []byte{byte(i)}) + bcastFunc.RegisterMessageIDFuncs(msgID, callback, checkMessage) + bcasts = append(bcasts, bcastFunc.Broadcast) + } + + // Signatures from a peer in a different session must not verify. + err := bcasts[0](ctx, msgID, timestamppb.Now()) + require.ErrorContains(t, err, "verify signatures") +} diff --git a/dkg/bcast/server.go b/dkg/bcast/server.go index 73c961703a..184d46657d 100644 --- a/dkg/bcast/server.go +++ b/dkg/bcast/server.go @@ -117,7 +117,7 @@ func (s *server) handleSigRequest(ctx context.Context, pID peer.ID, m proto.Mess return nil, false, errors.Wrap(err, "signature request message check") } - reqMessageHash, err := s.hashFunc(req.GetMessage()) + reqMessageHash, err := s.hashFunc(req.GetId(), req.GetMessage()) if err != nil { return nil, false, errors.Wrap(err, "hash any") } diff --git a/dkg/dkg.go b/dkg/dkg.go index f7b9a03be4..e1c9fa9d88 100644 --- a/dkg/dkg.go +++ b/dkg/dkg.go @@ -172,6 +172,10 @@ func Run(ctx context.Context, conf Config) (err error) { return errors.New("only v1.6.0 and newer cluster definition versions supported") } + if err := checkThreshold(ctx, def.Threshold, len(def.Operators)); err != nil { + return err + } + if err := validateKeymanagerFlags(ctx, conf.KeymanagerAddr, conf.KeymanagerAuthToken); err != nil { return err } @@ -266,7 +270,7 @@ func Run(ctx context.Context, conf Config) (err error) { } // Register libp2p handlers - caster := bcast.New(p2pNode, peerIDs, key) + caster := bcast.New(p2pNode, peerIDs, key, def.DefinitionHash) // register bcast callbacks for frostp2p tp, err := newFrostP2P(p2pNode, peerMap, caster, def.Threshold, newValidators) @@ -1279,6 +1283,27 @@ func writeLockToAPI(ctx context.Context, publishAddr string, lock cluster.Lock, return cl.LaunchpadURLForLock(lock), nil } +// checkThreshold returns an error if the threshold is out of bounds and +// logs a warning if it differs from the recommended value for the number of operators. +func checkThreshold(ctx context.Context, threshold, numOperators int) error { + const minThreshold = 2 + + if threshold < minThreshold { + return errors.New("threshold below minimum", z.Int("threshold", threshold), z.Int("min", minThreshold)) + } + + if threshold > numOperators { + return errors.New("threshold exceeds number of operators", z.Int("threshold", threshold), z.Int("operators", numOperators)) + } + + if safe := cluster.Threshold(numOperators); threshold != safe { + log.Warn(ctx, "Cluster definition threshold differs from recommended value, this will affect cluster safety", + nil, z.Int("threshold", threshold), z.Int("safe_threshold", safe)) + } + + return nil +} + // validateKeymanagerFlags returns an error if one keymanager flag is present but the other is not. func validateKeymanagerFlags(ctx context.Context, addr, authToken string) error { if addr != "" && authToken == "" { diff --git a/dkg/dkg_internal_test.go b/dkg/dkg_internal_test.go index cc7d7b3dea..140404e95e 100644 --- a/dkg/dkg_internal_test.go +++ b/dkg/dkg_internal_test.go @@ -8,7 +8,9 @@ import ( eth2p0 "github.com/attestantio/go-eth2-client/spec/phase0" "github.com/stretchr/testify/require" + "go.uber.org/zap/zaptest" + "github.com/obolnetwork/charon/app/log" "github.com/obolnetwork/charon/core" "github.com/obolnetwork/charon/dkg/share" "github.com/obolnetwork/charon/eth2util" @@ -200,3 +202,65 @@ func TestValidateKeymanagerFlags(t *testing.T) { }) } } + +func TestCheckThreshold(t *testing.T) { + tests := []struct { + name string + threshold int + numOperators int + errMsg string + warnMsg string + }{ + { + name: "safe threshold", + threshold: 3, + numOperators: 4, + }, + { + name: "unsafe low threshold", + threshold: 2, + numOperators: 4, + warnMsg: "Cluster definition threshold differs from recommended value", + }, + { + name: "unsafe high threshold", + threshold: 4, + numOperators: 4, + warnMsg: "Cluster definition threshold differs from recommended value", + }, + { + name: "threshold below minimum", + threshold: 1, + numOperators: 4, + errMsg: "threshold below minimum", + }, + { + name: "threshold exceeds number of operators", + threshold: 5, + numOperators: 4, + errMsg: "threshold exceeds number of operators", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf zaptest.Buffer + + log.InitLogfmtForT(t, &buf) + + err := checkThreshold(context.Background(), tt.threshold, tt.numOperators) + if tt.errMsg != "" { + require.ErrorContains(t, err, tt.errMsg) + return + } + + require.NoError(t, err) + + if tt.warnMsg != "" { + require.Contains(t, buf.String(), tt.warnMsg) + } else { + require.Empty(t, buf.String()) + } + }) + } +} diff --git a/dkg/dkgpb/v1/bcast.pb.go b/dkg/dkgpb/v1/bcast.pb.go index 4892f487a8..629ce73d8c 100644 --- a/dkg/dkgpb/v1/bcast.pb.go +++ b/dkg/dkgpb/v1/bcast.pb.go @@ -1,6 +1,6 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.11 +// protoc-gen-go v1.36.12 // protoc (unknown) // source: dkg/dkgpb/v1/bcast.proto diff --git a/dkg/dkgpb/v1/frost.pb.go b/dkg/dkgpb/v1/frost.pb.go index 44a0034e68..4627404715 100644 --- a/dkg/dkgpb/v1/frost.pb.go +++ b/dkg/dkgpb/v1/frost.pb.go @@ -1,6 +1,6 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.11 +// protoc-gen-go v1.36.12 // protoc (unknown) // source: dkg/dkgpb/v1/frost.proto diff --git a/dkg/dkgpb/v1/nodesigs.pb.go b/dkg/dkgpb/v1/nodesigs.pb.go index b73e6a82c9..4ea4bfa1c0 100644 --- a/dkg/dkgpb/v1/nodesigs.pb.go +++ b/dkg/dkgpb/v1/nodesigs.pb.go @@ -1,6 +1,6 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.11 +// protoc-gen-go v1.36.12 // protoc (unknown) // source: dkg/dkgpb/v1/nodesigs.proto diff --git a/dkg/dkgpb/v1/pedersen.pb.go b/dkg/dkgpb/v1/pedersen.pb.go index f6633e4a5d..af4b8a326e 100644 --- a/dkg/dkgpb/v1/pedersen.pb.go +++ b/dkg/dkgpb/v1/pedersen.pb.go @@ -1,6 +1,6 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.11 +// protoc-gen-go v1.36.12 // protoc (unknown) // source: dkg/dkgpb/v1/pedersen.proto diff --git a/dkg/dkgpb/v1/sync.pb.go b/dkg/dkgpb/v1/sync.pb.go index 7ff9286d9e..6998cf0374 100644 --- a/dkg/dkgpb/v1/sync.pb.go +++ b/dkg/dkgpb/v1/sync.pb.go @@ -1,6 +1,6 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.11 +// protoc-gen-go v1.36.12 // protoc (unknown) // source: dkg/dkgpb/v1/sync.proto diff --git a/dkg/nodesigs_internal_test.go b/dkg/nodesigs_internal_test.go index bd1c6c7d1c..ee072146eb 100644 --- a/dkg/nodesigs_internal_test.go +++ b/dkg/nodesigs_internal_test.go @@ -69,7 +69,7 @@ func TestSigsExchange(t *testing.T) { } for i := range n { - component := bcast.New(tcpNodes[i], peers, secrets[i]) + component := bcast.New(tcpNodes[i], peers, secrets[i], []byte("session hash")) nsigs = append(nsigs, newNodeSigBcast( clusterPeers, cluster.NodeIdx{PeerIdx: i}, @@ -160,7 +160,7 @@ func TestSigsCallbacks(t *testing.T) { } } - component := bcast.New(tcpNodes[0], peers, secrets[0]) + component := bcast.New(tcpNodes[0], peers, secrets[0], []byte("session hash")) ns := newNodeSigBcast( clusterPeers, diff --git a/dkg/pedersen/dkg.go b/dkg/pedersen/dkg.go index 2133755d3f..399af573e8 100644 --- a/dkg/pedersen/dkg.go +++ b/dkg/pedersen/dkg.go @@ -118,6 +118,8 @@ func RunDKG(ctx context.Context, config *Config, board *Board, numVals int) ([]s shares = append(shares, share) } + + log.Info(ctx, "DKG for key successful", z.Int("key", i+1), z.Int("total", numVals)) } log.Info(ctx, "Pedersen DKG completed.") diff --git a/dkg/pedersen/logger.go b/dkg/pedersen/logger.go index 31c4741494..988b7b2271 100644 --- a/dkg/pedersen/logger.go +++ b/dkg/pedersen/logger.go @@ -31,7 +31,7 @@ func (l *kyberLogger) Error(keyvals ...any) { func (l *kyberLogger) Info(keyvals ...any) { msg, _ := concatKeyVals(keyvals) - log.Info(l.logCtx, msg) + log.Debug(l.logCtx, msg) } func concatKeyVals(keyvals []any) (str string, err error) { diff --git a/dkg/pedersen/reshare.go b/dkg/pedersen/reshare.go index 0f2c7e95ab..f11dfc33b2 100644 --- a/dkg/pedersen/reshare.go +++ b/dkg/pedersen/reshare.go @@ -310,6 +310,8 @@ func RunReshareDKG(ctx context.Context, config *Config, board *Board, shares []s newShares = append(newShares, newShare) } } + + log.Info(ctx, "Reshare for key successful", z.Int("key", shareNum+1), z.Int("total", config.Reshare.TotalShares)) } log.Info(ctx, "Pedersen reshare completed.") diff --git a/dkg/pedersen/testutils.go b/dkg/pedersen/testutils.go index 3ae158577c..c7c672a680 100644 --- a/dkg/pedersen/testutils.go +++ b/dkg/pedersen/testutils.go @@ -74,7 +74,7 @@ func ConnectTestNodes(t *testing.T, nodes []*TestNode) { func (n *TestNode) InitBoard(t *testing.T, threshold int, peers []peer.ID, peerMap map[peer.ID]cluster.NodeIdx, session []byte) { t.Helper() - bc := bcast.New(n.NodeHost, peers, n.NodeSecret) + bc := bcast.New(n.NodeHost, peers, n.NodeSecret, session) logCtx := log.WithCtx(t.Context(), z.Int("index", n.NodeIdx.PeerIdx)) n.Config = NewConfig(n.NodeHost.ID(), peerMap, threshold, session, 3*time.Second, nil) n.Board = NewBoard(logCtx, n.NodeHost, n.Config, bc) diff --git a/dkg/protocol_addoperators.go b/dkg/protocol_addoperators.go index 3f8340d044..30c2396557 100644 --- a/dkg/protocol_addoperators.go +++ b/dkg/protocol_addoperators.go @@ -91,7 +91,9 @@ func (p *addOperatorsProtocol) PostInit(ctx context.Context, pctx *ProtocolConte } pctx.SigExchanger = sigEx - pctx.Caster = bcast.New(pctx.ThisNode, pctx.PeerIDs, pctx.ENRPrivateKey) + // Bind broadcasts to the lock hash since it changes with every cluster mutation, + // preventing replay of messages from previous ceremonies of the same cluster. + pctx.Caster = bcast.New(pctx.ThisNode, pctx.PeerIDs, pctx.ENRPrivateKey, pctx.Lock.LockHash) pctx.NodeSigCaster = newNodeSigBcast(pctx.Peers, pctx.ThisNodeIdx, pctx.Caster) newPeerIDs := pctx.PeerIDs[len(pctx.Lock.Operators):] diff --git a/dkg/protocol_removeoperators.go b/dkg/protocol_removeoperators.go index 5d082cd1f2..c855824d81 100644 --- a/dkg/protocol_removeoperators.go +++ b/dkg/protocol_removeoperators.go @@ -150,7 +150,9 @@ func (p *removeOperatorsProtocol) PostInit(ctx context.Context, pctx *ProtocolCo } // The broadcaster is created for all participating nodes, because it is used by the board and the node signature caster. - pctx.Caster = bcast.New(pctx.ThisNode, pctx.PeerIDs, pctx.ENRPrivateKey) + // Bind broadcasts to the lock hash since it changes with every cluster mutation, + // preventing replay of messages from previous ceremonies of the same cluster. + pctx.Caster = bcast.New(pctx.ThisNode, pctx.PeerIDs, pctx.ENRPrivateKey, pctx.Lock.LockHash) pctx.NodeSigCaster = newNodeSigBcast(pctx.Peers, pctx.ThisNodeIdx, pctx.Caster) if !p.oldNode { diff --git a/dkg/protocol_replaceoperator.go b/dkg/protocol_replaceoperator.go index ca0cfca26c..7d6dadc203 100644 --- a/dkg/protocol_replaceoperator.go +++ b/dkg/protocol_replaceoperator.go @@ -109,7 +109,9 @@ func (p *replaceOperatorProtocol) PostInit(ctx context.Context, pctx *ProtocolCo } pctx.SigExchanger = sigEx - pctx.Caster = bcast.New(pctx.ThisNode, pctx.PeerIDs, pctx.ENRPrivateKey) + // Bind broadcasts to the lock hash since it changes with every cluster mutation, + // preventing replay of messages from previous ceremonies of the same cluster. + pctx.Caster = bcast.New(pctx.ThisNode, pctx.PeerIDs, pctx.ENRPrivateKey, pctx.Lock.LockHash) pctx.NodeSigCaster = newNodeSigBcast(pctx.Peers, pctx.ThisNodeIdx, pctx.Caster) // For replace operator: identify the old and new peer IDs at the replacement position. diff --git a/dkg/protocol_reshare.go b/dkg/protocol_reshare.go index 0e6b14b094..23f9e11d76 100644 --- a/dkg/protocol_reshare.go +++ b/dkg/protocol_reshare.go @@ -55,7 +55,9 @@ func (p *reshareProtocol) PostInit(ctx context.Context, pctx *ProtocolContext) e } pctx.SigExchanger = sigEx - pctx.Caster = bcast.New(pctx.ThisNode, pctx.PeerIDs, pctx.ENRPrivateKey) + // Bind broadcasts to the lock hash since it changes with every cluster mutation, + // preventing replay of messages from previous ceremonies of the same cluster. + pctx.Caster = bcast.New(pctx.ThisNode, pctx.PeerIDs, pctx.ENRPrivateKey, pctx.Lock.LockHash) pctx.NodeSigCaster = newNodeSigBcast(pctx.Peers, pctx.ThisNodeIdx, pctx.Caster) pedersenReshareConfig := pedersen.NewReshareConfig(len(pctx.Lock.Validators), pctx.Lock.Threshold, nil, nil) diff --git a/dkg/protocolsteps_internal_test.go b/dkg/protocolsteps_internal_test.go index 85867f1965..ad0508ca92 100644 --- a/dkg/protocolsteps_internal_test.go +++ b/dkg/protocolsteps_internal_test.go @@ -194,7 +194,7 @@ func TestUpdateNodeSignaturesProtocolStep(t *testing.T) { for n := range numNodes { group.Go(func() error { - caster := bcast.New(nodes[n].NodeHost, peers, nodeKeys[n]) + caster := bcast.New(nodes[n].NodeHost, peers, nodeKeys[n], lock.DefinitionHash) nodeSigCaster := newNodeSigBcast(allPeers, cluster.NodeIdx{PeerIdx: n, ShareIdx: n + 1}, caster) step := &updateNodeSignaturesProtocolStep{} diff --git a/go.mod b/go.mod index 1a5fc38cec..16569ed0e3 100644 --- a/go.mod +++ b/go.mod @@ -34,7 +34,7 @@ require ( github.com/spf13/cobra v1.10.2 github.com/spf13/pflag v1.0.10 github.com/spf13/viper v1.21.0 - github.com/stretchr/testify v1.11.1 + github.com/stretchr/testify v1.12.0 github.com/wealdtech/go-eth2-wallet-encryptor-keystorev4 v1.4.1 go.opentelemetry.io/otel v1.45.0 go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.45.0 @@ -45,14 +45,14 @@ require ( go.uber.org/automaxprocs v1.6.0 go.uber.org/goleak v1.3.0 go.uber.org/zap v1.28.0 - golang.org/x/crypto v0.54.0 - golang.org/x/net v0.57.0 + golang.org/x/crypto v0.55.0 + golang.org/x/net v0.58.0 golang.org/x/sync v0.22.0 golang.org/x/term v0.45.0 - golang.org/x/text v0.40.0 + golang.org/x/text v0.41.0 golang.org/x/time v0.15.0 - golang.org/x/tools v0.48.0 - google.golang.org/protobuf v1.36.11 + golang.org/x/tools v0.49.0 + google.golang.org/protobuf v1.36.12 gopkg.in/natefinch/lumberjack.v2 v2.2.1 ) @@ -226,7 +226,6 @@ require ( github.com/pk910/dynamic-ssz v1.3.2 // indirect github.com/pk910/hashtree-bindings v0.2.2 // indirect github.com/pkg/errors v0.9.1 // indirect - github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/prometheus/common v0.70.1 // indirect github.com/prometheus/procfs v0.21.1 // indirect github.com/quic-go/qpack v0.6.0 // indirect @@ -271,9 +270,9 @@ require ( go.uber.org/multierr v1.11.0 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/exp v0.0.0-20260709172345-9ea1abe57597 // indirect - golang.org/x/mod v0.38.0 // indirect + golang.org/x/mod v0.39.0 // indirect golang.org/x/sys v0.47.0 // indirect - golang.org/x/telemetry v0.0.0-20260708182218-49f421fb7959 // indirect + golang.org/x/telemetry v0.0.0-20260811182544-a038080d80e5 // indirect golang.org/x/vuln v1.1.4 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260803160001-6ac0973c030d // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20260803160001-6ac0973c030d // indirect diff --git a/go.sum b/go.sum index 9bb8fcda50..7cd83e0226 100644 --- a/go.sum +++ b/go.sum @@ -555,8 +555,8 @@ github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UV github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= -github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= -github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= +github.com/stretchr/testify v1.12.0 h1:K6Mr6jO9JICuend/5xzTM03ydSV3vdNRYAdPSukj8uI= +github.com/stretchr/testify v1.12.0/go.mod h1:bOYBZb5qJ00vPzWfIqBUZPaxK8jWiXc6d3ErP4Ca9Gw= github.com/subosito/gotenv v1.6.0 h1:9NlTDc1FTs4qu0DDq7AEtTPNw6SVm7uBMsUCUjABIf8= github.com/subosito/gotenv v1.6.0/go.mod h1:Dk4QP5c2W3ibzajGcXpNraDfq2IrhjMIvMSWPKKo0FU= github.com/supranational/blst v0.3.16 h1:bTDadT+3fK497EvLdWRQEjiGnUtzJ7jjIUMF0jqwYhE= @@ -647,14 +647,14 @@ golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8U golang.org/x/crypto v0.0.0-20200602180216-279210d13fed/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/crypto v0.0.0-20210322153248-0c34fe9e7dc2/go.mod h1:T9bdIzuCu7OtxOm1hfPfRQxPLYneinmdGuTeoZ9dtd4= -golang.org/x/crypto v0.54.0 h1:YLIA59K4fiNzHzjnZt2tUJQjQtUWfWbeHBqKtk3eScw= -golang.org/x/crypto v0.54.0/go.mod h1:KWL8ny2AZdGR2cWmzeHrp2azQPGogOv+HeQaVEXC2dk= +golang.org/x/crypto v0.55.0 h1:+KWHjbgOaAQ66dh/YlkZKHlz9ZUlq61AFirAR9ntP8M= +golang.org/x/crypto v0.55.0/go.mod h1:uq0V9dE/fzQuJtbnL+2EhWOE63vo164FY8xqEnV9xis= golang.org/x/exp v0.0.0-20260709172345-9ea1abe57597 h1:qLvzZeaANDgyVOA8pyHCOStGlXn0rseXma+GQjeuv2g= golang.org/x/exp v0.0.0-20260709172345-9ea1abe57597/go.mod h1:EdfpwwqSu+0Li0mzskwHU6FWDV3t9Q+RZDo3QMUtL3Q= golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= -golang.org/x/mod v0.38.0 h1:MECBjubtXD7yj4HrhIUcywNaGeNVUdfVnxmPajOk4yk= -golang.org/x/mod v0.38.0/go.mod h1:V6Xz0pq8TQ3dGqVQ1FVHuelZpAL0uNhSkk9ogYP3c40= +golang.org/x/mod v0.39.0 h1:UF5zwQdCRRUpHfyPwr7d4UrGiVeldIsogtzWVnczL74= +golang.org/x/mod v0.39.0/go.mod h1:bvIbwjQ0HUFFf5AKukeeYQG4ZBUG9yxQbR9aEweIwYY= golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= @@ -665,8 +665,8 @@ golang.org/x/net v0.0.0-20200813134508-3edf25e44fcc/go.mod h1:/O7V0waA8r7cgGh81R golang.org/x/net v0.0.0-20201021035429-f5854403a974/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU= golang.org/x/net v0.0.0-20210119194325-5f4716e94777/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= -golang.org/x/net v0.57.0 h1:K5+3DljvIuDG9/Jv9rvyMywYNFCQ9RSUY6OOTTkT+tE= -golang.org/x/net v0.57.0/go.mod h1:KpXc8iv+r3XplLAG/f7Jsf9RPszJzdR0f58q9vGOuEU= +golang.org/x/net v0.58.0 h1:ynWG7rqYi4ccpTEuPZ2QGWHktVEM9DMCj9yzDE0Q7To= +golang.org/x/net v0.58.0/go.mod h1:YwCddHnFlT7eLQqVprV19OnhLGtc5xOKgE0RyqgfWAU= golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= @@ -692,24 +692,24 @@ golang.org/x/sys v0.0.0-20211117180635-dee7805ff2e1/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= -golang.org/x/telemetry v0.0.0-20260708182218-49f421fb7959 h1:RJhm5l6Fo4rmEIcndxDllNhhf/fAx8qIm4t6A7vpm2A= -golang.org/x/telemetry v0.0.0-20260708182218-49f421fb7959/go.mod h1:LV7u5Oco+Z/g6XI7PqN+EUUUGGkEcmB1uj2ceI0fOVg= +golang.org/x/telemetry v0.0.0-20260811182544-a038080d80e5 h1:ZUSxONxc981v7AW7QUg+I9WwZzSTTJ019ENBYr5pV/Q= +golang.org/x/telemetry v0.0.0-20260811182544-a038080d80e5/go.mod h1:LVehoXe41cL5SCVQilsV7Gg6BNG+Js6P9PhSbYTIUkQ= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.45.0 h1:NwWyBmoJCbfTHpxrWoZ9C6/VxOf7ic219I8xZZFdrf0= golang.org/x/term v0.45.0/go.mod h1:9aqxs0blBcrm/n0L9QW0aRVD+ktan8ssZromtqJC43w= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= -golang.org/x/text v0.40.0 h1:Ub2Z6/xjgF1WrYQz2nuITOEegKFtiIy+rieRJ5lHZKs= -golang.org/x/text v0.40.0/go.mod h1:hpnzDAfGV753zIKo+wk3u1bVKCGPbrnF7+7LBF/UHVY= +golang.org/x/text v0.41.0 h1:vz/seA0lnX87Othu2f/0L24RcgrXD9/YFTSuGjj3rH8= +golang.org/x/text v0.41.0/go.mod h1:jvf1O8ajNzZqhSrQBPbutR/EB83Cc0CFrezNQIwbb5M= golang.org/x/time v0.15.0 h1:bbrp8t3bGUeFOx08pvsMYRTCVSMk89u4tKbNOZbp88U= golang.org/x/time v0.15.0/go.mod h1:Y4YMaQmXwGQZoFaVFk4YpCt4FLQMYKZe9oeV/f4MSno= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.0.0-20200619180055-7c47624df98f/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20210106214847-113979e3529a/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA= -golang.org/x/tools v0.48.0 h1:3+hClM1aLL5mjMKm5ovokw9epgRXPuu2tILgismM6RE= -golang.org/x/tools v0.48.0/go.mod h1:08xX0orndb/F7jJxGDicx061tyd5pcMto75YMAXr6lk= +golang.org/x/tools v0.49.0 h1:3NI7VXzL9+1WZD52Dx2ttoPwD5DWrFGpl9mFZDlmisI= +golang.org/x/tools v0.49.0/go.mod h1:SJNXV9DBKT0UbdttsQjbfJlAE/q+y36++zo3uL3N0Oo= golang.org/x/tools/go/expect v0.1.1-deprecated h1:jpBZDwmgPhXsKZC6WhL20P4b/wmnpsEAGHaNy0n/rJM= golang.org/x/tools/go/expect v0.1.1-deprecated/go.mod h1:eihoPOH+FgIqa3FpoTwguz/bVUSGBlGQU67vpBeOrBY= golang.org/x/tools/go/packages/packagestest v0.1.1-deprecated h1:1h2MnaIAIXISqTFKdENegdpAgUXz6NrPEsbIeWaBRvM= @@ -736,8 +736,8 @@ google.golang.org/protobuf v0.0.0-20200228230310-ab0ca4ff8a60/go.mod h1:cfTl7dwQ google.golang.org/protobuf v1.20.1-0.20200309200217-e05f789c0967/go.mod h1:A+miEFZTKqfCUM6K7xSMQL9OKL/b6hQv+e19PK+JZNE= google.golang.org/protobuf v1.21.0/go.mod h1:47Nbq4nVaFHyn7ilMalzfO3qCViNmqZ2kzikPIcrTAo= google.golang.org/protobuf v1.23.0/go.mod h1:EGpADcykh3NcUnDUJcl1+ZksZNG86OlYog2l/sGQquU= -google.golang.org/protobuf v1.36.11 h1:fV6ZwhNocDyBLK0dj+fg8ektcVegBBuEolpbTQyBNVE= -google.golang.org/protobuf v1.36.11/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco= +google.golang.org/protobuf v1.36.12 h1:pJOKDDOyeXErUroCihFAd5LQuwXBSpVnKGrj5o/fwxc= +google.golang.org/protobuf v1.36.12/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco= gopkg.in/cenkalti/backoff.v1 v1.1.0 h1:Arh75ttbsvlpVA7WtVpH4u9h6Zl46xuptxqLxPiSo4Y= gopkg.in/cenkalti/backoff.v1 v1.1.0/go.mod h1:J6Vskwqd+OMVJl8C33mmtxTBs2gyzfv7UDAkHu8BrjI= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/p2p/bootnode.go b/p2p/bootnode.go index 1432faac0a..71cb66ff74 100644 --- a/p2p/bootnode.go +++ b/p2p/bootnode.go @@ -140,6 +140,15 @@ func resolveRelay(ctx context.Context, rawURL, lockHashHex, uuid string, callbac } } +const ( + // maxRelayResponseSize is the maximum accepted relay query response size. Valid responses are + // either an ENR string or a small json array of multiaddrs, so this is a generous upper bound. + maxRelayResponseSize = 64 << 10 // 64KB + + // relayQueryTimeout bounds a single relay query attempt, including reading the response body. + relayQueryTimeout = 10 * time.Second +) + // queryRelayAddrs returns the relay multiaddrs via a http GET query to the url. // // This supports resolving relay addrs from known http URLs which is handy @@ -155,7 +164,7 @@ func queryRelayAddrs(ctx context.Context, relayURL string, backoff func(), lockH } var ( - client http.Client + client = http.Client{Timeout: relayQueryTimeout} doBackoff bool ) for ctx.Err() == nil { @@ -178,11 +187,14 @@ func queryRelayAddrs(ctx context.Context, relayURL string, backoff func(), lockH log.Warn(ctx, "Failure querying relay addresses (will try again)", err) continue } else if resp.StatusCode/100 != 2 { + _ = resp.Body.Close() + log.Warn(ctx, "Non-200 response querying relay addresses (will try again)", nil, z.Int("status_code", resp.StatusCode)) + continue } - b, err := io.ReadAll(resp.Body) + b, err := io.ReadAll(io.LimitReader(resp.Body, maxRelayResponseSize+1)) _ = resp.Body.Close() if err != nil { @@ -190,6 +202,11 @@ func queryRelayAddrs(ctx context.Context, relayURL string, backoff func(), lockH continue } + if len(b) > maxRelayResponseSize { + log.Warn(ctx, "Relay addresses response too large (will try again)", nil, z.Int("max_bytes", maxRelayResponseSize)) + continue + } + if strings.HasPrefix(string(b), "enr:") { addrs, err := multiAddrFromENRStr(string(b)) if err != nil { diff --git a/p2p/bootnode_internal_test.go b/p2p/bootnode_internal_test.go new file mode 100644 index 0000000000..ba4df02293 --- /dev/null +++ b/p2p/bootnode_internal_test.go @@ -0,0 +1,152 @@ +// Copyright © 2022-2026 Obol Labs Inc. Licensed under the terms of a Business Source License 1.1 + +package p2p + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +const testRelayAddr = "/ip4/1.2.3.4/tcp/3030/p2p/16Uiu2HAm1bSDxrCubda6Esz3NkXamvzEjQh4jzMp1PdckJwwMcuw" + +// paddedAddrsJSON returns a valid json multiaddr array padded with trailing +// whitespace to exactly size bytes. +func paddedAddrsJSON(t *testing.T, size int) []byte { + t.Helper() + + b, err := json.Marshal([]string{testRelayAddr}) + require.NoError(t, err) + require.Less(t, len(b), size) + + return append(b, strings.Repeat(" ", size-len(b))...) +} + +// TestQueryRelayAddrsBoundsResponse asserts that a relay streaming an endless response +// cannot make charon read an unbounded amount of it into memory. +func TestQueryRelayAddrsBoundsResponse(t *testing.T) { + var written atomic.Int64 + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + chunk := []byte(strings.Repeat("a", 1<<12)) + for r.Context().Err() == nil { + n, err := w.Write(chunk) + written.Add(int64(n)) + + if err != nil { + return + } + + w.(http.Flusher).Flush() + } + })) + defer srv.Close() + + // Cancel as soon as the read returns, so the server cannot keep writing while the + // query backs off for another attempt. + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + _, err := queryRelayAddrs(ctx, srv.URL, cancel, "lockhash", "uuid") + require.ErrorContains(t, err, "timeout querying relay addresses") + + // The server writes into the socket buffers beyond what charon reads, so allow generous + // slack. Without the limit this grows unbounded until the context deadline. + require.Less(t, written.Load(), int64(8<<20), + "relay response read was not bounded by maxRelayResponseSize") +} + +func TestQueryRelayAddrs(t *testing.T) { + // writeValid responds with a valid json multiaddr array. + writeValid := func(t *testing.T, w http.ResponseWriter, _ *http.Request) { + t.Helper() + require.NoError(t, json.NewEncoder(w).Encode([]string{testRelayAddr})) + } + + // writeOversized responds with an otherwise valid body padded just over the accepted maximum. + // The body stays valid json so that the size limit is what rejects it, not the parser. + writeOversized := func(t *testing.T, w http.ResponseWriter, _ *http.Request) { + t.Helper() + _, _ = w.Write(paddedAddrsJSON(t, maxRelayResponseSize+1)) + } + + // writeAtLimit responds with a valid body padded to exactly the accepted maximum. + writeAtLimit := func(t *testing.T, w http.ResponseWriter, _ *http.Request) { + t.Helper() + _, _ = w.Write(paddedAddrsJSON(t, maxRelayResponseSize)) + } + + // writeUnbounded streams a body until the client stops reading and closes the connection. + // Without a limit on the client side, this never completes. + writeUnbounded := func(_ *testing.T, w http.ResponseWriter, r *http.Request) { + chunk := []byte(strings.Repeat("a", 1<<10)) + for r.Context().Err() == nil { + if _, err := w.Write(chunk); err != nil { + return + } + + w.(http.Flusher).Flush() + } + } + + writeNonOK := func(_ *testing.T, w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusServiceUnavailable) + _, _ = w.Write([]byte("unavailable")) + } + + tests := []struct { + name string + // handlers is applied per request attempt; the last one repeats. + handlers []func(*testing.T, http.ResponseWriter, *http.Request) + }{ + { + name: "valid response", + handlers: []func(*testing.T, http.ResponseWriter, *http.Request){writeValid}, + }, + { + name: "oversized response then valid", + handlers: []func(*testing.T, http.ResponseWriter, *http.Request){writeOversized, writeValid}, + }, + { + name: "response at exactly the limit", + handlers: []func(*testing.T, http.ResponseWriter, *http.Request){writeAtLimit}, + }, + { + name: "unbounded response then valid", + handlers: []func(*testing.T, http.ResponseWriter, *http.Request){writeUnbounded, writeValid}, + }, + { + name: "non-200 response then valid", + handlers: []func(*testing.T, http.ResponseWriter, *http.Request){writeNonOK, writeValid}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + var attempt atomic.Int64 + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + idx := min(int(attempt.Add(1))-1, len(test.handlers)-1) + test.handlers[idx](t, w, r) + })) + defer srv.Close() + + // The context bounds the whole test; a hanging read fails it rather than hanging forever. + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + addrs, err := queryRelayAddrs(ctx, srv.URL, func() {}, "lockhash", "uuid") + require.NoError(t, err) + require.Len(t, addrs, 1) + require.Equal(t, testRelayAddr, addrs[0].String()) + require.EqualValues(t, len(test.handlers), attempt.Load()) + }) + } +} diff --git a/p2p/p2p.go b/p2p/p2p.go index 3cadedd674..eb37152f1c 100644 --- a/p2p/p2p.go +++ b/p2p/p2p.go @@ -461,7 +461,6 @@ func UpgradeToQUICConnections(p2pNode host.Host, peerIDs []peer.ID) lifecycle.Ho forceQUICConn := func(ctx context.Context) { if !isQUICEnabled(p2pNode) { - log.Debug(ctx, "QUIC feature not enabled on this node") return // doesn't support QUIC } @@ -481,8 +480,6 @@ func UpgradeToQUICConnections(p2pNode host.Host, peerIDs []peer.ID) lifecycle.Ho } if hasDirectQUICConn(conns) { - log.Debug(ctx, "Direct QUIC connection to peer already established", z.Str("peer", PeerName(p)), z.Any("conns", conns)) - // Remove unwanted TCP connections for _, conn := range conns { addr := conn.RemoteMultiaddr() diff --git a/testutil/promrated/Dockerfile b/testutil/promrated/Dockerfile index d9ea8b36f7..b7029fff2e 100644 --- a/testutil/promrated/Dockerfile +++ b/testutil/promrated/Dockerfile @@ -1,4 +1,4 @@ -FROM golang:1.26.5-alpine AS builder +FROM golang:1.26.6-alpine AS builder # Install dependencies RUN apk add --no-cache build-base git