💬 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
...
(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)
(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
(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 :)
(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.
(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?
(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.
(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?
(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?
(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 {
...
(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.
(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.
(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.
(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.
💬 marcofleon commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2397522603)
I don't have a strong preference for which one is supported by the build system. I happened to learn about [llvm-cov](https://clang.llvm.org/docs/SourceBasedCodeCoverage.html) before coming upon the [coverage](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#compiling-for-test-coverage) section in the docs, so that's what I've been using on both macOS and Linux. It's been working well for me so far.
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2397522603)
I don't have a strong preference for which one is supported by the build system. I happened to learn about [llvm-cov](https://clang.llvm.org/docs/SourceBasedCodeCoverage.html) before coming upon the [coverage](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#compiling-for-test-coverage) section in the docs, so that's what I've been using on both macOS and Linux. It's been working well for me so far.
🤔 pablomartin4btc reviewed a pull request: "depends: Print ready-to-use `--toolchain` option for CMake invocation"
(https://github.com/bitcoin/bitcoin/pull/31008#pullrequestreview-2352574245)
ACK 605926da0ab4ac7ae4e9aed55a059f37c31c15b5
(https://github.com/bitcoin/bitcoin/pull/31008#pullrequestreview-2352574245)
ACK 605926da0ab4ac7ae4e9aed55a059f37c31c15b5
💬 Sjors commented on pull request "ci: Add missing -DWERROR=ON to test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/31045#issuecomment-2397533799)
> by assuming `nproc` exists on macos as well
It doesn't, but you can do `alias nproc="sysctl -n hw.physicalcpu"` instead of installing all of `coreutils`. Though on CI that's fine by me too.
https://gist.github.com/oleg-andreyev/053b90ef33d2c29446ef466a2817d01c
(https://github.com/bitcoin/bitcoin/pull/31045#issuecomment-2397533799)
> by assuming `nproc` exists on macos as well
It doesn't, but you can do `alias nproc="sysctl -n hw.physicalcpu"` instead of installing all of `coreutils`. Though on CI that's fine by me too.
https://gist.github.com/oleg-andreyev/053b90ef33d2c29446ef466a2817d01c
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790645774)
I have incorporated this change.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790645774)
I have incorporated this change.
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790649176)
Done.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790649176)
Done.
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790648786)
Done, with a few changes.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790648786)
Done, with a few changes.
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790648128)
I wouldn't call a Span a container; I have changed it to just saying "A Span such that ...".
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790648128)
I wouldn't call a Span a container; I have changed it to just saying "A Span such that ...".