wallet: Avoid duplicate address creation

[guggero]

Fixes lightningnetwork/lnd#8697.

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.

[yyforyongyu]

Thanks for the fix!
Tried to run the test after the fix but got a timeout,

go test -v -timeout=10s -run=TestDuplicateAddressDerivation

Looks like there's a dead lock?

[guggero]

Thanks for the fix! Tried to run the test after the fix but got a timeout,

go test -v -timeout=10s -run=TestDuplicateAddressDerivation

Looks like there's a dead lock?

No, it just takes quite a while to derive 10x10x10 addresses... Can you try again with something like 30 seconds or more?

[Roasbeef]

If I run the test w/o the second commit, I hit it pretty quickly:

=== RUN   TestDuplicateAddressDerivation
    wallet_test.go:359:
                Error Trace:    /Users/roasbeef/gocode/src/github.com/btcsuite/btcwallet/wallet/wallet_test.go:359
                Error:          Received unexpected error:
                                duplicate address! already have tb1qagh3rua975yy0shaw9nhgnm6avaect9te6np4v, want to add tb1qagh3rua975yy0shaw9nhgnm6avaect9te6np4v
                Test:           TestDuplicateAddressDerivation
--- FAIL: TestDuplicateAddressDerivation (3.39s)
FAIL
exit status 1
FAIL    github.com/btcsuite/btcwallet/wallet    3.699s

[yyforyongyu]

Cool the test passed,

=== RUN   TestDuplicateAddressDerivation
--- PASS: TestDuplicateAddressDerivation (20.86s)
PASS
ok      github.com/btcsuite/btcwallet/wallet    21.450s

Since deep down they are all calling nextAddresses, also noticed that,

• NewChangeAddress -> newChangeAddress -> NextInternalAddresses
• CurrentAddress -> newAddress -> NextExternalAddresses

Should we also update these calls?

The alternative, instead of locking over locks, we can make these indices atomic, not sure if it helps,

btcwallet/waddrmgr/manager.go

Lines 157 to 165 in e391a1c

	// The external branch is used for all addresses which are intended for
	// external use.
	nextExternalIndex uint32
	lastExternalAddr ManagedAddress
	// The internal branch is used for all adddresses which are only
	// intended for internal wallet use such as change addresses.
	nextInternalIndex uint32
	lastInternalAddr ManagedAddress

[Roasbeef]

@yyforyongyu I don't think that's enough (making the variables atomic) as we have other operations in the OnCommit call back:

btcwallet/waddrmgr/scoped_manager.go

Lines 1199 to 1209 in e391a1c

	for _, info := range addressInfo {
	ma := info.managedAddr
	s.addrs[addrKey(ma.Address().ScriptAddress())] = ma
	// Add the new managed address to the list of addresses
	// that need their private keys derived when the
	// address manager is next unlocked.
	if s.rootManager.isLocked() && !watchOnly {
	s.deriveOnUnlock = append(s.deriveOnUnlock, info)
	}
	}

[guggero]

Thanks for taking a closer look, @yyforyongyu !

You are correct, I missed a couple of paths. I now traced back the s.nextAddresses call sites to any place where we start a transaction walletdb.Update() and wrapped it with an additional lock.

Here are the ones I found and now added the new mutex to:

w.ImportAccountDryRun                                            -> s.NextInternalAddresses -> s.nextAddresses
w.NewChangeAddress                         -> w.newChangeAddress -> s.NextInternalAddresses -> s.nextAddresses
w.txToOutputs -> w.addrMgrWithChangeSource -> w.newChangeAddress -> s.NextInternalAddresses -> s.nextAddresses
w.FundPsbt    -> w.addrMgrWithChangeSource -> w.newChangeAddress -> s.NextInternalAddresses -> s.nextAddresses

w.ImportAccountDryRun            -> s.NextExternalAddresses -> s.nextAddresses
w.NewAddress     -> w.newAddress -> s.NextExternalAddresses -> s.nextAddresses
w.CurrentAddress -> w.newAddress -> s.NextExternalAddresses -> s.nextAddresses

I hope that's all of them.

[yyforyongyu]

LGTM🙏 I think before merging this we can create a PR in lnd first to see if the CI passes.

[Roasbeef]

CI passing on the lnd itests, so moving to merge this.

[Roasbeef]

LGTM 💰