From 3f21d50787b84d87b7312c4cb2766c117eb41839 Mon Sep 17 00:00:00 2001 From: Andrei Smirnov Date: Fri, 17 Jul 2026 15:49:18 +0300 Subject: [PATCH] core/parsigex,dkg: harden partial signature exchange Bind partial signatures received during the DKG lock-hash exchange to their authenticated sender: a peer may only contribute partial signatures under its own assigned share index. parsigex.handle now passes the authenticated sender to the verify function; the core workflow verifier ignores it, since those partial signatures are already verified cryptographically against the pubshare for the claimed share index. The DKG exchanger keys the binding on each peer's assigned share index via a peer map, so it stays correct when share indices are not contiguous with peer positions, such as after operators are removed. newExchanger validates its peer index and peer map up front and returns an error on a misconfigured map instead of silently timing out. category: bug ticket: none feature_flag: none Co-Authored-By: Claude Opus 4.8 --- core/parsigex/parsigex.go | 14 ++- core/parsigex/parsigex_test.go | 26 ++--- dkg/dkg.go | 21 ++-- dkg/exchanger.go | 46 +++++++- dkg/exchanger_internal_test.go | 180 ++++++++++++++++++++++++++++- dkg/protocol_addoperators.go | 7 +- dkg/protocol_removeoperators.go | 20 +++- dkg/protocol_replaceoperator.go | 7 +- dkg/protocol_reshare.go | 7 +- dkg/protocolsteps_internal_test.go | 5 +- 10 files changed, 291 insertions(+), 42 deletions(-) diff --git a/core/parsigex/parsigex.go b/core/parsigex/parsigex.go index 502bcfc6a1..9f4ab053fb 100644 --- a/core/parsigex/parsigex.go +++ b/core/parsigex/parsigex.go @@ -40,7 +40,7 @@ func Protocols() []protocol.ID { } func NewParSigEx(p2pNode host.Host, sendFunc p2p.SendFunc, peerIdx int, peers []peer.ID, - verifyFunc func(context.Context, core.Duty, core.PubKey, core.ParSignedData) error, + verifyFunc func(context.Context, peer.ID, core.Duty, core.PubKey, core.ParSignedData) error, gaterFunc core.DutyGaterFunc, p2pOpts ...p2p.SendRecvOption, ) *ParSigEx { parSigEx := &ParSigEx{ @@ -72,12 +72,12 @@ type ParSigEx struct { sendFunc p2p.SendFunc peerIdx int peers []peer.ID - verifyFunc func(context.Context, core.Duty, core.PubKey, core.ParSignedData) error + verifyFunc func(context.Context, peer.ID, core.Duty, core.PubKey, core.ParSignedData) error gaterFunc core.DutyGaterFunc subs []func(context.Context, core.Duty, core.ParSignedDataSet) error } -func (m *ParSigEx) handle(ctx context.Context, _ peer.ID, req proto.Message) (proto.Message, bool, error) { +func (m *ParSigEx) handle(ctx context.Context, sender peer.ID, req proto.Message) (proto.Message, bool, error) { pb, ok := req.(*pbv1.ParSigExMsg) if !ok { return nil, false, errors.New("invalid request type") @@ -110,7 +110,7 @@ func (m *ParSigEx) handle(ctx context.Context, _ peer.ID, req proto.Message) (pr verifyStart := time.Now() for pubkey, data := range set { - if err = m.verifyFunc(ctx, duty, pubkey, data); err != nil { + if err = m.verifyFunc(ctx, sender, duty, pubkey, data); err != nil { return nil, false, errors.Wrap(err, "invalid partial signature") } } @@ -165,8 +165,10 @@ func (m *ParSigEx) Subscribe(fn func(context.Context, core.Duty, core.ParSignedD } // NewEth2Verifier returns a partial signature verification function for core workflow eth2 signatures. -func NewEth2Verifier(eth2Cl eth2wrap.Client, pubSharesByKey map[core.PubKey]map[int]tbls.PublicKey) (func(context.Context, core.Duty, core.PubKey, core.ParSignedData) error, error) { - return func(ctx context.Context, duty core.Duty, pubkey core.PubKey, data core.ParSignedData) error { +// Core workflow partial signatures are verified cryptographically against the pubshare for the +// claimed share index, so the authenticated sender is not needed here. +func NewEth2Verifier(eth2Cl eth2wrap.Client, pubSharesByKey map[core.PubKey]map[int]tbls.PublicKey) (func(context.Context, peer.ID, core.Duty, core.PubKey, core.ParSignedData) error, error) { + return func(ctx context.Context, _ peer.ID, duty core.Duty, pubkey core.PubKey, data core.ParSignedData) error { pubshares, ok := pubSharesByKey[pubkey] if !ok { return errors.New("unknown pubkey, not part of cluster lock") diff --git a/core/parsigex/parsigex_test.go b/core/parsigex/parsigex_test.go index 41a0a7b162..413815003d 100644 --- a/core/parsigex/parsigex_test.go +++ b/core/parsigex/parsigex_test.go @@ -73,7 +73,7 @@ func TestParSigEx(t *testing.T) { } } - verifyFunc := func(context.Context, core.Duty, core.PubKey, core.ParSignedData) error { + verifyFunc := func(context.Context, peer.ID, core.Duty, core.PubKey, core.ParSignedData) error { return nil } @@ -171,7 +171,7 @@ func TestParSigExVerifier(t *testing.T) { att.Deneb.Signature = sign(sigData[:]) data, err := core.NewPartialVersionedAttestation(att, shareIdx) require.NoError(t, err) - require.NoError(t, verifyFunc(ctx, core.NewAttesterDuty(slot), pubkey, data)) + require.NoError(t, verifyFunc(ctx, "", core.NewAttesterDuty(slot), pubkey, data)) }) t.Run("Verify proposal", func(t *testing.T) { @@ -186,7 +186,7 @@ func TestParSigExVerifier(t *testing.T) { data, err := core.NewPartialVersionedSignedProposal(proposal, shareIdx) require.NoError(t, err) - require.NoError(t, verifyFunc(ctx, core.NewProposerDuty(slot), pubkey, data)) + require.NoError(t, verifyFunc(ctx, "", core.NewProposerDuty(slot), pubkey, data)) }) t.Run("Verify blinded proposal", func(t *testing.T) { @@ -204,7 +204,7 @@ func TestParSigExVerifier(t *testing.T) { data, err := core.NewPartialVersionedSignedBlindedProposal(ð2apiBlinded, shareIdx) require.NoError(t, err) - require.NoError(t, verifyFunc(ctx, core.NewProposerDuty(slot), pubkey, data)) + require.NoError(t, verifyFunc(ctx, "", core.NewProposerDuty(slot), pubkey, data)) }) t.Run("Verify Randao", func(t *testing.T) { @@ -216,7 +216,7 @@ func TestParSigExVerifier(t *testing.T) { randao := core.NewPartialSignedRandao(epoch, sign(sigData[:]), shareIdx) - require.NoError(t, verifyFunc(ctx, core.NewRandaoDuty(slot), pubkey, randao)) + require.NoError(t, verifyFunc(ctx, "", core.NewRandaoDuty(slot), pubkey, randao)) }) t.Run("Verify Voluntary Exit", func(t *testing.T) { @@ -232,7 +232,7 @@ func TestParSigExVerifier(t *testing.T) { require.NoError(t, err) - require.NoError(t, verifyFunc(ctx, core.NewVoluntaryExit(slot), pubkey, data)) + require.NoError(t, verifyFunc(ctx, "", core.NewVoluntaryExit(slot), pubkey, data)) }) t.Run("Verify validator registration", func(t *testing.T) { @@ -249,7 +249,7 @@ func TestParSigExVerifier(t *testing.T) { data, err := core.NewPartialVersionedSignedValidatorRegistration(®.VersionedSignedValidatorRegistration, shareIdx) require.NoError(t, err) - require.NoError(t, verifyFunc(ctx, core.NewBuilderRegistrationDuty(slot), pubkey, data)) + require.NoError(t, verifyFunc(ctx, "", core.NewBuilderRegistrationDuty(slot), pubkey, data)) }) t.Run("Verify beacon committee selection", func(t *testing.T) { @@ -263,7 +263,7 @@ func TestParSigExVerifier(t *testing.T) { selection.SelectionProof = sign(sigData[:]) data := core.NewPartialSignedBeaconCommitteeSelection(selection, shareIdx) - require.NoError(t, verifyFunc(ctx, core.NewPrepareAggregatorDuty(slot), pubkey, data)) + require.NoError(t, verifyFunc(ctx, "", core.NewPrepareAggregatorDuty(slot), pubkey, data)) }) t.Run("Verify aggregate and proof", func(t *testing.T) { @@ -286,7 +286,7 @@ func TestParSigExVerifier(t *testing.T) { agg.Deneb.Signature = sign(sigData[:]) data := core.NewPartialVersionedSignedAggregateAndProof(agg, shareIdx) - require.NoError(t, verifyFunc(ctx, core.NewAggregatorDuty(slot), pubkey, data)) + require.NoError(t, verifyFunc(ctx, "", core.NewAggregatorDuty(slot), pubkey, data)) }) t.Run("verify sync committee message", func(t *testing.T) { @@ -299,11 +299,11 @@ func TestParSigExVerifier(t *testing.T) { msg.Signature = sign(sigData[:]) data := core.NewPartialSignedSyncMessage(msg, shareIdx) - require.NoError(t, verifyFunc(ctx, core.NewSyncMessageDuty(slot), pubkey, data)) + require.NoError(t, verifyFunc(ctx, "", core.NewSyncMessageDuty(slot), pubkey, data)) // Invalid sync committee message. data = core.NewPartialSignedRandao(epoch, testutil.RandomEth2Signature(), shareIdx) - err = verifyFunc(ctx, core.NewSyncMessageDuty(slot), pubkey, data) + err = verifyFunc(ctx, "", core.NewSyncMessageDuty(slot), pubkey, data) require.Error(t, err) require.ErrorContains(t, err, "invalid signature") }) @@ -326,7 +326,7 @@ func TestParSigExVerifier(t *testing.T) { parSigData := core.NewPartialSignedSyncCommitteeSelection(selection, shareIdx) - require.NoError(t, verifyFunc(ctx, core.NewPrepareSyncContributionDuty(slot), pubkey, parSigData)) + require.NoError(t, verifyFunc(ctx, "", core.NewPrepareSyncContributionDuty(slot), pubkey, parSigData)) }) t.Run("verify sync committee contribution and proof", func(t *testing.T) { @@ -343,7 +343,7 @@ func TestParSigExVerifier(t *testing.T) { parSigData := core.NewPartialSignedSyncContributionAndProof(proof, shareIdx) - require.NoError(t, verifyFunc(ctx, core.NewPrepareSyncContributionDuty(slot), pubkey, parSigData)) + require.NoError(t, verifyFunc(ctx, "", core.NewPrepareSyncContributionDuty(slot), pubkey, parSigData)) }) } diff --git a/dkg/dkg.go b/dkg/dkg.go index 6e15c735e1..20ac6441ad 100644 --- a/dkg/dkg.go +++ b/dkg/dkg.go @@ -241,24 +241,27 @@ func Run(ctx context.Context, conf Config) (err error) { return errors.Wrap(err, "get peer IDs") } - ex := newExchanger(p2pNode, nodeIdx.PeerIdx, peerIDs, []sigType{ - sigLock, - sigDepositData, - sigValidatorRegistration, - }, conf.Timeout) - - // Register libp2p handlers peerMap := make(map[peer.ID]cluster.NodeIdx) for _, p := range peers { - nodeIdx, err := def.NodeIdx(p.ID) + idx, err := def.NodeIdx(p.ID) if err != nil { return err } - peerMap[p.ID] = nodeIdx + peerMap[p.ID] = idx } + ex, err := newExchanger(p2pNode, nodeIdx.PeerIdx, peerIDs, peerMap, []sigType{ + sigLock, + sigDepositData, + sigValidatorRegistration, + }, conf.Timeout) + if err != nil { + return err + } + + // Register libp2p handlers caster := bcast.New(p2pNode, peerIDs, key) // register bcast callbacks for frostp2p diff --git a/dkg/exchanger.go b/dkg/exchanger.go index 04ca2f8652..a8571a7d10 100644 --- a/dkg/exchanger.go +++ b/dkg/exchanger.go @@ -14,6 +14,7 @@ import ( "github.com/obolnetwork/charon/app/errors" "github.com/obolnetwork/charon/app/z" + "github.com/obolnetwork/charon/cluster" "github.com/obolnetwork/charon/core" "github.com/obolnetwork/charon/core/parsigdb" "github.com/obolnetwork/charon/core/parsigex" @@ -67,10 +68,25 @@ type exchanger struct { dutyGaterFunc func(duty core.Duty) bool } -func newExchanger(p2pNode host.Host, peerIdx int, peers []peer.ID, sigTypes []sigType, timeout time.Duration) *exchanger { - // Partial signature roots not known yet, so skip verification in parsigex, rather verify before we aggregate. - noopVerifier := func(context.Context, core.Duty, core.PubKey, core.ParSignedData) error { - return nil +func newExchanger(p2pNode host.Host, peerIdx int, peers []peer.ID, peerMap map[peer.ID]cluster.NodeIdx, sigTypes []sigType, timeout time.Duration) (*exchanger, error) { + if peerIdx < 0 || peerIdx >= len(peers) { + return nil, errors.New("peer index out of range", z.Int("peer_idx", peerIdx), z.Int("num_peers", len(peers))) + } + + // Every peer must have a valid share index in peerMap, otherwise its partial signatures would be + // rejected as unknown and the exchange would silently time out. Fail fast on a misconfigured map. + for _, p := range peers { + if nodeIdx, ok := peerMap[p]; !ok || nodeIdx.ShareIdx <= 0 { + return nil, errors.New("peer map missing valid share index for peer", z.Str("peer", p.String())) + } + } + + // Partial signature roots are not known during the exchange, so cryptographic verification is + // deferred until aggregation. Each received partial signature is still bound to its authenticated + // sender: a peer may only contribute partial signatures under its own assigned share index, + // consistent with the sender check in nodesigs.go. + verifyShareIdx := func(_ context.Context, sender peer.ID, _ core.Duty, _ core.PubKey, data core.ParSignedData) error { + return verifyPeerShareIdx(peerMap, sender, data) } st := make(map[sigType]bool) @@ -94,7 +110,7 @@ func newExchanger(p2pNode host.Host, peerIdx int, peers []peer.ID, sigTypes []si ex := &exchanger{ // threshold is len(peers) to wait until we get all the partial sigs from all the peers per DV sigdb: parsigdb.NewMemDB(len(peers), noopDeadliner{}, parsigdb.NewMemDBMetadata(0, time.Now())), // metadata timestamps are used for metrics, irrelevant for DKG - sigex: parsigex.NewParSigEx(p2pNode, p2p.Send, peerIdx, peers, noopVerifier, dutyGaterFunc, p2p.WithSendTimeout(timeout), p2p.WithReceiveTimeout(timeout)), + sigex: parsigex.NewParSigEx(p2pNode, p2p.Send, peerIdx, peers, verifyShareIdx, dutyGaterFunc, p2p.WithSendTimeout(timeout), p2p.WithReceiveTimeout(timeout)), sigTypes: st, sigData: dataByPubkey{ store: sigTypeStore{}, @@ -108,7 +124,25 @@ func newExchanger(p2pNode host.Host, peerIdx int, peers []peer.ID, sigTypes []si ex.sigdb.SubscribeThreshold(ex.pushPsigs) ex.sigex.Subscribe(ex.sigdb.StoreExternal) - return ex + return ex, nil +} + +// verifyPeerShareIdx checks that a received partial signature originates from a known peer and uses +// that peer's assigned share index. peerMap maps each participating peer to its node index, so the +// check remains correct when share indices are not contiguous with peer positions (for example when +// operators have been removed and the remaining ones keep their original share indices). +func verifyPeerShareIdx(peerMap map[peer.ID]cluster.NodeIdx, sender peer.ID, data core.ParSignedData) error { + nodeIdx, ok := peerMap[sender] + if !ok { + return errors.New("partial signature from unknown peer", z.Str("peer", sender.String())) + } + + if data.ShareIdx <= 0 || data.ShareIdx != nodeIdx.ShareIdx { + return errors.New("partial signature share index does not match sender peer", + z.Str("peer", sender.String()), z.Int("share_idx", data.ShareIdx), z.Int("expected_share_idx", nodeIdx.ShareIdx)) + } + + return nil } // exchange exchanges partial signatures of lockhash/deposit-data among dkg participants and returns all the partial diff --git a/dkg/exchanger_internal_test.go b/dkg/exchanger_internal_test.go index b2ce77fcb3..a68f330f2e 100644 --- a/dkg/exchanger_internal_test.go +++ b/dkg/exchanger_internal_test.go @@ -14,10 +14,22 @@ import ( "github.com/libp2p/go-libp2p/core/peerstore" "github.com/stretchr/testify/require" + "github.com/obolnetwork/charon/cluster" "github.com/obolnetwork/charon/core" "github.com/obolnetwork/charon/testutil" ) +// positionalPeerMap builds a peer-to-node-index map where each peer's share index is its position +// plus one, matching the default cluster layout. +func positionalPeerMap(peers []peer.ID) map[peer.ID]cluster.NodeIdx { + peerMap := make(map[peer.ID]cluster.NodeIdx, len(peers)) + for i, p := range peers { + peerMap[p] = cluster.NodeIdx{PeerIdx: i, ShareIdx: i + 1} + } + + return peerMap +} + // TODO(dhruv): add tests for negative scenarios (take inspiration from core/qbft/qbft_internal_test). func TestExchanger(t *testing.T) { ctx := context.Background() @@ -94,7 +106,9 @@ func TestExchanger(t *testing.T) { } for i := range nodes { - ex := newExchanger(hosts[i], i, peers, expectedSigTypes, 8*time.Second) + ex, err := newExchanger(hosts[i], i, peers, positionalPeerMap(peers), expectedSigTypes, 8*time.Second) + require.NoError(t, err) + exchangers = append(exchangers, ex) } @@ -189,7 +203,8 @@ func TestExchangerPushPsigsNeverBlocks(t *testing.T) { h := testutil.CreateHost(t, testutil.AvailableAddr(t)) peers := []peer.ID{h.ID()} - ex := newExchanger(h, 0, peers, []sigType{sigLock}, time.Second) + ex, err := newExchanger(h, 0, peers, positionalPeerMap(peers), []sigType{sigLock}, time.Second) + require.NoError(t, err) duty := core.NewSignatureDuty(uint64(sigLock)) @@ -224,3 +239,164 @@ func TestExchangerPushPsigsNeverBlocks(t *testing.T) { t.Fatal("pushPsigs deadlocked") } } + +// TestExchangerRejectsMismatchedShareIndex verifies the sender->shareIdx binding in the lock-hash +// exchange: a peer may only contribute partial signatures under its own share index. +// +// The exchange defers cryptographic verification until aggregation, so partial signatures are +// accepted at receive time without checking their signature. newExchanger therefore binds each +// received partial signature to its authenticated sender (shareIdx == peerIdx+1), consistent with +// the sender check in nodesigs.go. +// +// This drives the real /charon/parsigex/2.0.0 receive path: one node broadcasts partial signatures +// for a pubkey outside the cluster under every share index. Every share index other than that +// node's own is rejected, so the outside pubkey never reaches the threshold, while the honest +// exchange for the real validator completes with a full set of shares. +func TestExchangerRejectsMismatchedShareIndex(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + // threshold = len(peers), so reaching it for a single pubkey needs this many distinct share + // indices, which one sender can no longer supply on its own. + const nodes = 4 + + var ( + peers []peer.ID + hosts []host.Host + hostsInfo []peer.AddrInfo + ) + + for range nodes { + h := testutil.CreateHost(t, testutil.AvailableAddr(t)) + hostsInfo = append(hostsInfo, peer.AddrInfo{ID: h.ID(), Addrs: h.Addrs()}) + peers = append(peers, h.ID()) + hosts = append(hosts, h) + } + + // Connect every host to its peers so exchange broadcasts reach live handlers. + for i := range nodes { + for j := range nodes { + if i == j { + continue + } + + hosts[i].Peerstore().AddAddrs(hostsInfo[j].ID, hostsInfo[j].Addrs, peerstore.PermanentAddrTTL) + } + } + + exchangers := make([]*exchanger, nodes) + for i := range nodes { + ex, err := newExchanger(hosts[i], i, peers, positionalPeerMap(peers), []sigType{sigLock}, 8*time.Second) + require.NoError(t, err) + + exchangers[i] = ex + } + + realPk := testutil.RandomCorePubKey(t) // The cluster's genuine distributed validator. + outsidePk := testutil.RandomCorePubKey(t) // A pubkey that is not part of the cluster. + + // otherNode (peer index 1) has share index 2 as its own. It broadcasts partial signatures for + // outsidePk under every share index over the authenticated parsigex protocol. Every peer's + // sender->shareIdx binding must reject every share index other than otherNode's own, so outsidePk + // can never reach the threshold anywhere. + const otherNode = 1 + + sigLockDuty := core.NewSignatureDuty(uint64(sigLock)) + + for shareIdx := 1; shareIdx <= nodes; shareIdx++ { + set := core.ParSignedDataSet{ + outsidePk: core.NewPartialSignature(testutil.RandomCoreSignature(), shareIdx), + } + require.NoError(t, exchangers[otherNode].sigex.Broadcast(ctx, sigLockDuty, set)) + } + + // Every node runs the honest lock-hash exchange for the single real validator, each contributing + // only its own share. The real validator reaches the threshold legitimately and the exchange + // resolves with real data. + var wg sync.WaitGroup + + results := make([]map[core.PubKey][]core.ParSignedData, nodes) + errs := make([]error, nodes) + + for i := range nodes { + wg.Add(1) + + go func(node int) { + defer wg.Done() + + set := core.ParSignedDataSet{ + realPk: core.NewPartialSignature(testutil.RandomCoreSignature(), node+1), + } + results[node], errs[node] = exchangers[node].exchange(ctx, sigLock, set) + }(i) + } + + wg.Wait() + + // The outside pubkey never reached the threshold, so no exchange result contains it; the real + // validator is present with a full set of shares. + for i := range nodes { + require.NoErrorf(t, errs[i], "node %d exchange failed", i) + require.Containsf(t, results[i], realPk, "node %d missing the real validator", i) + require.Lenf(t, results[i][realPk], nodes, "node %d has an unexpected number of shares", i) + require.NotContainsf(t, results[i], outsidePk, "node %d accepted the outside pubkey", i) + } +} + +// TestVerifyPeerShareIdx covers the sender->shareIdx binding, including a non-contiguous layout where +// a peer's assigned share index does not equal its position (as when earlier operators are removed). +func TestVerifyPeerShareIdx(t *testing.T) { + const ( + self = peer.ID("self") + other = peer.ID("other") + unknown = peer.ID("unknown") + ) + + // "other" is the second peer but keeps share index 4, e.g. after operators with lower indices + // have been removed. + peerMap := map[peer.ID]cluster.NodeIdx{ + self: {PeerIdx: 0, ShareIdx: 1}, + other: {PeerIdx: 1, ShareIdx: 4}, + } + + tests := []struct { + name string + sender peer.ID + shareIdx int + wantErr string + }{ + {name: "own share index accepted", sender: self, shareIdx: 1}, + {name: "assigned non-contiguous share index accepted", sender: other, shareIdx: 4}, + {name: "mismatched share index rejected", sender: other, shareIdx: 2, wantErr: "share index does not match"}, + {name: "another peer's share index rejected", sender: self, shareIdx: 4, wantErr: "share index does not match"}, + {name: "non-positive share index rejected", sender: self, shareIdx: 0, wantErr: "share index does not match"}, + {name: "unknown sender rejected", sender: unknown, shareIdx: 1, wantErr: "unknown peer"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + data := core.NewPartialSignature(testutil.RandomCoreSignature(), tt.shareIdx) + + err := verifyPeerShareIdx(peerMap, tt.sender, data) + if tt.wantErr == "" { + require.NoError(t, err) + } else { + require.ErrorContains(t, err, tt.wantErr) + } + }) + } +} + +// TestNewExchangerRejectsIncompletePeerMap ensures construction fails fast when a peer has no valid +// share index, rather than silently rejecting its partial signatures and timing out. +func TestNewExchangerRejectsIncompletePeerMap(t *testing.T) { + h := testutil.CreateHost(t, testutil.AvailableAddr(t)) + peers := []peer.ID{h.ID(), peer.ID("missing")} + + peerMap := map[peer.ID]cluster.NodeIdx{ + h.ID(): {PeerIdx: 0, ShareIdx: 1}, + } + + _, err := newExchanger(h, 0, peers, peerMap, []sigType{sigLock}, time.Second) + require.ErrorContains(t, err, "missing valid share index") +} diff --git a/dkg/protocol_addoperators.go b/dkg/protocol_addoperators.go index 9028e96b89..3f8340d044 100644 --- a/dkg/protocol_addoperators.go +++ b/dkg/protocol_addoperators.go @@ -85,7 +85,12 @@ func (p *addOperatorsProtocol) PostInit(ctx context.Context, pctx *ProtocolConte p.allENRs = append(p.allENRs, p.newENRs...) - pctx.SigExchanger = newExchanger(pctx.ThisNode, pctx.ThisNodeIdx.PeerIdx, pctx.PeerIDs, []sigType{sigLock}, pctx.Config.Timeout) + sigEx, err := newExchanger(pctx.ThisNode, pctx.ThisNodeIdx.PeerIdx, pctx.PeerIDs, pctx.PeerMap, []sigType{sigLock}, pctx.Config.Timeout) + if err != nil { + return err + } + + pctx.SigExchanger = sigEx pctx.Caster = bcast.New(pctx.ThisNode, pctx.PeerIDs, pctx.ENRPrivateKey) pctx.NodeSigCaster = newNodeSigBcast(pctx.Peers, pctx.ThisNodeIdx, pctx.Caster) diff --git a/dkg/protocol_removeoperators.go b/dkg/protocol_removeoperators.go index 572f5078e0..5d082cd1f2 100644 --- a/dkg/protocol_removeoperators.go +++ b/dkg/protocol_removeoperators.go @@ -119,6 +119,12 @@ func (p *removeOperatorsProtocol) PostInit(ctx context.Context, pctx *ProtocolCo oldPeerIDs := make([]peer.ID, 0) newPeerIDs := make([]peer.ID, 0) + // exchangerPeerMap binds each remaining node to its original share index for the lock-hash + // exchange. Removed operators are excluded even when they still participate in the reshare, since + // they do not contribute lock-hash signatures; keeping them out prevents their authenticated peer + // ID from being used to submit one. + exchangerPeerMap := make(map[peer.ID]cluster.NodeIdx) + for i, op := range pctx.Lock.Operators { isOld := slices.Contains(p.oldENRs, op.ENR) if isOld { @@ -126,6 +132,9 @@ func (p *removeOperatorsProtocol) PostInit(ctx context.Context, pctx *ProtocolCo } else { newPeerIDs = append(newPeerIDs, allPeers[i].ID) p.operators = append(p.operators, op.ENR) + // ShareIdx is the 1-indexed original operator position, matching buildPeerMap and the + // index each remaining node signs with. + exchangerPeerMap[allPeers[i].ID] = cluster.NodeIdx{PeerIdx: i, ShareIdx: i + 1} } if pctx.ThisPeerID == allPeers[i].ID && isOld { @@ -146,12 +155,21 @@ func (p *removeOperatorsProtocol) PostInit(ctx context.Context, pctx *ProtocolCo if !p.oldNode { // SigExchanger is only created for nodes remaining in the cluster, because old nodes do not participate in signing. + // Remaining nodes keep their original share indices, so exchangerPeerMap (keyed by peer ID, + // with each node's original share index in the value) binds each partial signature to its + // sender's assigned share index. nodeIdx := slices.Index(newPeerIDs, pctx.ThisPeerID) pctx.ThisNodeIdx = cluster.NodeIdx{ PeerIdx: nodeIdx, ShareIdx: peerMap[pctx.ThisPeerID].ShareIdx, } - pctx.SigExchanger = newExchanger(pctx.ThisNode, nodeIdx, newPeerIDs, []sigType{sigLock}, pctx.Config.Timeout) + + sigEx, err := newExchanger(pctx.ThisNode, nodeIdx, newPeerIDs, exchangerPeerMap, []sigType{sigLock}, pctx.Config.Timeout) + if err != nil { + return err + } + + pctx.SigExchanger = sigEx } reshareConfig := pedersen.NewReshareConfig(len(pctx.Lock.Validators), p.newThreshold, nil, oldPeerIDs) diff --git a/dkg/protocol_replaceoperator.go b/dkg/protocol_replaceoperator.go index f9e82f976c..ca0cfca26c 100644 --- a/dkg/protocol_replaceoperator.go +++ b/dkg/protocol_replaceoperator.go @@ -103,7 +103,12 @@ func (p *replaceOperatorProtocol) GetPeers(lock *cluster.Lock) ([]p2p.Peer, erro } func (p *replaceOperatorProtocol) PostInit(ctx context.Context, pctx *ProtocolContext) error { - pctx.SigExchanger = newExchanger(pctx.ThisNode, pctx.ThisNodeIdx.PeerIdx, pctx.PeerIDs, []sigType{sigLock}, pctx.Config.Timeout) + sigEx, err := newExchanger(pctx.ThisNode, pctx.ThisNodeIdx.PeerIdx, pctx.PeerIDs, pctx.PeerMap, []sigType{sigLock}, pctx.Config.Timeout) + if err != nil { + return err + } + + pctx.SigExchanger = sigEx pctx.Caster = bcast.New(pctx.ThisNode, pctx.PeerIDs, pctx.ENRPrivateKey) pctx.NodeSigCaster = newNodeSigBcast(pctx.Peers, pctx.ThisNodeIdx, pctx.Caster) diff --git a/dkg/protocol_reshare.go b/dkg/protocol_reshare.go index 01a0c124b4..0e6b14b094 100644 --- a/dkg/protocol_reshare.go +++ b/dkg/protocol_reshare.go @@ -49,7 +49,12 @@ func (*reshareProtocol) GetPeers(lock *cluster.Lock) ([]p2p.Peer, error) { } func (p *reshareProtocol) PostInit(ctx context.Context, pctx *ProtocolContext) error { - pctx.SigExchanger = newExchanger(pctx.ThisNode, pctx.ThisNodeIdx.PeerIdx, pctx.PeerIDs, []sigType{sigLock}, pctx.Config.Timeout) + sigEx, err := newExchanger(pctx.ThisNode, pctx.ThisNodeIdx.PeerIdx, pctx.PeerIDs, pctx.PeerMap, []sigType{sigLock}, pctx.Config.Timeout) + if err != nil { + return err + } + + pctx.SigExchanger = sigEx pctx.Caster = bcast.New(pctx.ThisNode, pctx.PeerIDs, pctx.ENRPrivateKey) pctx.NodeSigCaster = newNodeSigBcast(pctx.Peers, pctx.ThisNodeIdx, pctx.Caster) diff --git a/dkg/protocolsteps_internal_test.go b/dkg/protocolsteps_internal_test.go index 6571d0f760..85867f1965 100644 --- a/dkg/protocolsteps_internal_test.go +++ b/dkg/protocolsteps_internal_test.go @@ -142,7 +142,8 @@ func TestUpdateLockProtocolStep(t *testing.T) { shares := valKeysToSharesNode0(t, valKeys, lock.Validators) host := testutil.CreateHost(t, testutil.AvailableAddr(t)) - sigex := newExchanger(host, 0, []peer.ID{host.ID()}, []sigType{sigLock}, 10*time.Second) + sigex, err := newExchanger(host, 0, []peer.ID{host.ID()}, positionalPeerMap([]peer.ID{host.ID()}), []sigType{sigLock}, 10*time.Second) + require.NoError(t, err) pctx := &ProtocolContext{ Lock: &lock, @@ -151,7 +152,7 @@ func TestUpdateLockProtocolStep(t *testing.T) { SigExchanger: sigex, ThisNodeIdx: cluster.NodeIdx{PeerIdx: 0, ShareIdx: 1}, } - err := step.Run(t.Context(), pctx) + err = step.Run(t.Context(), pctx) require.NoError(t, err) require.Equal(t, 4, pctx.Lock.Threshold)