👍 ismaelsadeeq approved a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2665558763)
Code review ACK 72a97c0a07ea6e5a95ab37c8d95e1ea02cff8e92 🛸

I've reviewed each commit and also looked at how the interface is being used by `CTxMemPool` in the big PR

🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: add txgraph diagrams/mining/eviction"
(https://github.com/bitcoin/bitcoin/pull/31444#pullrequestreview-2665561910)
Approach ACK will review in-depth soon

💬 darosior commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2704884264)
The code in this pull request is of good quality and is well tested, but it's not clear to me what's the goal with this pull request nor how it fits in the big picture. As this introduces a new consensus feature, it seems premature to merge this into Bitcoin Core until widespread consensus is reached among Bitcoin users to eventually activate this new feature in a soft fork.

To be clear, i am not saying the details of the activation need to be figured out before the implementation of the feat
...

💬 achow101 commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2704913533)
Needs rebase

💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2704915598)
Rebased

💬 achow101 commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2704952974)
ACK 89fd9a4a87d772798671da18538e45f0272365e4

💬 davidgumberg commented on pull request "RFC contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2704999209)
Concept ACK, while it might not be strictly necessary for this tarball to be deterministic, I think matching hashes of tarballs is the simplest way to verify that you've followed the same procedure as others to build the macOS sdk and have gotten the same result.

150MB seems to me a small price to pay to fix #31873 and reduce surface area for nondeterminism in generating the macOS sdk.

💬 l0rinc commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1984087810)
For consistency this can also be:
```suggestion
PATTERN_ONION = re.compile(r"^([a-z2-7]{56}\.onion):(\d{1,5})$")
```

💬 l0rinc commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1984089787)
1) the `chainparams: add signet fixed seeds if default network` commit should go after `seeds: update fixed dns seeds` which introduces `chainparams_seed_signet`
2) Now that we're pre-clearing it at the beginning, it may be slightly simpler to do:
```suggestion
vFixedSeeds.assign(std::begin(chainparams_seed_signet), std::end(chainparams_seed_signet));

```

💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2705005806)
Moved 49f155efbfb65ab60c7c67597f68489893015c71 before the chainparams updates so the commit-by-commit CI passes.

💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1984093001)
Already repushed, with no change apart from commit order for the CI (thanks!)

💬 l0rinc commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2705008756)
I have mostly reviewed the regexes, for the rest it's only a very lightweight ACK f0b659716bd455dca02053df573d888b5a115aa4

I see you've added new values to `23`,`26`,`27`,`28` as well in `makeseeds: update user agent rege` - I can't validate those, but I assume you had a reason to extend them.

💬 l0rinc commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1984094367)
I see, what do you think about 2)?

💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2705011713)
> I see you've added new values to `23`,`26`,`27`,`28` as well in `makeseeds: update user agent rege` - I can't validate those, but I assume you had a reason to extend them.

Those are for additional releases, e.g. seen when running `git tag -n | sort -V` (thanks for reviewing).

💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1984096837)
Will do if I need to re-run the scripts and re-push.

💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1984100831)
No strong opinion, might leave as-is for consistency with the existing code in that file setting vFixedSeeds for mainnet, test and testnet4.

💬 mzumsande commented on pull request "validation: set BLOCK_FAILED_CHILD correctly":
(https://github.com/bitcoin/bitcoin/pull/31835#discussion_r1984065154)
I think this would be better suited in a different test file since invalidateblock is not part of `interfaces/chain.h`: While the existing test already calls `InvalidateBlock()`, this is just as a means to the end to test `findCommonAncestor`. Maybe `blockchain_tests.cpp` would be a better place?

💬 darosior commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1984063154)
This is a bit confusing, don't you think it's preferable to directly have another member in `BIP9Info` to convey this to the RPC command rather than (ab)using the stats' members?

💬 darosior commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1982178935)
No need for the condition on test network and the seemingly-magic 2016 value here, you could just always return `DifficultyAdjustmentInterval()`?

🤔 darosior reviewed a pull request: "versionbits refactoring"
(https://github.com/bitcoin/bitcoin/pull/29039#pullrequestreview-2662479079)
Concept ACK. A couple comments as i was reading through, will delve deeper soon.