💬 jonatack commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#issuecomment-1834712813)
The code changes cause this test failure for me locally:

```
m'.
jon|(fb075bab8c9...):~/bitcoin/bitcoin$ ./test/functional/wallet_fundrawtransaction.py --legacy-wallet
2023-11-30T23:17:02.968000Z TestFramework (INFO): PRNG seed is: 4425912909580482609
.../...
2023-11-30T23:17:20.070000Z TestFramework (INFO): Test fundrawtxn with locked wallet and hardened derivation
2023-11-30T23:17:21.869000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent cal
...

👍 stickies-v approved a pull request: "rpc: keep `.cookie` file if it was not generated"
(https://github.com/bitcoin/bitcoin/pull/28784#pullrequestreview-1758684529)
ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d

💬 stickies-v commented on pull request "rpc: keep `.cookie` file if it was not generated":
(https://github.com/bitcoin/bitcoin/pull/28784#discussion_r1411417065)
nit: can be done with a bit less nesting

<details>
<summary>git diff on 7cb9367157</summary>

```diff
diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp
index b7acd62ee3..ad9f244f12 100644
--- a/src/rpc/request.cpp
+++ b/src/rpc/request.cpp
@@ -133,11 +133,10 @@ bool GetAuthCookie(std::string *cookie_out)

void DeleteAuthCookie()
{
+ // Don't delete the cookie file if it wasn't generated by this process
+ if (!g_generated_cookie) return;
try {
- if (g_g
...

🤔 furszy reviewed a pull request: "wallet: Cleanup accidental encryption keys in watchonly wallets"
(https://github.com/bitcoin/bitcoin/pull/28724#pullrequestreview-1758695779)
As the process of reading wallet records from db disregards unknown record types, what if the encryption key is ever used for something else?. Wouldn't that mean that opening such wallet on any previous version (post this PR) would erase the encryption key forever?

The erase process can only verify the private keys disabled flag and whether there is any crypted key or not. But it cannot verify if the encryption key was used for something else.

Guessing that in the case of adding a new type
...

🤔 furszy reviewed a pull request: "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs"
(https://github.com/bitcoin/bitcoin/pull/28868#pullrequestreview-1758715245)
ha, I made the same change two hours ago. https://github.com/furszy/bitcoin-core/commit/193e0c9c7ffe34ab2e8a4f5b95f66ecc4bac2791. Will review.

💬 mzumsande commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1411429030)
This is not continuous, it only schedules it once more after the initial execution. I think that you meant to use `scheduleEvery`.

📝 furszy opened a pull request: "rpc: encryptwallet help, mention HD seed rotation and backup requirement"
(https://github.com/bitcoin/bitcoin/pull/28980)
Small and simple PR, updating the `encryptwallet` help message.

Better to notify users about the HD seed rotation and the new
backup requirement before executing the encryption process.
Ensuring they are prepared to update previous backups and
securely safeguard the updated wallet file.

💬 MarnixCroes commented on pull request "rpc: encryptwallet help, mention HD seed rotation and backup requirement":
(https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1411485235)
```suggestion
"Therefore, it is crucial to replace any existing backups of your wallet file with the newly generated wallet file, containing the new encrypted keys.\n"
```

I read it as the wallet file itself being encrypted.
I'd tweak the wording a bit to prevent confusion.

💬 furszy commented on pull request "rpc: encryptwallet help, mention HD seed rotation and backup requirement":
(https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1411492361)
Sure. Done as suggested.

👍 MarnixCroes approved a pull request: "rpc: encryptwallet help, mention HD seed rotation and backup requirement"
(https://github.com/bitcoin/bitcoin/pull/28980#pullrequestreview-1758853700)
ack

💬 MarnixCroes commented on pull request "rpc: encryptwallet help, mention HD seed rotation and backup requirement":
(https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1411499748)
```suggestion
"For security reasons, the encryption process will generate a new HD seed, leading to the creation of a fresh set of active descriptors.\n"
```
nit, new line forgotten?
(sry didn't notice at previous review)

💬 furszy commented on pull request "rpc: encryptwallet help, mention HD seed rotation and backup requirement":
(https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1411513106)
That was on purpose. To continue the paragraph.

📝 hebasto opened a pull request: "POC: Replace Boost.Process with cpp-subprocess"
(https://github.com/bitcoin/bitcoin/pull/28981)
Closes https://github.com/bitcoin/bitcoin/issues/24907.

This PR is based on **theStack**'s [work](https://github.com/bitcoin/bitcoin/issues/24907#issuecomment-1466087049).

I've fixed Windows-related bugs and issues and commenced submitting patches to [upstream](https://github.com/arun11299/cpp-subprocess).

For more details, please refer to commit messages.

👍 hebasto approved a pull request: "build: disable external-signer for Windows"
(https://github.com/bitcoin/bitcoin/pull/28967#pullrequestreview-1758892319)
> It's come to light that Boost ASIO (a Boost Process sub dep) has in some instances, been quietly initialising our network stack on Windows (see PR #28486 and discussion in #28940).
>
> This has been shielding a bug in our own code, but the larger issue is that Boost Process/ASIO is running code before main, and doing things like setting up networking. This undermines our own assumptions about how our binary works, happens before we run any sanity checks, and before we call our own code to s
...

👍 vasild approved a pull request: "validation: log which peer sent us a header"
(https://github.com/bitcoin/bitcoin/pull/27826#pullrequestreview-1759207756)
ACK 708600f3b7df4cf166a2fcdfd3c0dc70b70812f0

💬 maflcko commented on pull request "wallet: Fix migration of blank wallets":
(https://github.com/bitcoin/bitcoin/pull/28976#discussion_r1411751970)
If this truly should never happen, and it seems like an internal bug when it happens, then I wonder if the translation is needed and if `STR_INTERNAL_BUG` can be used?

```suggestion
error = Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing"));
```

💬 maflcko commented on pull request "wallet: Fix migration of blank wallets":
(https://github.com/bitcoin/bitcoin/pull/28976#discussion_r1411752078)
(same below)

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1411756962)
Say we have 5 inbound Erlay peers and 0 inbound legacy peers.
`inbound_targets = (5+0) * 0.1 = 0.5`
`destinations = 0.5 - 0 = 0.5`

It just means that we will take 1 inbound peer for fanout with a 50% chance.

I guess it will work just fine if i drop the `0.01` (1% or less chance) check. It's kinda unfortunate we have to do the compute if chance is that small. Fine with me either way, let me know what you think.

💬 maflcko commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1411756653)
```suggestion
[#19160](https://github.com/bitcoin/bitcoin/pull/19160) added a new `bitcoin-node` `-ipcbind` option and an `bitcoind-wallet` `-ipcconnect` option to allow new wallet processes to connect to an existing node process.
```

The pull is merged?

💬 maflcko commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1411756818)
```suggestion
[#19161](https://github.com/bitcoin/bitcoin/pull/???) adds a new `bitcoin-gui` `-ipcconnect` option to allow new GUI processes to connect to an existing node process.
```

wrong number?