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

🤔 naumenkogs reviewed a pull request: "Nuke adjusted time (attempt 2)"
(https://github.com/bitcoin/bitcoin/pull/28956#pullrequestreview-1759382310)
Concept ACK

💬 maflcko commented on pull request "build: Patch Qt to handle minimum macOS version properly":
(https://github.com/bitcoin/bitcoin/pull/28851#issuecomment-1835807820)
lgtm ACK 05aca093819be276ac7d648472c6ed5c7d235cc5

💬 Sjors commented on pull request "net: additional disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-1835817607)
@maflcko indeed it is, though for inbound it depends on the exact log settings. I think right now you won't see these disconnect messages without `-debug=net` and with it you'll see all the connect messages. But if we increase the relative priority of disconnect messages then you might not. So it still seems useful to add.

💬 dergoegge commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1835850051)
```
$ echo "jtHRO///YED/AAD/AAABCQAAgQAN/wAAAQAALAEB/QAAAAAJAAAAAAAAAAAAAAAA+GwAAPAV/v8BkAEicG9vcyYANwAAAAEBADsBAQHo/v7+bv4RAQEBkQABAQE3CTsBIQEBAQEBAAEBATcAACYKAAQBAf8BAQEBAQEBCAABAQEBfgEAAQEBCgAAADsBAQEBAQEB/v8BJgABQQE3AAAAAAFBATcAAAAA8AAAAQEBAQEBAQEBCAABLgEBAX4BAAE=" | base64 --decode > tx_package_eval-81b8ec0e06b86811790d61f69fe27701ca0c8305.crash
$ FUZZ=tx_package_eval ./src/test/fuzz/fuzz tx_package_eval-81b8ec0e06b86811790d61f69fe27701ca0c8305.crash
test/fuzz/package_eval.cpp:320 tx_pac
...

⚠️ DMUNEY45 opened an issue: "> In file included from ../dist/./../cxx/cxx_db.cpp:13:"
(https://github.com/bitcoin/bitcoin/issues/28982)
> In file included from ../dist/./../cxx/cxx_db.cpp:13:
> ./db_cxx.h:59:10: fatal error: 'iostream.h' file not found

The issue is some code like this in bdb:
```cpp
#ifdef HAVE_CXX_STDHEADERS
#include <iostream>
#include <exception>
#define __DB_STD(x) std::x
#else
#include <iostream.h>
#include <exception.h>
#define __DB_STD(x) x
#endif
```

For some reason, configure fails to detect C++ header availability, and `HAVE_CXX_STDHEADERS` is not defined. Is `g++` and a
...

✅ fanquake closed an issue: "> In file included from ../dist/./../cxx/cxx_db.cpp:13:"
(https://github.com/bitcoin/bitcoin/issues/28982)

:lock: fanquake locked an issue: "> In file included from ../dist/./../cxx/cxx_db.cpp:13:"
(https://github.com/bitcoin/bitcoin/issues/28982)

💬 fanquake commented on pull request "build: disable external-signer for Windows":
(https://github.com/bitcoin/bitcoin/pull/28967#issuecomment-1835873829)
> and drop boost-process from vcpkg.json.

Added.

👍 hebasto approved a pull request: "build: disable external-signer for Windows"
(https://github.com/bitcoin/bitcoin/pull/28967#pullrequestreview-1759532758)
re-ACK 308aec3e5655327d98e0428d8205d246f24d6af5.