💬 Sjors commented on pull request "doc: add missing packages for BSDs (cmake, gmake, curl) to depends/README.md":
(https://github.com/bitcoin/bitcoin/pull/32711#issuecomment-2959008370)
re-utACK 89526deddf87904619b26319e8d149cf97466868

🚀 fanquake merged a pull request: "doc: add missing packages for BSDs (cmake, gmake, curl) to depends/README.md"
(https://github.com/bitcoin/bitcoin/pull/32711)

💬 fanquake commented on pull request "doc: add missing packages for BSDs (cmake, gmake, curl) to depends/README.md":
(https://github.com/bitcoin/bitcoin/pull/32711#issuecomment-2959026171)
Backported to 29.x in #32589.

💬 Crypt-iQ commented on pull request "doc: fuzz: fix AFL++ link":
(https://github.com/bitcoin/bitcoin/pull/32713#discussion_r2137822742)
Anecdotally, I've had at least one situation where afl-clang-lto wouldn't build but I forget which clang version I was on.

💬 fanquake commented on pull request "cmake: Fix `FindQt` module":
(https://github.com/bitcoin/bitcoin/pull/32709#discussion_r2137823180)
In 89d3daf823e4350667b42473c0924430ea7400bc: Why is `XKB_*` being added here?

👍 ryanofsky approved a pull request: "index: move disk read lookups to base class"
(https://github.com/bitcoin/bitcoin/pull/32694#pullrequestreview-2913210912)
Code review ACK 3f390efa313f4ac43e9134d5299d7faec74cca60. Nice changes! This gets rid of blockstorage and CBlockIndex* accesses from index subclasses during sync to decouple the indexes from node internals and make new indexes more straightforward to write in the future. It should also simplify the parallelization PR and avoid the need for refactoring there.

💬 ryanofsky commented on pull request "index: move disk read lookups to base class":
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2137685544)
In commit "test: indexes, avoid creating threads when sync runs synchronously" (331a25cb16632042dd6782a9b62fcc5c8aa6da3b)

Motivation for this commit seems unclear. It seems to initialize indexes in unit tests differently than they are initialized in the node and reduce test coverage a little. Maybe commit message can be updated to mention reasons for wanting to do this?

💬 ryanofsky commented on pull request "index: move disk read lookups to base class":
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2137774461)
In commit "index: remove CBlockIndex access from CustomAppend()" (3f390efa313f4ac43e9134d5299d7faec74cca60)

It's a bit inefficient to copy the undo data here, and the code seems overcomplicated because the `block_undo` variable is only referenced once, 60 lines below. Would suggest dropping the `block_undo` variable and just accessing `block.undo_data` directly below.

The `block_undo` variable in BlockFilterIndex::CustomAppend could also go away to simplify that function.

💬 ryanofsky commented on pull request "index: move disk read lookups to base class":
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2137763212)
In commit "index: remove CBlockIndex access from CustomAppend()" (3f390efa313f4ac43e9134d5299d7faec74cca60)

Should be `ProcessBlock(pindex, block.get())` to avoid rereading the block from disk?

💬 ryanofsky commented on pull request "index: move disk read lookups to base class":
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2137710074)
In commit "test: indexes, avoid creating threads when sync runs synchronously" (331a25cb16632042dd6782a9b62fcc5c8aa6da3b)

This seems ok but the description is now longer than the function is and it seems to repeat a number of uninteresting details about what the function is doing. Maybe consider dropping the function level comment and just adding a short comment above the findfork call on line 151 since that is the part that seems to be surprising. Something like "Since block is not in the ch
...

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2137865722)
Here is what I had in mind, making `IsUnique` work by ByWtxid iterator only, and making `Erase` figure out uniqueness of the passed argument by itself: https://github.com/sipa/bitcoin/commits/pr31829

👍 ryanofsky approved a pull request: "ci: Rewrite test-each-commit as py script"
(https://github.com/bitcoin/bitcoin/pull/32680#pullrequestreview-2913521833)
Code review ACK fa9cfdf3be754b01e6ce73a0cc2f998b81e12970

💬 ryanofsky commented on pull request "ci: Rewrite test-each-commit as py script":
(https://github.com/bitcoin/bitcoin/pull/32680#discussion_r2137869796)
Could add file=sys.stderr if want to emulate bash tracing (not necessarily better though)

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2137884115)
Ah, you'd need the `ffff...ffff` Wtxid for that, but I think you can avoid it by incrementing the peer instead:

```c++
auto it = index_by_peer.lower_bound(ByPeerView{peer, false, 0});
auto it_end = index_by_peer.lower_bound(ByPeerView{peer + 1, false, 0});
```

💬 fjahr commented on pull request "init, doc: Replace datacarrier(size) deprecation with non-recommendation text":
(https://github.com/bitcoin/bitcoin/pull/32714#issuecomment-2959250396)
Concept NACK

I personally prefer to keep the deprecation warning as I think all the rationale given (among many other places) [here](https://bitcoincore.org/en/2025/06/06/relay-statement/) and [here](https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2955614286) points to the fact that it should be removed some time in the future. The fact that outsiders don't understand deprecation and how it is handled in Bitcoin Core is not a strong argument. I am also surprised by this because the
...

💬 maflcko commented on pull request "ci: Rewrite test-each-commit as py script":
(https://github.com/bitcoin/bitcoin/pull/32680#discussion_r2137916712)
CI usually combines both stdout and stderr into one log file, so I'll leave this as-is for now for simplicity.

🤔 TheCharlatan reviewed a pull request: "index: move disk read lookups to base class"
(https://github.com/bitcoin/bitcoin/pull/32694#pullrequestreview-2913396907)
Approach ACK

This felt all felt very familiar :)

Can you double-check the commit message of 50696f07784b5dcf04a398f7592dead4dde6d2e9. I think there is a word too much in the first and second sentence.

💬 TheCharlatan commented on pull request "index: move disk read lookups to base class":
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2137793161)
Nit: There is no reason to declare this (and CBlockUndo) outside the if statement, right?

💬 yuvicc commented on pull request "test: Fix reorg patterns in mempool tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-2959307381)
Addressed some nits and rebased with master!

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2137937680)
Yeah, it didn't work for me yesterday for some reason. Taking another look.