Skip to content

Commit

Permalink
pool: Correct account races.
Browse files Browse the repository at this point in the history
The account and name fields of a client are not set until they are
received via a mining.authorize message and the account field is
accessed from separate goroutines which is a race.

This corrects that issue by introducing a separate mutex to protect both
the account and the name field and updating all references to by
protected by the mutex accordingly.

The name field doesn't appear to be used anywhere currently, however, it
would also be a race to access it without the mutex for the same reason,
so this takes the opportunity to ensure it is protected in case it is
used in the future.
  • Loading branch information
davecgh authored and jholdstock committed Oct 9, 2023
1 parent c997341 commit 3a35654
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 9 deletions.
25 changes: 18 additions & 7 deletions pool/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,11 +129,15 @@ type Client struct {
reader *bufio.Reader
ctx context.Context
cancel context.CancelFunc
name string
extraNonce1 string
ch chan Message
readCh chan readPayload
account string

// These fields track the client's account ID and name from the authorize
// message.
accountMtx sync.RWMutex
account string
name string

// These fields track the authorization and subscription status of
// the miner.
Expand Down Expand Up @@ -205,7 +209,7 @@ func (c *Client) claimWeightedShare() error {
return errs.PoolError(errs.ClaimShare, desc)
}
weight := ShareWeights[miner]
share := NewShare(c.account, weight)
share := NewShare(c.FetchAccountID(), weight)
return c.cfg.db.PersistShare(share)
}

Expand Down Expand Up @@ -268,14 +272,17 @@ func (c *Client) handleAuthorizeRequest(req *Request, allowed bool) error {
}
}

c.accountMtx.Lock()
c.account = account.UUID
c.name = name
c.accountMtx.Unlock()

case true:
// Set a default account id.
c.accountMtx.Lock()
c.account = defaultAccountID

c.name = username
c.accountMtx.Unlock()
}

c.statusMtx.Lock()
Expand Down Expand Up @@ -614,7 +621,7 @@ func (c *Client) handleSubmitWorkRequest(ctx context.Context, req *Request, allo
// Create accepted work if the work submission is accepted
// by the mining node.
work := NewAcceptedWork(hash.String(), header.PrevBlock.String(),
header.Height, c.account, miner)
header.Height, c.FetchAccountID(), miner)
err = c.cfg.db.persistAcceptedWork(work)
if err != nil {
// If the submitted accepted work already exists, ignore the
Expand Down Expand Up @@ -916,6 +923,9 @@ func (c *Client) FetchMinerType() string {

// FetchAccountID gets the client's account ID.
func (c *Client) FetchAccountID() string {
defer c.accountMtx.RUnlock()
c.accountMtx.RLock()

return c.account
}

Expand Down Expand Up @@ -968,11 +978,12 @@ func (c *Client) hashMonitor() {
miner := c.miner
c.mtx.RUnlock()

hashID := hashDataID(c.account, c.extraNonce1)
account := c.FetchAccountID()
hashID := hashDataID(account, c.extraNonce1)
hashData, err := c.cfg.db.fetchHashData(hashID)
if err != nil {
if errors.Is(err, errs.ValueNotFound) {
hashData = newHashData(miner, c.account, c.addr.String(),
hashData = newHashData(miner, account, c.addr.String(),
c.extraNonce1, hash)
err = c.cfg.db.persistHashData(hashData)
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions pool/endpoint.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2019-2021 The Decred developers
// Copyright (c) 2019-2023 The Decred developers
// Use of this source code is governed by an ISC
// license that can be found in the LICENSE file.

Expand Down Expand Up @@ -221,7 +221,7 @@ func (e *Endpoint) generateHashIDs() map[string]struct{} {

ids := make(map[string]struct{}, len(e.clients))
for _, c := range e.clients {
hashID := hashDataID(c.account, c.extraNonce1)
hashID := hashDataID(c.FetchAccountID(), c.extraNonce1)
ids[hashID] = struct{}{}
}

Expand Down

0 comments on commit 3a35654

Please sign in to comment.