💬 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 ...".
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790649002)
Done.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790649002)
Done.
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790647190)
I have phrased it as "this information has to be inferred from the ancestor sets" (and similar for `GetReducedChildren`).
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790647190)
I have phrased it as "this information has to be inferred from the ancestor sets" (and similar for `GetReducedChildren`).
💬 ismaelsadeeq commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790656735)
much better 👍🏾
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790656735)
much better 👍🏾
🤔 pablomartin4btc reviewed a pull request: "build: Switch to Qt 6"
(https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2352608848)
tACK dae1d48c546c2e1daadbf982420ba3d8feda2a9f
Tested on Ubuntu 22.04 using `-DCMAKE_BUILD_TYPE=Debug` work fine.
With `depends`:
`-- Found Qt: /home/pablo/workspace/bitcoin/depends/x86_64-pc-linux-gnu/lib/cmake/Qt6 (found suitable version "6.7.3", minimum required is "6.2") `
In order to run `bitcoin-qt` I had to install `libxcb-cursor0` (as expected specified in the description). Is this going to be clarified somewhere?
tACK dae1d48c546c2e1daadbf982420ba3d8feda2a9f
Tested on Ubuntu 22.04 using `-DCMAKE_BUILD_TYPE=Debug` work fine.
With `depends`:
`-- Found Qt: /home/pablo/workspace/bitcoin/depends/x86_64-pc-linux-gnu/lib/cmake/Qt6 (found suitable version "6.7.3", minimum required is "6.2") `
In order to run `bitcoin-qt` I had to install `libxcb-cursor0` (as expected specified in the description). Is this going to be clarified somewhere?
![image](https://github.com/user-attachments/assets/000109cd
...
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1790666758)
What if the peer doesn't answer our ping (because they are broken / malicious)? I think we would never disconnect them because the disconnection logic is inside of `MaybeSendPing()`, which will never be called again for private peers.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1790666758)
What if the peer doesn't answer our ping (because they are broken / malicious)? I think we would never disconnect them because the disconnection logic is inside of `MaybeSendPing()`, which will never be called again for private peers.
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1790700993)
> Which one?
No strong opinion (I think that both should work), but the second one (also check wtxid) seems simpler.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1790700993)
> Which one?
No strong opinion (I think that both should work), but the second one (also check wtxid) seems simpler.
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1790649950)
The final fixup commit leads to the question what should happen if the peer doesn't send a GETDATA to our INV. I think this would happen in practice if they are one of the later broadcast attempts and already received the tx from someone else (as a result of our first attempts). I think that we would currently never disconnect on our end, but they might disconnect us after `TIMEOUT_INTERVAL=20min` because we don't answer their ping. But if they don't have this logic the connection might stay ope
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1790649950)
The final fixup commit leads to the question what should happen if the peer doesn't send a GETDATA to our INV. I think this would happen in practice if they are one of the later broadcast attempts and already received the tx from someone else (as a result of our first attempts). I think that we would currently never disconnect on our end, but they might disconnect us after `TIMEOUT_INTERVAL=20min` because we don't answer their ping. But if they don't have this logic the connection might stay ope
...