💬 theuni commented on pull request "RFC: build: support for pre-compiled headers.":
(https://github.com/bitcoin/bitcoin/pull/31053#discussion_r1831297585)
Yes. To be clear, I really don't like the arbitrary nature of this at all. I was hoping someone would have a better idea for an approach to selection.

I really meant this as an RFC because I agree that what's here is a pretty crappy solution. But it's clear that pch is a win, so I think we should come up with _something_.

💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1831358176)
This is a great idea!

💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1831368185)
I think `self` is no longer a good name for this parameter if we make this static. It should just be `pair`.

Same for `SetDirty` and `SetFresh`.

👍 andrewtoth approved a pull request: "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests"
(https://github.com/bitcoin/bitcoin/pull/30906#pullrequestreview-2418862806)
re-crACK aa2f3139529c054b011a0f75ff314e6d63f0d977

It might be worth updating the `AddFlags` comment now, but otherwise non-blocking nits.

💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1831367497)
nit: this comment is no longer accurate after d5e3bbf440aa948df8159999fd4eb5275c354b93

We should just remove the part about a self reference, and only mention the sentinel.

💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1831374417)
nit: `// Check that we can clear state then re-set it`

💬 glozow commented on pull request "TxDownloadManager followups":
(https://github.com/bitcoin/bitcoin/pull/31190#discussion_r1831397906)
What do you mean any value in?

💬 hebasto commented on pull request "cmake: Revamp `FindLibevent` module":
(https://github.com/bitcoin/bitcoin/pull/31181#issuecomment-2460305663)
My Guix build:
```
aarch64
b526b46943f39780cb2a204cd64159f7dd4f502e482256ba78371c82d717a591 guix-build-5a96767e3f53/output/aarch64-linux-gnu/SHA256SUMS.part
77714a2034b96c572598342da2147a638abd626b29e4ccecfe786f53dbf72a4e guix-build-5a96767e3f53/output/aarch64-linux-gnu/bitcoin-5a96767e3f53-aarch64-linux-gnu-debug.tar.gz
a32ce8d85863ec00f75b37ab7b10bfc37e85efd3a1016ed66ce6cbd0024d018a guix-build-5a96767e3f53/output/aarch64-linux-gnu/bitcoin-5a96767e3f53-aarch64-linux-gnu.tar.gz
751eba9b
...

💬 instagibbs commented on pull request "TxDownloadManager followups":
(https://github.com/bitcoin/bitcoin/pull/31190#discussion_r1831404449)
would the suggested change potentially give coverage

🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: Implement changeset interface for mempool"
(https://github.com/bitcoin/bitcoin/pull/31122#pullrequestreview-2413495210)
ACK 9512bd34a3d74d15c3ce1086a80a1edfa5ac3acf
With some comments.

💬 ismaelsadeeq commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1828023652)
Can just be vector of `TxHandle` here.

💬 ismaelsadeeq commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1831210600)
The return here is redundant.

💬 ismaelsadeeq commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1827993798)
For clarity:

```suggestion
// Finally, check that we can prioritize tx_child_1 to get package1 into the mempool.
```

We can also prioritize tx_parent_1 to get package1 in, but the nFeeDelta must be high enough to evict package3.

💬 ismaelsadeeq commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1831172493)
This seems fine to remove because it's redundant anyways.
But worth noting in the commit message.

💬 ismaelsadeeq commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1831252615)
tracing.md doc is still saying

> 5. Replacement transaction ID (hash) as `pointer to unsigned chars` (i.e. 32 bytes in little-endian)

🤔 darosior reviewed a pull request: "descriptor: Add proper Clone function to miniscript::Node"
(https://github.com/bitcoin/bitcoin/pull/30866#pullrequestreview-2418934243)
Concept ACK.

💬 darosior commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1831410534)
The shallow copy in the version before this PR is causing a problem because we mess with a `Node`'s `subs` in the destructor to avoid a recursion stack overflow:
https://github.com/bitcoin/bitcoin/blob/2c90f8e08c4cf44d4c1ef3dde0e7f7991b8b9390/src/script/miniscript.h#L513-L524

In the scriptpubkeyman fuzz target using the seed from #30864 we would `Parse` the `tr(%17/<2;3>,l:pk(%08))` multipath descriptor into a `parsed_descs` vector of 2 `Descriptor` pointers. We would use the first of those
...

⚠️ maflcko opened an issue: "fuzz: connman target: terminate called after throwing an instance of 'std::bad_alloc'"
(https://github.com/bitcoin/bitcoin/issues/31234)
Base64 reproducer:

```
XP//////////BiAgICBbICAHAADg/4Hf394gICAgICAgIAAgICAgIHb/FiAgICAgdGggtyAgICCB
CAQAIDAXIAEAAAAAAAAAAQAAAAAAAAABAAAAAAAAgYGBgYGB7QH//2ZoZWNrcHRhYWFhYWFhl2Fh
YWFhYWFhYWFhYWFhYWFhYWFhYWFhYQAAAAAAAFxjYWFhYWFhYWFcdIFhYWFhZHJh9xP3ExPAE8BA
7/cTwBP3ExP398D3AQAAAAAAAASBgYGBgYHtAf///WdldGNmaGVja3B0YWFhYWFhYWFhl2FhYWFh
YWFhYWFhYWFhYWFhYWFhYWFhYQ==
```

```
$ sha1sum ./crash && FUZZ=connman ./bld/src/test/fuzz/fuzz ./crash
c0f5ddd240439f74d6eac83bbb67115b1ad1d209 ./crash

...

💬 maflcko commented on pull request "fuzz: fuzz connman with non-empty addrman + ASMap":
(https://github.com/bitcoin/bitcoin/pull/29536#issuecomment-2460392280)
https://github.com/bitcoin/bitcoin/issues/31234

💬 mzumsande commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1831468608)
yes, happy to ack that follow-up. I think everything is fine, but my suggestion would be something like "Can't serve the connection to..." because we already log "Serving connection to..." in the success case. And maybe add another log entry "Keeping connection alive" `else` clause in the `finally` block a few lines below to avoid conditional log messages.