💬 achow101 commented on pull request "Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test":
(https://github.com/bitcoin/bitcoin/pull/30681#issuecomment-2302548350)
ACK 59ff17e5af4e382cbe16f183767beef1bdcd9131

💬 marcofleon commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2302558442)
> Perhaps it would be good to mention this in docs?

Agreed @brunoerg. We can add it post-merge, assuming people are on board with the method.

👍 ryanofsky approved a pull request: "test: [refactor] Use m_rng directly"
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2251432210)
Code review ACK b6b2b38aa77162420babb34e01dd510512411abf. Just simplified prevector_tests and removed misleading BOOST_CHECK_MESSAGE print since last review.

💬 ryanofsky commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1725435882)
In commit "test: Correct the random seed log on a prevector test failure" (faa2608e6c3e1763d0991443c3ce6d9bb1466da8)

I think it would be nice to just fix the bug and print the seed when the test fails rather than requiring looking up the seed in logs. It seems like we could easily support this by adding `return seed` as the last line of `SeedRandomForTest` and this would be helpful for debugging tests that only fail with certain seeds, because they could be run without always enabling logging
...

💬 achow101 commented on pull request "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate":
(https://github.com/bitcoin/bitcoin/pull/30636#issuecomment-2302595244)
ACK 917e70a6206c62c4c492fa922425fc8e00d3f328

💬 l0rinc commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2302599944)
ACK c139a788e0052ece8f6c5689e4cd04b406032875

🚀 achow101 merged a pull request: "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate"
(https://github.com/bitcoin/bitcoin/pull/30636)

📝 pablomartin4btc opened a pull request: "devtools, utxo-snapshot: Fix block height out of range in script"
(https://github.com/bitcoin/bitcoin/pull/30690)
<details>
<summary>Fixing a <a href="https://github.com/bitcoin/bitcoin/pull/28553#pullrequestreview-2251032570">bug</a> in <code>utxo_snapshot.sh</code>.</summary>

```
/contrib/devtools/utxo_snapshot.sh 840000 snapshot2.dat ./src/bitcoin-cli -datadir=${AU_DATADIR}
Do you want to disable network activity (setnetworkactive false) before running invalidateblock? (Y/n):
Disabling network activity
false
error code: -8
error message:
Block height out of range
```

And the user will see
...

💬 brunoerg commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1725475551)
Cool, thank you.

👍 theuni approved a pull request: "depends: Fix CMake-generated `libzmq.pc` file"
(https://github.com/bitcoin/bitcoin/pull/30508#pullrequestreview-2251549063)
Nice. UtACK f1e39c4594295b4ce5976145ee665e38f2f8e5f1

📝 achow101 opened a pull request: "Fix maybe-uninitialized warning in IsSpentKey"
(https://github.com/bitcoin/bitcoin/pull/30691)
After 6ed424f2db609f9f39ec1d1da2077c7616f3a0c2, I started seeing a maybe-unitialized warning in `CWallet::IsSpentKey`:

```
In destructor ‘constexpr std::_Vector_base<_Tp, _Alloc>::~_Vector_base() [with _Tp = unsigned char; _Alloc = std::allocator<unsigned char>]’,
inlined from ‘constexpr std::vector<_Tp, _Alloc>::~vector() [with _Tp = unsigned char; _Alloc = std::allocator<unsigned char>]’ at /usr/lib/gcc/x86_64-pc-linux-gnu/13.3.0/include/c++/bits/stl_vector.h:738:7,
inlined from
...

👍 instagibbs approved a pull request: "cluster mempool: optimized candidate search"
(https://github.com/bitcoin/bitcoin/pull/30286#pullrequestreview-2251100080)
code review ACK 1b8f143fb791fe5ba40ce6610cd4e8e04b951495

Thanks for dropping those couple commits.

> I think this will need analysis with real-world clusters to see actual impact, which may be easier once the cluster mempool project is further along. I've dropped it for this reason.

On that note I think we'll need some diagram-quality style benchmarks to make informed decisions in the future, otherwise it makes ongoing changes difficult to justify.

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1725236618)
a12ca7b8d99d868ff49e90d33b5e90c169849ccc

may want to consider something like `m_sorted_depgraph`

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1725257738)
same check already being done in split_fn, but I also like it here on creation
```Suggestion
inc(std::move(i)), und(std::move(u)), pot_feerate(std::move(p_f)) {
Assume(pot_feerate.IsEmpty() == inc.feerate.IsEmpty());
}
```

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1725534553)
new name is better and makes flow easier to understand,thanks

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1725548581)
:facepalm: I was missing the `static` in front of `prevout_hash`. This is making a lot more sense now.

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1725548968)
might be worth a comment!

💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1725555941)
Done.

💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1725556045)
Done.

💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2302743849)
> On that note I think we'll need some diagram-quality style benchmarks to make informed decisions in the future, otherwise it makes ongoing changes difficult to justify.

FWIW, the two dropped commits are pure optimizations (they should have zero effect on the outcome, or the number of iterations performed; just on the amount of time spent per iteration).

I'll add an overview of the expected impact of each commit in this PR description.