Skip to content

Commit

Permalink
Add lock in notifier to avoid data race
Browse files Browse the repository at this point in the history
Write at 0x00c000288110 by goroutine 380:
  gitlab.com/thorchain/tss/go-tss/keysign.(*notifier).updateUnset()
      /home/runner/work/go-tss/go-tss/keysign/notifier.go:62 +0xe4
  gitlab.com/thorchain/tss/go-tss/keysign.(*SignatureNotifier).createOrUpdateNotifier()
      /home/runner/work/go-tss/go-tss/keysign/signature_notifier.go:252 +0x2fd
  gitlab.com/thorchain/tss/go-tss/keysign.(*SignatureNotifier).handleStream()
      /home/runner/work/go-tss/go-tss/keysign/signature_notifier.go:136 +0xa71
  gitlab.com/thorchain/tss/go-tss/keysign.(*SignatureNotifier).handleStream-fm()
      <autogenerated>:1 +0x4d
  github.com/libp2p/go-libp2p/p2p/host/basic.(*BasicHost).SetStreamHandler.func1()
      /home/runner/go/pkg/mod/github.com/zeta-chain/[email protected]/p2p/host/basic/basic_host.go:580 +0x86
  github.com/libp2p/go-libp2p/p2p/host/basic.(*BasicHost).newStreamHandler.func1()
      /home/runner/go/pkg/mod/github.com/zeta-chain/[email protected]/p2p/host/basic/basic_host.go:421 +0x74

Previous read at 0x00c000288110 by goroutine 208:
  gitlab.com/thorchain/tss/go-tss/keysign.(*notifier).readyToProcess()
      /home/runner/work/go-tss/go-tss/keysign/notifier.go:51 +0xd46
  gitlab.com/thorchain/tss/go-tss/keysign.TestSignatureNotifierBroadcastFirst()
      /home/runner/work/go-tss/go-tss/keysign/signature_notifier_test.go:162 +0xdb8
  testing.tRunner()
      /opt/hostedtoolcache/go/1.20.14/x64/src/testing/testing.go:1576 +0x216
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.20.14/x64/src/testing/testing.go:1629 +0x47
  • Loading branch information
gartnera committed Aug 26, 2024
1 parent 85ab1a3 commit 1d43c69
Showing 1 changed file with 11 additions and 0 deletions.
11 changes: 11 additions & 0 deletions keysign/notifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"fmt"
"math/big"
"sync"
"time"

"github.com/bnb-chain/tss-lib/v2/common"
Expand All @@ -27,6 +28,7 @@ type notifier struct {
processed bool
lastUpdated time.Time
ttl time.Duration
lock sync.RWMutex
}

// newNotifier create a new instance of notifier.
Expand All @@ -42,11 +44,14 @@ func newNotifier(messageID string, messages [][]byte, poolPubKey string, signatu
resp: make(chan []*common.SignatureData, 1),
lastUpdated: time.Now(),
ttl: defaultNotifierTTL,
lock: sync.RWMutex{},
}, nil
}

// readyToProcess ensures we have everything we need to process the signatures
func (n *notifier) readyToProcess() bool {
n.lock.RLock()
defer n.lock.RUnlock()
return len(n.messageID) > 0 &&
len(n.messages) > 0 &&
len(n.poolPubKey) > 0 &&
Expand All @@ -57,6 +62,8 @@ func (n *notifier) readyToProcess() bool {
// updateUnset will incrementally update the internal state of notifier with any new values
// provided that are not nil/empty.
func (n *notifier) updateUnset(messages [][]byte, poolPubKey string, signatures []*common.SignatureData) {
n.lock.Lock()
defer n.lock.Unlock()
n.lastUpdated = time.Now()
if n.messages == nil {
n.messages = messages
Expand All @@ -75,7 +82,9 @@ func (n *notifier) updateUnset(messages [][]byte, poolPubKey string, signatures
// go-tss respect the payload it receives , assume the payload had been hashed already by whoever send it in.
func (n *notifier) verifySignature(data *common.SignatureData, msg []byte) error {
// we should be able to use any of the pubkeys to verify the signature
n.lock.RLock()
pubKey, err := sdk.UnmarshalPubKey(sdk.AccPK, n.poolPubKey)
n.lock.RUnlock()
if err != nil {
return fmt.Errorf("fail to get pubkey from bech32 pubkey string(%s):%w", n.poolPubKey, err)
}
Expand Down Expand Up @@ -117,6 +126,8 @@ func (n *notifier) verifySignature(data *common.SignatureData, msg []byte) error
// return value bool , true indicated we already gather all the signature from keysign party, and they are all match
// false means we are still waiting for more signature from keysign party
func (n *notifier) processSignature(data []*common.SignatureData) error {
n.lock.RLock()
defer n.lock.RUnlock()
// only need to verify the signature when data is not nil
// when data is nil , which means keysign failed, there is no signature to be verified in that case
// for gg20, it wrap the signature R,S into ECSignature structure
Expand Down

0 comments on commit 1d43c69

Please sign in to comment.