💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3116367414)

@ryanofsky
I missed the essence of this commit, and thanks to @Sjors for clarifying that it’s intended for test coverage. If I understand correctly, test coverage is usually done on the test network.
I have no strong objection to the commit itself, just that initially I thought it demonstrate mining and I saw some edge-cases that are not covered.

It would be better to limit the mining to `regtest` only (since its for test), add a bound to the number of tries, with the recent comments, I
...

💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-3116401175)
> I believe the following is done now and can be removed from the PR description?

Yes, removed.

💬 ajtowns commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-3116411732)
Still working on this? Rebase is just https://github.com/ajtowns/bitcoin/commit/d6055bdc77552f395be1a4f90b8b98c0bec5a0af

💬 b-l-u-e commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3116435254)
Thanks for the thorough reviews! I've investigated the issues you raised and found the root cause.

### 🔍 **Root Cause Analysis**
The test was failing because the **instructions were wrong**. The original instructions told users to use loopback interfaces (`lo:0`, `lo:1`), but the code filters out loopback addresses (line 340 in `netif.cpp`).

This is exactly what issue #31336 reported - users following the instructions got empty local addresses.

### 📝 **Reviewer Feedback Addressed**

...

💬 l0rinc commented on pull request "refactor: inline constant return values from `dbwrapper` write methods":
(https://github.com/bitcoin/bitcoin/pull/33042#issuecomment-3116482500)
> How verbose would it be to pass it up and check it everywhere?

With `CDBWrapper::Read` all callers are trying to handle the failure (usually by returning `false` or `std::nullopt` or `std::vector<uint256>()` or resetting the state or manually logging a failure).
I have tried unifying `Write` and `Read` via:
```diff
diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h
--- a/src/node/blockstorage.h (revision 09f004bd9fecbee2216ffb6b7bb787d9ec4f0362)
+++ b/src/node/blockstorage.
...

👍 rkrux approved a pull request: "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase"
(https://github.com/bitcoin/bitcoin/pull/33032#pullrequestreview-3054176313)
tACK 1cdb9161774ccf3edcdcb1e482b20be430ed5430

Thanks for incorporating suggestions.

💬 rkrux commented on pull request "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase":
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2230208195)
Nit to use tighter function signature that goes along with the function name too. Builds & tests fine on my system.

<details>
<summary>Use MockableSQLiteDatabase in return type that passes for WalletDatabase</summary>

```diff
diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp
index 6017582395..58fafeecf7 100644
--- a/src/wallet/test/util.cpp
+++ b/src/wallet/test/util.cpp
@@ -80,12 +80,12 @@ void TestUnloadWallet(std::shared_ptr<CWallet>&& wallet)
WaitForDeleteWal
...

💬 maflcko commented on pull request "refactor: inline constant return values from `dbwrapper` write methods":
(https://github.com/bitcoin/bitcoin/pull/33042#issuecomment-3116540946)
> and handling all of the calls, but I'm getting failures all over the place, mostly because `Read` cannot tell the difference between missing values and errors. I'll investigate further.

Yeah, I'd presume you'd have to use `Result<bool>`, which I'd suspect would be highly verbose.

> I didn't see a single case where we're trying to handle any exception - seems we assumed the return value reflects the status.

I tried that yesterday, and an error in the index code (`scheduler` thread, or
...

💬 maflcko commented on pull request "fuzz: Make process_message(s) more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3116570133)
fuzz-only pull with two acks and no conflicts with reviewed pulls, rfm?

💬 pablomartin4btc commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2230275124)
out of curiosity... why do we need this here now? what's the issue if we don't (currently in `master`)? could we not use `UpgradeDescriptorCache()` from an upper level?

💬 Sjors commented on pull request "[POC] wallet: Enable non-electronic (paper-based) wallet backup with codex32":
(https://github.com/bitcoin/bitcoin/pull/33043#issuecomment-3116588223)
It might be useful to open an issue to discuss paper backups in general.

I think conceptually the are three different things to backup, and each has different requirements and frequencies:

1. Private key material: backed up once at wallet creation time (or never, in some multisig setups where recovery consists of using the other keys and moving to a fresh wallet). Needs to be kept secure against theft.
2. Output descriptors: for anything more complicated than a [BIP48 multisig](https://
...

💬 pablomartin4btc commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2230279837)
nit: shouldn't be something like GetDescriptorsInfo... ? or better naming... it's not doing an "export"...

🤔 pablomartin4btc reviewed a pull request: "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet"
(https://github.com/bitcoin/bitcoin/pull/32489#pullrequestreview-3054273346)
light cr ACK e6ac2015a852caa0f50e44becf9cd4b7592c48e7

I like the refactoring in moving the logic of extracting descriptors info out of `listdescriptors` RPC to `CWallet` itself.

I'll continue reviewing and will test it soon.

Left a couple of comments.

💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2230294157)
Let's not (re)build a full fletched CPU miner :-)

💬 pablomartin4btc commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2230304020)
nit: this could be just GetDescriptors? you can list them or export them later?

💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3116624817)
re-utACK 3d099384a60c8f64c8d67a9f1e7b8649a9c54313

💬 Sjors commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2230329744)
I'm regretting that assumption in the context of multisig :-)

💬 Sjors commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-3116675604)
re-ACK 604955304dfca14b1c0bb5ed1044141a5d1b2d79

💬 Sjors commented on pull request "Have createwalletdescriptor auto-detect an unused(KEY)":
(https://github.com/bitcoin/bitcoin/pull/32861#issuecomment-3116687436)
> The linked comment also states that any RPC using the unused descriptor should delete it immediately afterwords.

That should be done in #29136 which introduces `createwalletdescriptor `, or in a followup, but not in this PR.

👍 rkrux approved a pull request: "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively"
(https://github.com/bitcoin/bitcoin/pull/28333#pullrequestreview-3054401144)
ACK 8e9de762f66f3e99e02944006999fe5afb1896e4

Thanks for addressing comments.