💬 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?

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1411776086)
done

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1411780306)
Yes. My primary concern is the complexity though. Cascade removal is more difficult to reason about. I just thought it was not worth it.

💬 Sjors commented on pull request "net: additional disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-1835692974)
I added `peeraddr=` to each of them. This is indeed quite useful, because we can't look up the IP after disconnection. Adding `net=` also makes sense, but let's do that everywhere in another PR (probably using a helper function).

💬 maflcko commented on pull request "net: additional disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-1835740082)
> because we can't look up the IP after disconnection.

Is the IP not logged on connection, when IP logging is enabled?

💬 TheCharlatan commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1835740522)
Concept ACK

💬 naumenkogs commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1411821098)
122658f95b36b7f940351bd34a89e13831931f18

should we make this addition overflow safe?