Merge pull request #941 from guggero/duplicate-addr-fix

wallet: Avoid duplicate address creation

go.mod

@@ -26,25 +26,23 @@ require (
 	github.com/stretchr/testify v1.8.4
 	golang.org/x/crypto v0.7.0
 	golang.org/x/net v0.10.0
+	golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f
 	golang.org/x/term v0.8.0
 	google.golang.org/grpc v1.53.0
 )
 
 require (
 	github.com/aead/siphash v1.0.1 // indirect
 	github.com/btcsuite/go-socks v0.0.0-20170105172521-4720035b7bfd // indirect
-	github.com/btcsuite/winsvc v1.0.0 // indirect
 	github.com/decred/dcrd/crypto/blake256 v1.0.0 // indirect
 	github.com/decred/dcrd/lru v1.0.0 // indirect
-	github.com/golang/snappy v0.0.4 // indirect
 	github.com/kkdai/bstream v1.0.0 // indirect
 	github.com/kr/pretty v0.3.0 // indirect
 	github.com/lightningnetwork/lnd/clock v1.0.1 // indirect
 	github.com/lightningnetwork/lnd/queue v1.0.1 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/rogpeppe/go-internal v1.9.0 // indirect
 	github.com/stretchr/objx v0.5.0 // indirect
-	github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7 // indirect
 	go.etcd.io/bbolt v1.3.7 // indirect
 	golang.org/x/sys v0.8.0 // indirect
 	golang.org/x/text v0.9.0 // indirect

go.sum

@@ -4,8 +4,6 @@ github.com/btcsuite/btcd v0.20.1-beta/go.mod h1:wVuoA8VJLEcwgqHBwHmzLRazpKxTv13P
 github.com/btcsuite/btcd v0.22.0-beta.0.20220111032746-97732e52810c/go.mod h1:tjmYdS6MLJ5/s0Fj4DbLgSbDHbEqLJrtnHecBFkdz5M=
 github.com/btcsuite/btcd v0.22.0-beta.0.20220207191057-4dc4ff7963b4/go.mod h1:7alexyj/lHlOtr2PJK7L/+HDJZpcGDn/pAU98r7DY08=
 github.com/btcsuite/btcd v0.23.5-0.20231215221805-96c9fd8078fd/go.mod h1:nm3Bko6zh6bWP60UxwoT5LzdGJsQJaPo6HjduXq9p6A=
-github.com/btcsuite/btcd v0.24.1-0.20240318151728-2fc99e0496d2 h1:b7EiiYEZypI2id3516ptqjzhUfFAgNfF4YVtxikAg6Y=
-github.com/btcsuite/btcd v0.24.1-0.20240318151728-2fc99e0496d2/go.mod h1:5C8ChTkl5ejr3WHj8tkQSCmydiMEPB0ZhQhehpq7Dgg=
 github.com/btcsuite/btcd v0.24.2-beta.rc1.0.20240625142744-cc26860b4026 h1:s8/96vQSj05bqLl9RyM/eMX8gLtiayEj520TVE4YGy0=
 github.com/btcsuite/btcd v0.24.2-beta.rc1.0.20240625142744-cc26860b4026/go.mod h1:5C8ChTkl5ejr3WHj8tkQSCmydiMEPB0ZhQhehpq7Dgg=
 github.com/btcsuite/btcd/btcec/v2 v2.1.0/go.mod h1:2VzYrv4Gm4apmbVVsSq5bqf1Ec8v56E48Vt0Y/umPgA=
@@ -43,7 +41,6 @@ github.com/btcsuite/snappy-go v0.0.0-20151229074030-0bdef8d06723/go.mod h1:8woku
 github.com/btcsuite/snappy-go v1.0.0/go.mod h1:8woku9dyThutzjeg+3xrA5iCpBRH8XEEg3lh6TiUghc=
 github.com/btcsuite/websocket v0.0.0-20150119174127-31079b680792 h1:R8vQdOQdZ9Y3SkEwmHoWBmX1DNXhXZqlTpq6s4tyJGc=
 github.com/btcsuite/websocket v0.0.0-20150119174127-31079b680792/go.mod h1:ghJtEyQwv5/p4Mg4C0fgbePVuGr935/5ddU9Z3TmDRY=
-github.com/btcsuite/winsvc v1.0.0 h1:J9B4L7e3oqhXOcm+2IuNApwzQec85lE+QaikUcCs+dk=
 github.com/btcsuite/winsvc v1.0.0/go.mod h1:jsenWakMcC0zFBFurPLEAyrnc/teJEM1O46fmI40EZs=
 github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
 github.com/davecgh/go-spew v0.0.0-20171005155431-ecdeabc65495/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
@@ -147,6 +144,7 @@ golang.org/x/net v0.0.0-20200520004742-59133d7f0dd7/go.mod h1:qpuaurCH72eLCgpAm/
 golang.org/x/net v0.0.0-20200813134508-3edf25e44fcc/go.mod h1:/O7V0waA8r7cgGh81Ro3o1hOxt32SMVPicZroKQ2sZA=
 golang.org/x/net v0.10.0 h1:X2//UzNDwYmtCLn7To6G58Wr6f5ahEAQgKNzv9Y951M=
 golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg=
+golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f h1:wMNYb4v58l5UBM7MYRLPG6ZhfOqbKu7X5eyFl8ZhKvA=
 golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sys v0.0.0-20180909124046-d0be0721c37e/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=

wallet/createtx.go

@@ -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(

wallet/import.go

@@ -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

wallet/psbt.go

@@ -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 {
@@ -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)

wallet/wallet.go

@@ -133,6 +133,8 @@ type Wallet struct {
 	chainClientSynced  bool
 	chainClientSyncMtx sync.Mutex
 
+	newAddrMtx sync.Mutex
+
 	lockedOutpoints    map[wire.OutPoint]struct{}
 	lockedOutpointsMtx sync.Mutex
 
@@ -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
@@ -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
@@ -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)

wallet/wallet_test.go

@@ -2,15 +2,18 @@ package wallet
 
 import (
 	"encoding/hex"
+	"fmt"
+	"sync"
 	"testing"
 	"time"
 
+	"github.com/btcsuite/btcd/btcutil"
+	"github.com/btcsuite/btcd/chaincfg/chainhash"
+	"github.com/btcsuite/btcwallet/waddrmgr"
 	"github.com/btcsuite/btcwallet/walletdb"
 	"github.com/btcsuite/btcwallet/wtxmgr"
 	"github.com/stretchr/testify/require"
-
-	"github.com/btcsuite/btcd/btcutil"
-	"github.com/btcsuite/btcd/chaincfg/chainhash"
+	"golang.org/x/sync/errgroup"
 )
 
 var (
@@ -305,3 +308,54 @@ func TestGetTransaction(t *testing.T) {
 		})
 	}
 }
