💬 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
...
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1790699672)
I didn't mean to suggest a change to the logic, just to make it more conditional on `-privatebroadcast`, so that in principle we wouldn't even need to initiate a `m_private_broadcast` object unless the user is running in `-privatebroadcast` mode. But I don't have a concrete suggestion right now (I might suggest on at a later point but need to look deeper into the grant logic for that) so it's fine to resolve for now.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1790699672)
I didn't mean to suggest a change to the logic, just to make it more conditional on `-privatebroadcast`, so that in principle we wouldn't even need to initiate a `m_private_broadcast` object unless the user is running in `-privatebroadcast` mode. But I don't have a concrete suggestion right now (I might suggest on at a later point but need to look deeper into the grant logic for that) so it's fine to resolve for now.
👍 BrandonOdiwuor approved a pull request: "test: remove unused code from `script_tests`"
(https://github.com/bitcoin/bitcoin/pull/31051#pullrequestreview-2352705365)
ACK e0287bc4b2d83cefb7af48aef457153d0729bcd4
(https://github.com/bitcoin/bitcoin/pull/31051#pullrequestreview-2352705365)
ACK e0287bc4b2d83cefb7af48aef457153d0729bcd4
💬 fanquake commented on issue "win64-cross CI timeout after 2h ":
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2397658116)
Hitting the 20m `--timeout` https://cirrus-ci.com/task/5886749702881280:
```bash
[17:40:38.816] 129/136 Test #135: walletload_tests ..................... Passed 4.68 sec
[17:40:42.991] 130/136 Test #132: wallet_tests ......................... Passed 11.89 sec
[17:40:47.154] 131/136 Test #122: coinselector_tests ................... Passed 23.22 sec
[17:40:54.153] 132/136 Test #1: util_test_runner ..................... Passed 119.52 sec
[17:41:12.100] 133/136 Test #5: nov
...
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2397658116)
Hitting the 20m `--timeout` https://cirrus-ci.com/task/5886749702881280:
```bash
[17:40:38.816] 129/136 Test #135: walletload_tests ..................... Passed 4.68 sec
[17:40:42.991] 130/136 Test #132: wallet_tests ......................... Passed 11.89 sec
[17:40:47.154] 131/136 Test #122: coinselector_tests ................... Passed 23.22 sec
[17:40:54.153] 132/136 Test #1: util_test_runner ..................... Passed 119.52 sec
[17:41:12.100] 133/136 Test #5: nov
...
💬 pablomartin4btc commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2397799341)
> > Ninja is required to build the `qt` package in depends. It is mentioned in `depends/README.md`. However, `doc/build-osx.md` does not cover building with depends, so I don't think it is necessary to mention Ninja there.
>
> Could you please explain why this is the case? Ninja seems like an odd requirement here.
It's described in the Qt 6/ 6.7 [dependencies](https://doc.qt.io/qt-6/integrity-installing-dependencies.html). I've tested it myself and can't build `depends` without `ninja-buil
...
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2397799341)
> > Ninja is required to build the `qt` package in depends. It is mentioned in `depends/README.md`. However, `doc/build-osx.md` does not cover building with depends, so I don't think it is necessary to mention Ninja there.
>
> Could you please explain why this is the case? Ninja seems like an odd requirement here.
It's described in the Qt 6/ 6.7 [dependencies](https://doc.qt.io/qt-6/integrity-installing-dependencies.html). I've tested it myself and can't build `depends` without `ninja-buil
...