💬 ismaelsadeeq commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790355464)
nit maybe calling `mapping` a container will be better.
```suggestion
*@param mapping A container, such that mapping[i] gives the position in the new DepGraph
```

💬 ismaelsadeeq commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790049084)
nit: This loop can be modified to be more readable by using using descriptive variable names and some helpful comment on what the inner loops are doing.
```suggestion
for (ClusterIndex i = 0; i < depgraph.TxCount(); ++i) {
// Initialize the set of ancestors with just the current transaction itself.
SetType ancestor_union = SetType::Singleton(i);
// Iteratively add parents and parents of parents to the ancestor set until there is no more ancestor t
...

📝 fanquake opened a pull request: "test: remove unused code from `script_tests`"
(https://github.com/bitcoin/bitcoin/pull/31051)
This has been unused since #29648. Noticed while running a newer version of clang-tidy (19.1.1):
```bash
[127/391][6.2s] /opt/homebrew/opt/llvm/bin/clang-tidy -p=build -quiet --config-file=/bitcoin/src/.clang-tidy /bitcoin/src/test/script_tests.cpp
bitcoin/src/test/script_tests.cpp:126:25: error: local copy 'tx2' of the variable 'tx' is never modified and never used; consider removing the statement [performance-unnecessary-copy-initialization,-warnings-as-errors]
126 | CMutableTransact
...

🤔 instagibbs reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2352291145)
Remaining (non nitty) things I think to be addressed:
1) https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2391768053
2) https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1787010637

`git range-diff master ce205abe10ebccfdbea60adf4c568a8ba61390c3 ff1f2dc055efe92b3202387c66a12948134eced2`

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1790442628)
correct_tx appears in both for the txid-indexed version

🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: extend DepGraph functionality"
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2352323652)
li

💬 ismaelsadeeq commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790464732)
nit: can go further with the explanation the comments with something like

```suggestion
* i's ancestors (unless i is part of a cycle of dependencies). Note that DepGraph does not
* store set of parents; it has to be reduced from ancestor set.
```

for both the parent and child reducing methods.

💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2397283288)
> threadripper

Nice. Any machine with at least 96 CPU threads, or two machines with at least 48 cores should be a good start. (Less cores would be fine as well, but require the remote ccache, which could break. Generally, if there is something that can break, it will break, given enough time)

> (which should have local-ish mirrors for apt and etc)

`apt` should be cached in the docker image and doesn't take more than 30 seconds on a cache miss, so should be fine. Also, inside docker, any
...

💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2397286103)
(I still want to benchmark the ramdisk difference, to see how much can be squeezed out before switching, but if people think a switch should be done sooner, that is fine, too)

💬 maflcko commented on pull request "test: remove unused code from `script_tests`":
(https://github.com/bitcoin/bitcoin/pull/31051#issuecomment-2397292949)
review ACK e0287bc4b2d83cefb7af48aef457153d0729bcd4

💬 theuni commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2397293278)
I'm pretty sure the potential hardware is 24core/48threads, so it doesn't quite meet those requirements. There's another available, though I'm currently using it, but I guess sacrificing that one is an option as well :)

💬 hodlinator commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#discussion_r1790490375)
Thanks! Leftover WIP change. Removed filter in latest push here and to my personal master.

💬 maflcko commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1790492621)
Indeed, for new code, it would be good to not use the ambiguous bool fallback. Especially, given that it is undocumented?

👍 TheCharlatan approved a pull request: "refactor: Replace g_genesis_wait_cv with m_tip_block_cv"
(https://github.com/bitcoin/bitcoin/pull/30967#pullrequestreview-2352371969)
ACK fa22e5c430acaef9713d9a4b4b97bb3f4876f816

Nice improvements to the shutdown handling since I last reviewed. The nits can be ignored.

💬 TheCharlatan commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1790519737)
Nit: This `if` statement is a bit tricky to read, because of the open braces on the next line. How about adding whitespace, or adding braces to the if statement?

💬 TheCharlatan commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1790497695)
Nit: The other arguments are using curly braces, is this using parentheses on purpose?

💬 marcofleon commented on pull request "fuzz: Add fuzz-only build mode option for targets":
(https://github.com/bitcoin/bitcoin/pull/31028#discussion_r1790584206)
Agreed, that's a better way to do it. Let me know what you think about d470af336a3f66230e9c68ffc924caad0ad84980. Another possibility I was thinking of was something like:
```
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
#define CONDITIONAL_REGISTER(name, target, opts) FuzzFrameworkRegisterTarget(name, target, opts)
#else
#define CONDITIONAL_REGISTER(name, target, opts)
do {
...

💬 marcofleon commented on pull request "fuzz: Add fuzz-only build mode option for targets":
(https://github.com/bitcoin/bitcoin/pull/31028#discussion_r1790590334)
Dealing with the logic in `DETAIL_FUZZ` seems better.

💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2397513425)
> -v2transport=0 should be added to the bitcoind args used for netdriver fuzzing. Otherwise, it doesn't seem like honggfuzz ever fuzzes anything besides the v2 handshake.

Well noticed, just added it.

💬 Sjors commented on pull request "build: Bump minimum supported macOS to 12.0":
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2397513781)
Trying to test if this still works on macOS 13.7 I got stuck. Most likely unrelated to the changes here, but see #31050.