waddrmgr: avoid in-memory side effects of ImportAccountDryRun

[starius]

Change Description

Fix two in-memory side effects in ImportAccountDryRun that survive the dry-run DB rollback.

Problem 1 -- address cache leak. nextAddresses calls loadAndCacheAddress for a post-write round-trip check, which populates ScopedKeyManager.addrs before commit. On dry-run rollback these entries survive, so Manager.Address(addr) succeeds from cache while Manager.AddrAccount(addr) fails from the rolled-back DB -- a split-brain inconsistency.

Problem 2 -- stale deriveOnUnlock entries. During dry-run, loadAccountInfo calls keyToManaged for the account's last derived addresses, which appends to deriveOnUnlock when the derived key is public-only. After rollback these entries reference a non-existent account, causing the next Manager.Unlock to fail with account N not found.

Fix (commit 1): Thread an onUnlock parameter through the address-loading call chain (keyToManaged, chainAddressRowToManaged, rowInterfaceToManaged, loadAndCacheAddress) so callers can skip the deriveOnUnlock append. Add a loadAddress helper that uses this to perform the round-trip read-back check without any cache or deriveOnUnlock side effects, preserving the full address mismatch validation. Also extend InvalidateAccountCache to filter deriveOnUnlock entries for the invalidated account.

Fix (commit 2): Add TestImportAccountDryRunNoAddrLeak covering both scenarios: watch-only wallet (cache leak / split-brain) and normal wallet (stale deriveOnUnlock breaking Lock/Unlock).

Steps to Test

go test ./waddrmgr/ -count=1
go test ./wallet/ -run TestImportAccountDryRunNoAddrLeak -v -count=1 -timeout 600s
go test ./wallet/ -run TestImportAccount -count=1 -timeout 600s

The new test has two subtests:

• watch-only: dry-run preview addresses must not be visible via HaveAddress; AccountOfAddress must return ErrAddressNotFound consistently (no split-brain). A subsequent real import and address derivation must work normally.
• normal wallet unlock after dry-run: Lock then Unlock must succeed after a dry-run import (previously failed with account 1 not found).

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change is not insubstantial. Typo fixes are not accepted to fight bot spam.
• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.

[Copilot]

Pull request overview

Fixes ImportAccountDryRun leaving behind in-memory state after the dry-run DB transaction is rolled back, preventing post-dry-run inconsistencies (address cache “split-brain”) and unlock failures due to stale deriveOnUnlock entries.

Changes:

• Add “on-unlock side effect” toggles to the address-loading/row-to-managed pipeline to avoid mutating deriveOnUnlock during dry-run readback paths.
• Update nextAddresses to perform the disk round-trip verification without caching preview addresses.
• Extend InvalidateAccountCache to also remove pending deriveOnUnlock entries for the invalidated (dry-run) account; add a regression test covering both reported scenarios.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
wallet/import_test.go	Adds TestImportAccountDryRunNoAddrLeak to ensure dry-run imports don’t leak address cache state and don’t break subsequent lock/unlock.
waddrmgr/scoped_manager.go	Introduces “OnUnlock” variants in the managed-address creation/loading chain, avoids caching during disk read-back checks, and filters stale deriveOnUnlock entries when invalidating an account cache.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

When filtering deriveOnUnlock with the [:0] pattern, the removed tail entries remain referenced in the underlying array. That can retain ManagedAddress objects (and any associated key material) longer than necessary. Consider explicitly nil-ing the truncated tail before re-slicing/assigning, or allocating a new slice, so removed entries can be GC’d promptly.

Suggested change

	// entries would cause the next Unlock to fail with account not found.
	filtered := s.deriveOnUnlock[:0]
	for _, info := range s.deriveOnUnlock {
	if info.managedAddr.InternalAccount() != account {
	filtered = append(filtered, info)
	}
	}
	// entries would cause the next Unlock to fail with account not found.
	origLen := len(s.deriveOnUnlock)
	filtered := s.deriveOnUnlock[:0]
	for _, info := range s.deriveOnUnlock {
	if info.managedAddr.InternalAccount() != account {
	filtered = append(filtered, info)
	}
	}
	var zeroInfo unlockDeriveInfo
	for i := len(filtered); i < origLen; i++ {
	s.deriveOnUnlock[i] = zeroInfo
	}

[starius]

Fixed. I allocated new slice. This code is not performance critical, so we don't need this slice-reuse optimization here. Zeroing code looks too heavy to use it, so I decided just to make a new slice.

[Copilot]

This test picks the BIP84 case via testCases[len(testCases)-1], which makes it order-dependent and easy to break when new cases are appended. Consider selecting the desired test case by matching on tc.name/expectedScope/accountIndex instead of relying on slice ordering.

Suggested change

	tc := testCases[len(testCases)-1] // bip84, accountIndex=1
	var tc *testCase
	for i := range testCases {
	if testCases[i].accountIndex == 1 &&
	testCases[i].expectedScope == waddrmgr.KeyScopeBIP0084Plus {
	tc = &testCases[i]
	break
	}
	}
	require.NotNil(t, tc, "expected bip84 test case with accountIndex=1")

[starius]

Good catch! Fixed.

[Abdulkbk]

LGTM

just one non-blocking nit.

[Abdulkbk]

nit: 10 mins seem excessive for this test

[starius]

10 mins it is the time after which the wallet will lock again. If we choose too small value, it might become flaky in CI, so I propose to keep 10 mins.