Skip to content

Commit

Permalink
wallet: add address-level mutex to avoid duplicates
Browse files Browse the repository at this point in the history
Because the way the scoped manager uses the OnCommit callback of the
database transaction mechanism, the manager's lock is released first,
then re-acquired in the OnCommit. But during that short time of
releasing the lock, another goroutine might already have updated the
in-memory state.
We add another mutex around the whole database transaction to address
the problem.
  • Loading branch information
guggero committed Jul 16, 2024
1 parent db9f7b4 commit b901f0b
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 0 deletions.
9 changes: 9 additions & 0 deletions wallet/import.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,15 @@ func (w *Wallet) ImportAccountDryRun(name string,
*waddrmgr.AccountProperties, []waddrmgr.ManagedAddress,
[]waddrmgr.ManagedAddress, error) {

// The address manager uses OnCommit on the walletdb tx to update the
// in-memory state of the account state. But because the commit happens
// _after_ the account manager internal lock has been released, there
// is a chance for the address index to be accessed concurrently, even
// though the closure in OnCommit re-acquires the lock. To avoid this
// issue, we surround the whole address creation process with a lock.
w.newAddrMtx.Lock()
defer w.newAddrMtx.Unlock()

var (
accountProps *waddrmgr.AccountProperties
externalAddrs []waddrmgr.ManagedAddress
Expand Down
20 changes: 20 additions & 0 deletions wallet/wallet.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ type Wallet struct {
chainClientSynced bool
chainClientSyncMtx sync.Mutex

newAddrMtx sync.Mutex

lockedOutpoints map[wire.OutPoint]struct{}
lockedOutpointsMtx sync.Mutex

Expand Down Expand Up @@ -3153,6 +3155,15 @@ func (w *Wallet) NewAddress(account uint32,
return nil, err
}

// The address manager uses OnCommit on the walletdb tx to update the
// in-memory state of the account state. But because the commit happens
// _after_ the account manager internal lock has been released, there
// is a chance for the address index to be accessed concurrently, even
// though the closure in OnCommit re-acquires the lock. To avoid this
// issue, we surround the whole address creation process with a lock.
w.newAddrMtx.Lock()
defer w.newAddrMtx.Unlock()

var (
addr btcutil.Address
props *waddrmgr.AccountProperties
Expand Down Expand Up @@ -3211,6 +3222,15 @@ func (w *Wallet) NewChangeAddress(account uint32,
return nil, err
}

// The address manager uses OnCommit on the walletdb tx to update the
// in-memory state of the account state. But because the commit happens
// _after_ the account manager internal lock has been released, there
// is a chance for the address index to be accessed concurrently, even
// though the closure in OnCommit re-acquires the lock. To avoid this
// issue, we surround the whole address creation process with a lock.
w.newAddrMtx.Lock()
defer w.newAddrMtx.Unlock()

var addr btcutil.Address
err = walletdb.Update(w.db, func(tx walletdb.ReadWriteTx) error {
addrmgrNs := tx.ReadWriteBucket(waddrmgrNamespaceKey)
Expand Down

0 comments on commit b901f0b

Please sign in to comment.