+
+// TestDuplicateAddressDerivation tests that duplicate addresses are not
+// derived when multiple goroutines are concurrently requesting new addresses.
+func TestDuplicateAddressDerivation(t *testing.T) {
+	w, cleanup := testWallet(t)
+	defer cleanup()
+
+	var (
+		m           sync.Mutex
+		globalAddrs = make(map[string]btcutil.Address)
+	)
+
+	for o := 0; o < 10; o++ {
+		var eg errgroup.Group
+
+		for n := 0; n < 10; n++ {
+			eg.Go(func() error {
+				addrs := make([]btcutil.Address, 10)
+				for i := 0; i < 10; i++ {
+					addr, err := w.NewAddress(
+						0, waddrmgr.KeyScopeBIP0084,
+					)
+					if err != nil {
+						return err
+					}
+
+					addrs[i] = addr
+				}
+
+				m.Lock()
+				defer m.Unlock()
+
+				for idx := range addrs {
+					addrStr := addrs[idx].String()
+					if a, ok := globalAddrs[addrStr]; ok {
+						return fmt.Errorf("duplicate "+
+							"address! already "+
+							"have %v, want to "+
+							"add %v", a, addrs[idx])
+					}
+
+					globalAddrs[addrStr] = addrs[idx]
+				}
+
+				return nil
+			})
+		}
+
+		require.NoError(t, eg.Wait())
+	}
+}