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 17, 2024
1 parent db9f7b4 commit b3fc760
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 0 deletions.
12 changes: 12 additions & 0 deletions wallet/createtx.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,18 @@ func (w *Wallet) txToOutputs(outputs []*wire.TxOut,
strategy = CoinSelectionLargest
}

// The addrMgrWithChangeSource function of the wallet creates a
// new change address. 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 tx *txauthor.AuthoredTx
err = walletdb.Update(w.db, func(dbtx walletdb.ReadWriteTx) error {
addrmgrNs, changeSource, err := w.addrMgrWithChangeSource(
Expand Down
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
13 changes: 13 additions & 0 deletions wallet/psbt.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,17 @@ func (w *Wallet) FundPsbt(packet *psbt.Packet, keyScope *waddrmgr.KeyScope,
opts.changeKeyScope = keyScope
}

// The addrMgrWithChangeSource function of the wallet creates a
// new change address. 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()

// We also need a change source which needs to be able to insert
// a new change address into the database.
err = walletdb.Update(w.db, func(dbtx walletdb.ReadWriteTx) error {
Expand All @@ -190,6 +201,8 @@ func (w *Wallet) FundPsbt(packet *psbt.Packet, keyScope *waddrmgr.KeyScope,

return nil
})
w.newAddrMtx.Unlock()

if err != nil {
return 0, fmt.Errorf("could not add change address to "+
"database: %w", err)
Expand Down
29 changes: 29 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 @@ -1693,6 +1695,15 @@ func (w *Wallet) CurrentAddress(account uint32, scope waddrmgr.KeyScope) (btcuti
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 @@ -3153,6 +3164,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 +3231,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 b3fc760

Please sign in to comment.