From db9f7b4757476700e41965fa55ac97b4e7dbf9ce Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Tue, 16 Jul 2024 09:57:52 +0200 Subject: [PATCH 1/2] mod+wallet: add test to demonstrate duplicate addr issue This commit adds a test that demonstrates that duplicate addresses can be created in the wallet before the fix. --- go.mod | 4 +-- go.sum | 4 +-- wallet/wallet_test.go | 60 ++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 59 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 5bab78504b..904210a336 100644 --- a/go.mod +++ b/go.mod @@ -26,6 +26,7 @@ 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 ) @@ -33,10 +34,8 @@ require ( 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 @@ -44,7 +43,6 @@ require ( 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 diff --git a/go.sum b/go.sum index e860fabdfa..67608078ce 100644 --- a/go.sum +++ b/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= diff --git a/wallet/wallet_test.go b/wallet/wallet_test.go index bf479d7735..1a06777a6d 100644 --- a/wallet/wallet_test.go +++ b/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()) + } +} From b3fc760adea1c5356bba4c27d7f5c8913bb7af68 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Tue, 16 Jul 2024 09:58:32 +0200 Subject: [PATCH 2/2] wallet: add address-level mutex to avoid duplicates 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. --- wallet/createtx.go | 12 ++++++++++++ wallet/import.go | 9 +++++++++ wallet/psbt.go | 13 +++++++++++++ wallet/wallet.go | 29 +++++++++++++++++++++++++++++ 4 files changed, 63 insertions(+) diff --git a/wallet/createtx.go b/wallet/createtx.go index 9c2b061984..3fe133e16f 100644 --- a/wallet/createtx.go +++ b/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( diff --git a/wallet/import.go b/wallet/import.go index 9f37e93d90..3bbcf97dcb 100644 --- a/wallet/import.go +++ b/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 diff --git a/wallet/psbt.go b/wallet/psbt.go index 846534c09d..2151858954 100644 --- a/wallet/psbt.go +++ b/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) diff --git a/wallet/wallet.go b/wallet/wallet.go index fb7f10419b..96891f0c3e 100644 --- a/wallet/wallet.go +++ b/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)