# BCR Agent — Workshop #32489

**Model:** zai/glm-5.2
**Mode:** autonomous
**Version:** [`1e9e334`](https://github.com/Amperstrand/bcr-agent/commit/1e9e334)
**Machine:** cpx32, 4 vCPU, 7.6Gi RAM
**Generated:** 2026-06-17T17:02:09Z

---


---

# Summary — Workshop #32489: `wallet: Add exportwatchonlywallet RPC`

**Reviewer:** autonomous (GLM-5.2) · **PR:** [bitcoin/bitcoin#32489](https://github.com/bitcoin/bitcoin/pull/32489) by achow101, hosted by ryanofsky · **Verdict: Concept ACK + Approach ACK (tested)**

## What was actually done

- **Recovered the PR**: the workspace repo was on a 2019 master with no PR tag; fetched `pr32489` directly from `bitcoin/bitcoin`, checked out, and confirmed the 5-commit diff (10 files, ~817 lines).
- **Built** a Debug `bitcoind` (`cmake -DCMAKE_BUILD_TYPE=Debug -DENABLE_IPC=OFF`, `make -j$(nproc) bitcoind`) — ~4 min, clean.
- **Ran** `test/functional/wallet_exported_watchonly.py` — **PASSED** (all 6 sub-tests).
- **Ran a custom security/behavior experiment** (`fun_experiment.py`) — **PASSED**.

## Confidence per question

| # | Topic | Confidence | Basis |
|---|---|---|---|
| 1 | Review approach (test/concept ACK) | **High** | Built + ran tests; traced export algorithm & refactor. |
| 2 | `CanSelfExpand` vs `IsRange`/`IsSingleType`; wpkh(0h/*) vs pkh(pubkey) | **High** | Read all 5 overrides + `IsHardened` + derivation paths. |
| 3 | Descriptor cache contents & incomplete-cache effect | **High** | Read `DescriptorCache` fields, `GetPubKey` write logic, exhaustion test. |
| 4 | Preserving `AVOID_REUSE` & flags | **High** | Traced `AvailableCoins`→`IsSpentKey`→`previously_spent` + copied markers. |
| 5 | Best-block locator copied verbatim | **High** | Read export code + wallet scan/locator usage; no-rescan rationale confirmed. |
| 6 | Multisig privacy delta | **High** | Determined unhardened ⇒ no cache; enumerated copied metadata. |
| 7 | Keypool top-up before spendability check | **High** | Found `TopUpKeyPool` in RPC (test doesn't call `keypoolrefill`); empirically confirmed exhaustion. |
| 8 | `dumpwallet`/`importdescriptors` vs new RPC | **High** | Verified dump RPCs removed; only `bitcoin-wallet dump` survives (with privkeys). |

## Key findings

- **`CanSelfExpand()`** is the conceptual core: it recursively detects *hardened derivation*, the one thing `IsRange()`/`IsSingleType()` can't see, and which forces private-key-free wallets to rely on a copied cache.
- **The cache** holds parent/last-hardened/derived `CExtPubKey`s; for fully-hardened ranged descriptors it's a finite per-index list → watch-only wallets are **keypool-bounded** (empirically: exactly 5 addresses with `-keypool=5`, vs unlimited for the key-holding source).
- **Security verified empirically**: exported watch-only wallet has `private_keys_enabled=False`, public descriptors byte-match the source, and `listdescriptors(private=true)` is **rejected** (`-4 Can't get private descriptor string for watch-only wallets`).
- **Rescan-free**: copying the best-block locator + all transactions makes `restorewallet` immediately useful — the central UX win.
- **Flags preserved** (`GetWalletFlags() | WALLET_FLAG_DISABLE_PRIVATE_KEYS`) so `AVOID_REUSE` balance behavior and copied `previously_spent` markers stay consistent.

## Honest caveats

- Q7's stated `wallet.keypoolrefill(100)` is **not** in the test; the real mechanism is the RPC's internal `TopUpKeyPool()` — noted explicitly in the answer.
- No prior lineage existed for *this* workshop (#32489); the `/workspace/lineage/*.bin` files are all from #33300. Build-system knowledge still applied.
- This is a feature PR with no bug/crash to revert; the fun exercise was a security/behavior experiment instead of crash reproduction.

## Output files
`q1.md`–`q8.md`, `journal.md`, `recommendations.md`, `fun_exercise.md`, `fun_experiment.py`, build/test logs in `/workspace/results/`.

---

# Q1 — Review approach: Did you test, concept ACK, approach ACK, or NACK the PR?

## Verdict: **Concept ACK + Approach ACK** (tested)

I actually built the code and ran the test suite, so this is a tested review rather than a pure concept ACK.

## What I did

1. **Fetched the PR** directly from `bitcoin/bitcoin` (`refs/pull/32489/head`), since the workspace repo was on a 2019 master with no `pr32489` tag.
2. **Built a Debug `bitcoind`**: `cmake -B build -S . -DCMAKE_BUILD_TYPE=Debug -DENABLE_IPC=OFF` then `make -j$(nproc) bitcoind`. Compiled cleanly in ~4 min → `build/bin/bitcoind`.
3. **Ran the functional test** `test/functional/wallet_exported_watchonly.py` — **PASSED** (all 6 sub-tests: basic export, address-book copy, tx/locked-coin copy, imported descriptors incl. hardened/multisig/taproot/miniscript, avoid-reuse, encrypted source).

## Aspects I focused on

- **The new `Descriptor::CanSelfExpand()` API** (`src/script/descriptor.cpp:236,307,372,599,1101`) — the conceptual heart of the PR. I traced every override and the `IsHardened()` logic (`src/script/descriptor.cpp:416`) it depends on.
- **The export algorithm** in `src/wallet/export.cpp:41-217`: temp-wallet creation, descriptor re-parsing, conditional cache copy (`export.cpp:129`), locked-coin copy, atomic `WalletBatch` transaction block, best-block locator verbatim copy, and `BackupWallet` to destination.
- **Refactor quality**: the `listdescriptors` RPC body was cleanly extracted into the reusable `ExportDescriptors()` free function (`src/wallet/export.cpp:18`), with `CHECK_NONFATAL` upgraded to a proper `Assume()` + `util::Unexpected` error path (`src/wallet/rpc/backup.cpp`). This is an improvement.
- **Correctness of the cache copy**: only copied when `!desc->CanSelfExpand()` (`export.cpp:129`), which is exactly the condition under which the watch-only side cannot regenerate the keys.
- **Resource safety**: the `MakeCleanupHandler` lambda (`export.cpp:96-103`) guarantees the temp wallet dir and its files are deleted even on early error return — good defensive design, and `BackupWallet` is now `[[nodiscard]]` (`src/wallet/wallet.h:885`) which prevents silently ignoring backup failures.
- **Test coverage**: the test exercises bad-args, descriptor equality across online/offline, PSBT sign-offline/broadcast-online flow, address book (purpose + label + receive requests), persistent vs. temporary locks, hardened-descriptor keypool exhaustion (`assert_raises_rpc_error(-12, "No addresses available")`), and avoid-reuse propagation.

## Minor observations (not NACK-worthy)

- The RPC calls `pwallet->TopUpKeyPool()` (`src/wallet/rpc/wallet.cpp:951`) before exporting, which is the right thing to do for hardened descriptors but is not documented in the RPC help text.
- `assert(dummy_keys.keys.size() == 0)` (`export.cpp:127`) guards against private keys leaking into exported descriptors — a good security assertion.

Overall the change is self-contained, builds on descriptor-only wallet infrastructure, and the test demonstrates a complete end-to-end offline-signing workflow. Concept and approach are sound.

---

# Q2 — Why IsRange()/IsSingleType() are insufficient; CanSelfExpand() logic

## Why the existing methods don't answer "can it expand on the watch-only side?"

- **`IsRange()`** (`src/script/descriptor.cpp:427`) only reports whether the descriptor contains a `*` (i.e. can produce *more than one* address). It says **nothing about *how* those addresses are derived**.
- **`IsSingleType()`** reports whether all derivations yield the same output script type — purely a script-shape question, unrelated to key derivation.

The real question the exporter must answer is: *"Can new scriptPubKeys be produced using **only public information** (no private keys, no pre-derived cache)?"* Neither method addresses the actual blocker, which is **hardened derivation**. Hardened child derivation (`index | 0x80000000`) requires the **parent private key** — an xpub alone cannot compute hardened children. A descriptor can be ranged and single-type yet still be un-expandable on the watch-only side if any derivation step is hardened.

`CanSelfExpand()` (`src/script/descriptor.cpp:236`, base virtual) was added precisely to answer this: it recursively walks the descriptor/pubkey-providers and returns `false` the moment it finds anything that needs a privkey or cache to derive.

## (a) `wpkh(xpub/0h/*)` — a hardened path

This parses into a `BIP32PubkeyProvider` with `m_path = [0h]` and `m_derive = UNHARDENED_RANGED` (the trailing `*` is unhardened).

`CanSelfExpand()` for BIP32 → `return !IsHardened();` (`src/script/descriptor.cpp:599`).
`IsHardened()` (`src/script/descriptor.cpp:416-423`) returns **true** here, because it scans the path and finds the hardened entry `0h` (`entry >> 31` is set at line 420).

Therefore `CanSelfExpand()` returns **false**.

**Why**: To compute any address at index `i`, the wallet must first derive the xpub at `0h`. Hardened derivation needs the parent xprv; the watch-only wallet has only the root xpub. So without a **cached** xpub-at-0h (see Q3), the wallet is stuck — it cannot get past the `0h` step to then do the unhardened `*` derivation. Hence the exporter must copy the cache for this descriptor (`src/wallet/export.cpp:129`).

## (b) `pkh(pubkey)` — a constant single key

This parses into a `ConstPubkeyProvider` holding a literal `CPubKey`.

`CanSelfExpand()` → `return true;` (`src/script/descriptor.cpp:372`, `final`).

**Why**: There is no derivation at all — `IsRange()` is false (`descriptor.cpp:335`), `GetPubKey` just returns the stored pubkey (`descriptor.cpp:333`). It needs neither a private key nor a cache to produce its (single) scriptPubKey, so it trivially "self-expands". (Note: this does **not** mean it can generate *new* addresses — it can't, it's not ranged. `CanSelfExpand` only means "no privkey/cache needed to produce its outputs".)

This confirms the host's clarification in the IRC log: *"pkh(pubkey) should self-expand… Anything that doesn't include a hardened path should self-expand."* The earlier confusion in the meeting (`stringintech` initially said pkh(pubkey) could not expand) was a mix-up with `CanGetAddresses()`, which is a different (wallet-level) concept checked at `src/wallet/scriptpubkeyman.cpp:1175`.

## The recursive plumbing

`DescriptorImpl::CanSelfExpand()` (`src/script/descriptor.cpp:1101-1110`) recurses over both `m_pubkey_args` and `m_subdescriptor_args`, so composite descriptors like `wsh(multi(...))` or `tr(...)` correctly report false if *any* nested key path is hardened. `OriginPubkeyProvider` (`descriptor.cpp:307`) delegates to its wrapped provider since the `[origin]` metadata doesn't affect derivation.

---

# Q3 — What's stored in the descriptor cache, and how an incomplete cache affects derivation

## When the cache is copied

`ExportWatchOnlyWallet` only copies the cache when the descriptor cannot self-expand:

```cpp
if (!w_desc.descriptor->CanSelfExpand()) {
    DescriptorScriptPubKeyMan* desc_spkm = dynamic_cast<DescriptorScriptPubKeyMan*>(wallet.GetScriptPubKeyMan(desc_id));
    w_desc.cache = WITH_LOCK(desc_spkm->cs_desc_man, return desc_spkm->GetWalletDescriptor().cache);
}
```
(`src/wallet/export.cpp:129-132`)

This is exactly the hardened-derivation case from Q2: a descriptor whose path contains hardened steps needs a cache, because the watch-only side cannot recompute hardened children from public data.

## What the cache actually contains

`DescriptorCache` (`src/script/descriptor.h:19`) holds three maps of `CExtPubKey` objects, keyed by the key-expression's position within the descriptor (`m_expr_index`) and (for derived keys) by derivation index:

1. **`m_parent_xpubs`** (`descriptor.h:24`) — the **parent xpub** at the point just after the last hardened step, i.e. the xpub from which unhardened `*` children can be derived. Written by `CacheParentExtPubKey` (`src/script/descriptor.cpp:479`).
2. **`m_last_hardened_xpubs`** (`descriptor.h:26`) — the xpub **immediately after the last hardened derivation step**. Written when a private key was available (`CacheLastHardenedExtPubKey`, `descriptor.cpp:482`). This is the most recently hardened intermediate xpub.
3. **`m_derived_xpubs`** (`descriptor.h:22`) — for a **fully-hardened ranged** descriptor (`HARDENED_RANGED`, e.g. `xpub/0'/*'`), each individual address index must be pre-derived, so the cache stores a map `{derivation_index → final xpub}`. Written by `CacheDerivedExtPubKey` (`src/script/descriptor.cpp:485`).

The caching logic in `BIP32PubkeyProvider::GetPubKey` (`src/script/descriptor.cpp:476-487`) decides which of these to populate:
- If `m_derive != HARDENED_RANGED` (there is some trailing unhardened derivation, e.g. `xpub/0h/*`) → cache the **parent** xpub (+ last-hardened if known).
- Else (`HARDENED_RANGED`, e.g. `xpub/0h/*h`) → cache each **derived** xpub per index.

On read (`descriptor.cpp:444-451`): it first tries `GetCachedDerivedExtPubKey`; if that misses and the range step is hardened, it returns `nullopt` (cannot derive); otherwise it falls back to the cached **parent** xpub and does the unhardened child derivation itself.

## How an incomplete cache hurts the watch-only wallet

The cache is the **only** way a private-key-less wallet can learn addresses past a hardened step. Two failure modes:

- **Fully-hardened ranged (`xpub/0h/*h`)**: every address is a separate cache entry. If only the first N indices were pre-derived/topped-up, the watch-only wallet can see **only those N addresses**. This is exactly what the test verifies — `wallet_exported_watchonly.py:190-192` consumes `KEYPOOL_SIZE - 1` addresses and then hits `assert_raises_rpc_error(-12, "No addresses available")`. An incoming payment to index N+1 would be **invisible** (the wallet won't recognize the scriptPubKey as mine) — equivalent to exceeding the gap limit, as `stickies-v` noted in IRC.

- **Partially-hardened (`xpub/0h/*`)**: only the parent xpub (and last-hardened xpub) is needed. If that parent entry is missing from the cache, `GetPubKey` returns `nullopt` at `descriptor.cpp:448`, so **no** child address can be produced and the whole descriptor becomes unusable on the watch-only side. Copying the parent/last-hardened cache is therefore essential.

In short: for hardened descriptors the cache *is* the derivable key material on the watch-only side. An incomplete cache silently truncates the set of addresses the wallet can match against incoming transactions, causing missed/hidden balances. This is why the RPC calls `TopUpKeyPool()` (`src/wallet/rpc/wallet.cpp:951`) before exporting — to make sure the cache is as full as possible first.

---

# Q4 — Why preserve the original wallet flags (`GetWalletFlags() | WALLET_FLAG_DISABLE_PRIVATE_KEYS`)

```cpp
options.create_flags = wallet.GetWalletFlags() | WALLET_FLAG_DISABLE_PRIVATE_KEYS;
```
(`src/wallet/export.cpp:79`)

`GetWalletFlags()` (`src/wallet/wallet.cpp:1828`) returns the source wallet's full flag set. OR-ing in `WALLET_FLAG_DISABLE_PRIVATE_KEYS` guarantees the new wallet has no private keys, while **preserving every other flag** the user chose — most notably `WALLET_FLAG_AVOID_REUSE`, plus `WALLET_FLAG_DESCRIPTORS`, `WALLET_FLAG_LAST_HARDENED_XPUB_CACHED`, etc. (`src/wallet/wallet.h:150-157`).

## Why clearing all flags and starting fresh would be wrong

### 1. `WALLET_FLAG_AVOID_REUSE` changes balance/coin-selection behaviour — even for a watch-only wallet

This was the key insight from `glozow` in the IRC log. `AvailableCoins` (`src/wallet/spend.cpp`) computes:

```cpp
bool allow_used_addresses = !wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE) || (coinControl && !coinControl->m_avoid_address_reuse);
...
if (!allow_used_addresses && wallet.IsSpentKey(output.scriptPubKey)) {  // spend.cpp:444
    continue;
}
```

When `AVOID_REUSE` is set, coins whose address has been spent before are **excluded** from the available set. `IsSpentKey` (`src/wallet/wallet.cpp:1045-1056`) checks `IsAddressPreviouslySpent(dest)`, which reads the `previously_spent` address-book marker.

Critically, **the exporter copies that `previously_spent` data**:
```cpp
if (entry.previously_spent) watchonly_batch.WriteAddressPreviouslySpent(dest, true);
```
(`src/wallet/export.cpp:205`)

So if the watch-only wallet were created *without* `AVOID_REUSE`, those copied `previously_spent` markers would be ignored, and `getbalance`/`listunspent` would report a **different available balance** than the source wallet — confusing the user (and breaking the test at `wallet_exported_watchonly.py:229-230`, which asserts `avoid_reuse == True` and `reused == True` in the exported wallet). Preserving the flag keeps balance display and coin visibility consistent.

### 2. `WALLET_FLAG_DESCRIPTORS` is mandatory for descriptor wallets

A descriptor-based source wallet (the only kind this RPC targets) has `WALLET_FLAG_DESCRIPTORS` set. Clearing it would create a legacy-wallet DB that can't hold descriptors at all, breaking the whole export.

### 3. Future-proofing / user intent

`AVOID_REUSE` is the only currently-relevant *mutable* flag (`MUTABLE_WALLET_FLAGS`, `wallet.h:159-160`), but preserving the full flag set means any future flag that affects wallet behaviour is automatically carried over, so the exported wallet behaves like the original. As `monlovesmango` noted in IRC: "you never know if future flags might have differences in behavior."

### What about `WALLET_FLAG_DISABLE_PRIVATE_KEYS` being OR-ed rather than replaced?

OR-ing is correct and robust: it forces the watch-only bit on without clobbering the others. There's no flag that should be *cleared* on export except (implicitly) encryption-related private state — and indeed the test confirms the exported wallet from an encrypted/locked source is **not** encrypted (`wallet_exported_watchonly.py:244-245`: `"unlocked_until" not in online_wallet.getwalletinfo()`), since a wallet without private keys has nothing to encrypt.

In summary: preserve flags → the watch-only wallet shows the same balances, honors the same avoid-reuse policy, and stays forward-compatible with the source wallet's configuration.

---

# Q5 — Why copy the best-block locator verbatim instead of starting from block 0

## The code

```cpp
// Write the best block locator to avoid rescanning on reload
CBlockLocator best_block_locator;
{
    WalletBatch local_wallet_batch(wallet.GetDatabase());
    if (!local_wallet_batch.ReadBestBlock(best_block_locator)) {
        return util::Error{_("Error: Unable to read wallet's best block locator record")};
    }
}
if (!watchonly_batch.WriteBestBlock(best_block_locator)) {
    return util::Error{_("Error: Unable to write watchonly wallet best block locator record")};
}
```
(`src/wallet/export.cpp:171-182`)

The comment states the reason directly: **"to avoid rescanning on reload."**

## What the best-block locator is

`CBlockLocator` is the same structure used in P2P header sync (`getheaders`) — a chain of block hashes back from the tip, sparse as they get older. In the wallet it is persisted via `WalletBatch::WriteBestBlock`/`ReadBestBlock` (`src/wallet/walletdb.h:238-239`) and records **how far the wallet has already scanned the chain**. `CWallet::blockConnected` updates and periodically flushes it (`src/wallet/wallet.cpp:1586-1588`), and `SetBestBlock` sets `m_best_block_time` (`wallet.cpp:1640`).

## Why starting from block 0 would be bad

When a wallet loads, it compares its stored locator against the node's current chain to decide **from where to resume scanning** (`CWallet::ScanForWalletTransactions`, `src/wallet/wallet.cpp:1892+`). If the wallet's locator is empty/genesis, it would conclude it has scanned nothing and **rescan from block 0** (or from the wallet birth time) — walking every block and running every transaction through `AddToWalletIfInvolvingMe` (`wallet.cpp:1227`) to (re)discover which txs are relevant.

That rescan would be:
- **Slow** — proportional to chain height (hundreds of thousands of blocks).
- **Redundant** — the exporter **already copied all of the wallet's transactions** verbatim into the new wallet (`src/wallet/export.cpp:184-194`, `LoadToWallet` + `WriteTx`). So the watch-only wallet starts with a complete transaction history; it doesn't need to rediscover them.
- **Potentially incomplete on a pruned node** — a deep rescan can fail (`ScanResult::FAILURE`) if the needed historical blocks have been pruned (`wallet.cpp:1884-1885`), even though all the data is already in the DB.

By copying the source wallet's locator verbatim, the new wallet's "last scanned block" matches the source's, so on load it sees it's already caught up to that point and only scans forward from there (the few blocks between export and load). This is what makes `exportwatchonlywallet` + `restorewallet` a true single-step, **rescan-free** watch-only setup — the central UX promise of the PR, and the reason the offline-signing tutorial was updated to point users at this RPC.

(The locator is not security-sensitive: it's just a chain-position bookmark. There's no privacy or correctness risk in copying it, and it doesn't need to match the *node's* tip, only to be a real point the wallet has processed.)

---

# Q6 — Security/privacy: multisig `wsh(multi(2,xpub1,xpub2))` and what a third party learns

## Setup

One cosigner runs `exportwatchonlywallet` on their wallet (which contains `wsh(multi(2,xpub1/*,xpub2/*))` as an active descriptor) and hands the resulting `.dat` to a third party. Compare that to simply handing the third party the two descriptor strings.

## First: would the cache even be copied here?

Both key expressions are unhardened ranged xpubs (`xpub.../*`). For a `BIP32PubkeyProvider` with no hardened step, `IsHardened()` is false (`src/script/descriptor.cpp:416-423`), so `CanSelfExpand()` is true (`descriptor.cpp:599`), and `DescriptorImpl::CanSelfExpand()` returns true (no nested hardened provider, `descriptor.cpp:1101-1110`). Therefore **no cache is copied** for this descriptor (`export.cpp:129` is skipped). So the privacy delta is *not* about extra derivable key material — it's about **wallet state/metadata**.

## New information a third party learns from the exported wallet

Beyond the bare descriptor strings, the watch-only wallet file exposes:

1. **Full transaction history** — every wallet transaction is copied (`src/wallet/export.cpp:184-194`). The third party learns *which* derived addresses have actually been used, the amounts, timing, and counterparties. From the descriptor strings alone they could derive *candidate* addresses but would **not** know which have received coins or the complete set of past activity.
2. **Address book entries** — labels, purposes ("send"/"receive"), and receive-request metadata (`export.cpp:197-204`). These can leak socially meaningful information: human-readable labels tied to addresses/counterparties.
3. **`previously_spent` / reuse markers** (`export.cpp:205`) — reveals which addresses have been spent from, i.e. the wallet's address-reuse history.
4. **Persisted locked coins** (`export.cpp:135-138`) — reveals which specific UTXOs the cosigner has chosen to freeze, hinting at spending strategy/intent.
5. **Wallet metadata** — `orderPosNext`, descriptor `creation_time`, `next_index`/keypool position (`export.cpp:157`, and `WalletDescInfo` in `export.h:18-27`), and the best-block locator (`export.cpp:171-182`) which leaks roughly *when* and *how active* the wallet has been.

## What they do *not* learn (compared to the descriptor strings)

Nothing new about the *keys* themselves: the descriptors are exported with `export_private=false` (`export.cpp:62`), so no private keys are included (guarded by `assert(dummy_keys.keys.size() == 0)`, `export.cpp:127`). For an **unhardened** multisig, the public derivation power is identical to having the descriptor strings.

## The hardened-path caveat

The host noted in IRC: for a descriptor that *does* use hardened paths, the cache copy would additionally reveal **pre-derived pubkeys** that the descriptor strings alone could not produce — `DescriptorCache` holds `CExtPubKey`s past hardened steps (`src/script/descriptor.h:22-26`). For this specific `wsh(multi(2,xpub1,xpub2))` that doesn't apply, but in general the watch-only file can carry more derivable address material than the bare descriptor.

## Conclusion

Handing over a watch-only wallet is materially more sensitive than handing over descriptor strings: it's a **full activity log + labeled address book + UTXO/lock state**, not just the spending policy. The RPC help text (`src/wallet/rpc/wallet.cpp:923-928`) describes what's included, so users should treat the exported file as sensitive transaction-history data even though it contains no private keys.

---

# Q7 — Why top up the keypool before checking spendability across the online/offline pair

## Note on the question's wording

The literal `wallet.keypoolrefill(100)` call is **not** present in the committed test (`test/functional/wallet_exported_watchonly.py`); `grep` for `keypoolrefill` returns no matches in that file. The actual mechanism is that the **`exportwatchonlywallet` RPC itself calls `pwallet->TopUpKeyPool()`** before doing the export:

```cpp
LOCK(pwallet->cs_wallet);
pwallet->TopUpKeyPool();                                                       // src/wallet/rpc/wallet.cpp:951
util::Result<std::string> exported = ExportWatchOnlyWallet(*pwallet, fs::PathFromString(dest), context);
```

The question is really asking *why this top-up step matters* for the online/offline spendability test. The test's keypool handling is via the `-keypool={KEYPOOL_SIZE}` (10) node arg (`wallet_exported_watchonly.py:24`) and the exhaustion check at lines 190-192.

## Why the top-up is essential

### For hardened descriptors: it fills the cache that becomes the watch-only wallet's only key material

As established in Q2/Q3, a descriptor with hardened derivation has `CanSelfExpand() == false`, so the watch-only wallet cannot generate new addresses itself — it relies entirely on the **cache** copied out of the source wallet (`src/wallet/export.cpp:129-132`). That cache is populated by `TopUpKeyPool()` → `TopUp` → `CacheDerivedExtPubKey`/`CacheParentExtPubKey` (`src/script/descriptor.cpp:476-487`) as the source wallet pre-derives keys into its keypool. If the pool weren't topped up before export, the cache would contain only the keys derived so far, and the watch-only wallet would start with few or no usable addresses.

The test makes this concrete: for the fully-hardened `sh(wpkh(tprv…/0'/*'))` descriptor, after exporting, the online watch-only wallet can produce exactly `KEYPOOL_SIZE - 1` more addresses and then errors out:
```python
for _ in range(KEYPOOL_SIZE - 1):
    online_wallet.getnewaddress(address_type="p2sh-segwit")
assert_raises_rpc_error(-12, "No addresses available", online_wallet.getnewaddress, address_type="p2sh-segwit")  # line 192
```
That count is a direct consequence of how many keys were cached at export time.

### For the online/offline spendability check specifically

In `test_basic_export` the online (watch-only) wallet generates a receive address and builds an unsigned PSBT, then the **offline** wallet must sign it (`wallet_exported_watchonly.py:64-74`):

```python
send_res = online_wallet.send([{funds_addr: 5}])
assert_equal(send_res["complete"], False)
signed_psbt = offline_wallet.walletprocesspsbt(send_res["psbt"])["psbt"]
```

For `walletprocesspsbt` to sign, the offline wallet must **recognize the inputs as its own** and possess the corresponding private keys. Both wallets derive addresses from the *same* descriptor using the *same* address indices, so the receive address the online wallet handed out must correspond to a key the offline wallet has already derived (and for which it holds the privkey). Topping up the keypool guarantees the offline wallet has pre-computed/retained that key (and the matching change address) rather than having it still pending derivation — keeping the two wallets in sync. The subsequent loop (`wallet_exported_watchonly.py:84-89`) further verifies they keep agreeing on addresses generated past the keypool end, confirming descriptor/index alignment.

## Summary

Topping up the keypool before export (a) maximizes the hardened-key cache shipped to the watch-only wallet, and (b) ensures the offline signer has already derived the keys behind the addresses the online wallet will use, so the PSBT round-trip (online builds → offline signs → online broadcasts) succeeds. Without it, the watch-only side could run out of addresses (`-12 No addresses available`) and the signing wallet might not recognize the inputs.

---

# Q8 — Alternatives: `dumpwallet`→`importwallet` / `importdescriptors` vs `exportwatchonlywallet`

## Status of the legacy tools today

- **`dumpwallet` / `importwallet` RPCs no longer exist** in this codebase. `grep "dumpwallet\|importwallet" src/wallet/rpc/backup.cpp` returns nothing — they were removed in the transition to descriptor wallets. The only surviving analogue is the offline **`bitcoin-wallet` tool's `dump` / `createfromdump`** commands (`src/wallet/wallettool.cpp:96,136,160`), which still operate on a raw wallet file and explicitly warn: *"The dumpfile may contain private keys. To ensure the safety of your Bitcoin, do not share the dumpfile."* (`wallettool.cpp:158`). So `dumpwallet`→`importwallet` is **not** a viable modern path — and even historically it exported private keys (a non-starter for handing to an online watch-only node).
- **`importdescriptors`** still exists and is the closest functional alternative.

## Comparison

| Dimension | `dumpwallet`→`importwallet` | `importdescriptors` | **`exportwatchonlywallet`** |
|---|---|---|---|
| **Private keys** | Exports them (security risk) | None (public descriptors) | None — `export_private=false`, asserted (`export.cpp:62,127`) |
| **Exists on modern descriptor wallets?** | **No** (RPC removed) | Yes | Yes (this PR) |
| **UX / steps** | Multi-step, manual file shuffle | Manual: must obtain/export descriptor strings, decide active/internal/range, paste JSON | **Single RPC call** producing a self-contained file loadable via `restorewallet` |
| **Transaction history** | Copied (legacy dump) | **Lost** — must rescan to rebuild | **Copied verbatim** (`export.cpp:184-194`) |
| **Rescan required?** | Partial | **Yes** — fresh wallet starts at block 0 / birth time | **No** — best-block locator copied (`export.cpp:171-182`) |
| **Address book (labels, purpose, receive requests)** | Partially | **Lost** | **Copied** (`export.cpp:197-204`) |
| **Hardened descriptors** | N/A (legacy) | **Broken** — no private key and no cache ⇒ `CanSelfExpand()==false`, can't derive addresses (Q2) | **Works** — cache copied for `!CanSelfExpand()` descriptors (`export.cpp:129-132`) |
| **Locked coins (persistent)** | No | No | **Copied** (`export.cpp:135-138`) |
| **Wallet flags (avoid-reuse, …)** | No | No | **Preserved** (`export.cpp:79`, Q4) |
| **Output format** | Text dump / raw file | In-memory import | A real wallet DB file via `BackupWallet` (`export.cpp:209`) |
| **Long-term maintainability** | Deprecated/removed | Manual & lossy; every new wallet metadata field must be re-imported by hand | Robust: reuses `CWallet` internals, copies structured records, future metadata travels automatically |

## Why the new RPC is superior

- **Completeness**: it is the only option that preserves *the entire wallet state needed to be a faithful watch-only mirror* — descriptors **and** their hardened-derivation cache, the full transaction set, the address book, persistent locks, flags, and the scan position. `importdescriptors` reconstructs only the descriptors; everything else is dropped or must be rescanned.
- **No rescan**: copying the locator + all txs means the restored wallet is immediately useful (critical for air-gapped signing workflows and for pruned nodes where a deep rescan may be impossible, Q5).
- **Handles hardened paths**: the cache copy is the decisive feature `importdescriptors` cannot replicate — without it, hardened descriptors are unusable watch-only (the test's `-12 No addresses available` at `wallet_exported_watchonly.py:192`).
- **Maintainability**: the design extracts reusable `ExportDescriptors()` (`export.cpp:18`, shared with `listdescriptors`, `backup.cpp`) and copies structured DB records, so future wallet fields are carried automatically instead of requiring parallel updates to a text-import format.

The main thing `exportwatchonlywallet` gives up vs. `importdescriptors` is **flexibility/granularity** — you export the *whole* wallet rather than cherry-picking individual descriptors, and (per Q6) you also leak the wallet's activity history, so it's a "hand over a wallet" operation rather than a "hand over a key template" one. For the intended air-gapped-signing use case, that's exactly the desired trade-off.

---

# Recommendations for Future Agents

## What Worked Well

- **Fetch the PR yourself when the checkout is wrong.** The workspace repo was on a stale 2019 master with no `pr32489` tag and no `CMakeLists.txt`. Detecting this early and running `git fetch https://github.com/bitcoin/bitcoin.git refs/pull/<N>/head:pr<N>` recovered the situation in one step. Network was available — always test it with `git ls-remote` / `curl -sI`.
- **The lineage `build_system.md` applied across workshops.** Even though the lineage was all for workshop #33300 (compact blocks), the cmake lessons (`-DENABLE_IPC=OFF`, `make` not `ninja`, background builds with `setsid`, Debug build for tests) transferred directly. Read the cross-workshop knowledge even when it's from a different PR.
- **Start the build immediately, read code while it compiles.** The Debug `bitcoind` finished in ~4 min; I was reading `export.cpp`/`descriptor.cpp` in parallel.
- **Run the actual functional test.** For a wallet-RPC PR, the functional test is far more valuable than fuzzing. It passed, giving high confidence and concrete line-number references.
- **Design your own experiment for the fun exercise.** A feature PR has no crash to reproduce; empirically attacking the security claims (no private keys, keypool exhaustion) was the right substitute and produced strong evidence.
- **Trace every `CanSelfExpand` override with grep, then Read each site.** Gave a complete, line-referenced answer for Q2/Q3.

## What Didn't Work

- **Assumed the repo was on the PR branch.** The brief said it would be; it wasn't. Lost a few minutes before checking `git log` / `ls CMakeLists.txt`.
- **First functional-test runs failed on test-harness plumbing** (`test/config.ini` missing; relative `BITCOIND` path). The test framework changes CWD, so `BITCOIND` and `--configfile` must be **absolute**. This should be in `build_system.md`.
- **Wrong `listdescriptors` result key** (`descriptor` vs `desc`) in my experiment script. When in doubt, grep the RPC result definition in the C++ source first.

## Pitfalls for Future Agents

- **The merge-base diff is NOT the PR diff.** `git diff master..pr32489` shows 3053 files (all upstream history). Use `git diff <first-pr-commit>^..pr32489` or `git diff cf4653dfd1..pr32489` to see only the PR's changes (10 files, ~817 lines).
- **`dumpwallet`/`importwallet` RPCs are gone** in modern descriptor wallets — don't cite them as currently-available (Q8). Only `bitcoin-wallet` tool `dump`/`createfromdump` survive, and they include private keys.
- **Q7's premise (`keypoolrefill(100)` in the test) is inaccurate** — the test never calls it; the `TopUpKeyPool()` happens inside the RPC (`src/wallet/rpc/wallet.cpp:951`). State this honestly rather than inventing the call.
- **Watch-only wallets reject `listdescriptors(private=true)`** with RPC -4; useful as a security assertion but will crash a naive test.
- **For wallet PRs, build `bitcoind` (not `fuzz`).** `-DCMAKE_BUILD_TYPE=Debug -DENABLE_IPC=OFF`; target `make -j$(nproc) bitcoind` (~4 min). Run tests with absolute `BITCOIND` + `--configfile build/test/config.ini`.

## Prompt Improvements

- The prompt says "The Bitcoin Core repository is at `/workspace/bitcoin`, checked out at the PR branch." This was **false**. Add a step: "Verify the checkout — if `git log` doesn't show the PR commits or `CMakeLists.txt` is missing, fetch the PR from `bitcoin/bitcoin` via `refs/pull/<N>/head`."
- The fun-exercise section assumes a "revert a fix PR / reproduce a crash" workflow that only fits bug-fix PRs. For feature PRs, suggest an alternative (security/behavior experiment, edge-case probing). Provide a per-workshop fun-exercise hint in `workshop.json`.
- Clarify that the lineage is per-workshop; cross-workshop lineage is only useful for build/general knowledge. Note explicitly when no prior runs exist for the current workshop.

## Build/Environment Improvements

- **Pre-populate `/workspace/bitcoin` on the actual PR branch** (or document the fetch step). Stale 2019 master is a confusing starting point.
- **Add `config.ini`/absolute-path test-runner guidance to `build_system.md`**: `BITCOIND=/abs/path build/bin/bitcoind ... --configfile /abs/path/build/test/config.ini`.
- **Ship the workshop's own cheatsheet** at `/opt/bcr-agent/workshops/<workshop_id>/cheatsheet.md`. Only #33300's existed; #32489 had none.

---

# Journal — Workshop #32489 (exportwatchonlywallet RPC)

## Environment setup

### Approach
- Read `workshop.json`, knowledge base (`build_system.md`, `common_pitfalls.md`, `review_strategies.md`), lineage (`learnings.jsonl`).
- Discovered the repo was on OLD master (2019, autotools, no CMakeLists.txt) with NO `pr32489` tag.
- Lineage files (`/workspace/lineage/*.bin`) are from workshop #33300 (a DIFFERENT PR — compact block fuzz harness). No prior runs for THIS workshop existed. The build-system knowledge still applied.

### Result
- **Network was available.** Fetched the PR directly: `git fetch https://github.com/bitcoin/bitcoin.git refs/pull/32489/head:pr32489`. This pulled the full modern (2025) Bitcoin Core + the 5 PR commits.
- PR base = `cf4653dfd1` ("wallet: Write new descriptor's cache in AddWalletDescriptor"). The `pr32489` branch = modern Bitcoin Core + 5 commits on top.
- Checked out `pr32489`; `CMakeLists.txt` now present (modern cmake build).

### Challenges
- The repo was NOT pre-checked-out at the PR branch (contrary to the workshop brief). Had to detect this and fetch it myself.
- The PR-only diff is the 5 commits (`cf4653dfd1..pr32489`), not the merge-base diff (which shows 3053 files = all upstream history 2019→2025).

## Build

### Approach
- Configured debug build: `cmake -B build -S . -DCMAKE_BUILD_TYPE=Debug -DENABLE_IPC=OFF` (applied lineage lesson: `-DENABLE_IPC=OFF` for missing Cap'n Proto).
- Built `bitcoind` in background with `setsid make -j$(nproc) bitcoind`.

### Result
- **Build succeeded in ~4 minutes.** Binary: `build/bin/bitcoind` (188 MB).
- CMake configure was clean (no errors).

## Functional test

### Approach
- Ran `test/functional/wallet_exported_watchonly.py`.
- First two attempts failed: missing `test/config.ini` (used `--configfile build/test/config.ini`), then relative `BITCOIND` path (used absolute path).

### Result
- **Test PASSED** on third attempt with `BITCOIND=/workspace/bitcoin/build/bin/bitcoind --configfile /workspace/bitcoin/build/test/config.ini`.
- All 6 sub-tests ran: basic export, address book, txs+locked coins, imported descriptors, avoid-reuse, encrypted wallet.

## Key code locations discovered
| Topic | File:Line |
|---|---|
| ExportWatchOnlyWallet impl | `src/wallet/export.cpp:41-217` |
| ExportDescriptors (refactored from listdescriptors) | `src/wallet/export.cpp:18-39` |
| RPC wrapper (calls TopUpKeyPool first) | `src/wallet/rpc/wallet.cpp:920-961` |
| CanSelfExpand — PubkeyProvider base | `src/script/descriptor.cpp:236` |
| CanSelfExpand — OriginPubkeyProvider | `src/script/descriptor.cpp:307` |
| CanSelfExpand — ConstPubkeyProvider (pkh(pubkey)) | `src/script/descriptor.cpp:372` |
| CanSelfExpand — BIP32PubkeyProvider | `src/script/descriptor.cpp:599` |
| CanSelfExpand — DescriptorImpl (recursive) | `src/script/descriptor.cpp:1101-1110` |
| IsHardened() | `src/script/descriptor.cpp:416-423` |
| Cache write in GetPubKey | `src/script/descriptor.cpp:476-487` |
| DescriptorCache fields | `src/script/descriptor.h:19-71` |
| Avoid-reuse filtering in AvailableCoins | `src/wallet/spend.cpp:333,444` |
| IsSpentKey → previously_spent | `src/wallet/wallet.cpp:1045-1056` |
| Wallet flags table | `src/wallet/wallet.h:150-180` |
| Best-block locator read/write | `src/wallet/export.cpp:170-182` |

---

## Appendix: fun_exercise.md

# Fun Exercise — Empirical Security & Behavior Verification of `exportwatchonlywallet`

## Context

PR #32489 is a **feature** (new RPC), not a bug fix, so there is no "fix PR to revert and crash to reproduce." Instead I designed an experiment to attack the PR's two most important claims with real evidence:

1. **Security**: the exported watch-only wallet must contain **zero** private keys.
2. **Behavior**: a hardened-derivation watch-only wallet is **bounded** by the cached keypool, while the private-key-holding source is not.

## Setup

Custom functional test `fun_experiment.py` run against the Debug `build/bin/bitcoind` in regtest (clean chain, `-keypool=5`). It imports an **all-hardened** descriptor `wpkh(tprv…/84h/0h/0h/0/*)` (every path element hardened) into a descriptor wallet, exports it, restores it as `wo`, and compares.

Reproduce:
```bash
env BITCOIND=/workspace/bitcoin/build/bin/bitcoind \
  python3 /workspace/results/fun_experiment.py \
  --configfile /workspace/bitcoin/build/test/config.ini
```

## Results (all assertions passed)

### 1. No private keys leak — PASS (stronger than expected)
- **Source** wallet exposes `tprv…` via `listdescriptors(private=true)` → `Source has private keys in descriptors: True`.
- **Watch-only** wallet does not merely return public descriptors; the RPC **outright refuses** the private call:
  ```
  code -4: "Can't get private descriptor string for watch-only wallets"
  ```
- `getwalletinfo` on the watch-only shows `private_keys_enabled = False`, `descriptors = True`.
- Public descriptors are **byte-identical** between source and watch-only.

This validates the PR's `export_private=false` path (`src/wallet/export.cpp:62`) and the `assert(dummy_keys.keys.size() == 0)` guard (`export.cpp:127`), and confirms `WALLET_FLAG_DISABLE_PRIVATE_KEYS` (`export.cpp:79`) is actually in effect.

### 2. Hardened watch-only is address-bounded — PASS
- Watch-only produced **exactly 5** bech32 addresses (matching `-keypool=5`) then would exhaust.
- Source (has the `tprv`) produced **20** bech32 addresses with no limit.

This empirically confirms the `CanSelfExpand()==false` / cache-exhaustion reasoning from Q2/Q3: for a fully-hardened descriptor the watch-only side can only ever use the pre-derived keys that were shipped in the `DescriptorCache`, while the private-key holder can derive indefinitely.

## What I tried that didn't work / adjustments
- First run failed: I assumed the `listdescriptors` result key was `descriptor`; it's `desc`. Fixed.
- Second run "failed" but actually surfaced the best possible evidence: calling `listdescriptors(private=true)` on a watch-only wallet raises an RPC error rather than returning public-only descriptors. I converted that into an explicit positive security assertion (catch the error = PASS).

## Conclusion
The PR's core security guarantee (no private keys in export) and its key behavioral trade-off (hardened watch-only wallets are keypool-bounded) both hold under direct experimentation. No crash to reproduce here, but the experiment gives concrete, run-it-yourself proof of the properties discussed in Q2–Q7.