💬 fanquake commented on pull request "build: add a depends dependency provider":
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2922745196)
> Yes. The idea is to provide an approach to build core with minimal effort.
I think `make -C depends && cmake -B --toolchain` and `apt install your_dependencies && cmake -B` are both already pretty minimal effort. It's not really clear why a developer would `apt install x`, but not `apt install x y`, and then would need a behaviour to fallback to depends for `y`?
I'm also not currently aware of a platform that we support, where the dependencies needed to compile Core, aren't available vi
...
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2922745196)
> Yes. The idea is to provide an approach to build core with minimal effort.
I think `make -C depends && cmake -B --toolchain` and `apt install your_dependencies && cmake -B` are both already pretty minimal effort. It's not really clear why a developer would `apt install x`, but not `apt install x y`, and then would need a behaviour to fallback to depends for `y`?
I'm also not currently aware of a platform that we support, where the dependencies needed to compile Core, aren't available vi
...
💬 fanquake commented on pull request "depends: hard-code necessary c(xx)flags rather than setting them per-host":
(https://github.com/bitcoin/bitcoin/pull/32584#issuecomment-2922748283)
Guix Build:
```bash
c118e9965e3dc46a58e78ca4bb1573bf799d10b0a5a33c978ed5873801ded1e4 guix-build-51f6aa8ac47b/output/aarch64-linux-gnu/SHA256SUMS.part
abe0a4b2fee4bc928b73a0d8bfaeb7441a45afc01147f71ec76339844fa1a502 guix-build-51f6aa8ac47b/output/aarch64-linux-gnu/bitcoin-51f6aa8ac47b-aarch64-linux-gnu-debug.tar.gz
444bccc99b7a8910b62aef674ac9a0249f895c78ed19268017651eefeef5abd2 guix-build-51f6aa8ac47b/output/aarch64-linux-gnu/bitcoin-51f6aa8ac47b-aarch64-linux-gnu.tar.gz
ec1a9597e32d60a8
...
(https://github.com/bitcoin/bitcoin/pull/32584#issuecomment-2922748283)
Guix Build:
```bash
c118e9965e3dc46a58e78ca4bb1573bf799d10b0a5a33c978ed5873801ded1e4 guix-build-51f6aa8ac47b/output/aarch64-linux-gnu/SHA256SUMS.part
abe0a4b2fee4bc928b73a0d8bfaeb7441a45afc01147f71ec76339844fa1a502 guix-build-51f6aa8ac47b/output/aarch64-linux-gnu/bitcoin-51f6aa8ac47b-aarch64-linux-gnu-debug.tar.gz
444bccc99b7a8910b62aef674ac9a0249f895c78ed19268017651eefeef5abd2 guix-build-51f6aa8ac47b/output/aarch64-linux-gnu/bitcoin-51f6aa8ac47b-aarch64-linux-gnu.tar.gz
ec1a9597e32d60a8
...
📝 fanquake opened a pull request: "build: add -Wthread-safety-pointer"
(https://github.com/bitcoin/bitcoin/pull/32647)
This will become available, and on by default in Clang 21:
> ThreadSafetyAnalysis now supports -Wthread-safety-pointer, which
> enables warning on passing or returning pointers to guarded variables
> as function arguments or return value respectively. Note that
> ThreadSafetyAnalysis still does not perform alias analysis. The
> feature will be default-enabled with -Wthread-safety in a future release.
See https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst.
Upd
...
(https://github.com/bitcoin/bitcoin/pull/32647)
This will become available, and on by default in Clang 21:
> ThreadSafetyAnalysis now supports -Wthread-safety-pointer, which
> enables warning on passing or returning pointers to guarded variables
> as function arguments or return value respectively. Note that
> ThreadSafetyAnalysis still does not perform alias analysis. The
> feature will be default-enabled with -Wthread-safety in a future release.
See https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst.
Upd
...
💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-2922787970)
tACK d253fa9ebe4305118417a02aa72cc06810662ac6
I tested on top of af227a0e7d9754623d36925c56599f9b921f5cb3 from #31802 on macOS 15.5 and Windows 11 (not impacted).
The `bitcoin test` command is perhaps a bit too well hidden. Maybe it's better to drop the `help` command and put a bit more emphasis on the `--all` option:
```
Commands:
gui [ARGS] Start GUI, equivalent to running 'bitcoin-qt [ARGS]' or 'bitcoin-gui [ARGS]'
...
tx ...
Use 'bitcoin --help --all' for additional
...
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-2922787970)
tACK d253fa9ebe4305118417a02aa72cc06810662ac6
I tested on top of af227a0e7d9754623d36925c56599f9b921f5cb3 from #31802 on macOS 15.5 and Windows 11 (not impacted).
The `bitcoin test` command is perhaps a bit too well hidden. Maybe it's better to drop the `help` command and put a bit more emphasis on the `--all` option:
```
Commands:
gui [ARGS] Start GUI, equivalent to running 'bitcoin-qt [ARGS]' or 'bitcoin-gui [ARGS]'
...
tx ...
Use 'bitcoin --help --all' for additional
...
💬 stickies-v commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2922814814)
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
No changes since 35bcd8eed38130445aef5ebe217ab42248fa6f18 except rebasing and removing the `-datacarriersize=100000` no-op in `mempool_updatefromblock.py` that would cause the test to fail just for using a deprecated option.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2922814814)
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
No changes since 35bcd8eed38130445aef5ebe217ab42248fa6f18 except rebasing and removing the `-datacarriersize=100000` no-op in `mempool_updatefromblock.py` that would cause the test to fail just for using a deprecated option.
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2116212200)
Yup I had thought about this and wasn't sure how to evaluate the overhead. I'll change it.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2116212200)
Yup I had thought about this and wasn't sure how to evaluate the overhead. I'll change it.
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2116215143)
Yup, this makes sense as well. Can change.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2116215143)
Yup, this makes sense as well. Can change.
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2116217752)
I think callers just need to be aware that they cannot set the rate limit very high in certain cases. Maybe this change and a comment will suffice?
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2116217752)
I think callers just need to be aware that they cannot set the rate limit very high in certain cases. Maybe this change and a comment will suffice?
💬 Sjors commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2922826515)
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
Just a rebase and minor test tweak since my previous review.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2922826515)
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
Just a rebase and minor test tweak since my previous review.
💬 polespinasa commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2922833884)
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
Just a rebase due to silent merge conflict since my prev review
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2922833884)
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
Just a rebase due to silent merge conflict since my prev review
📝 hebasto opened a pull request: "cmake, qt: Process `*.qrc` files manually"
(https://github.com/bitcoin/bitcoin/pull/32648)
The first commit enables modification of the properties of the source files produced by Qt Resource Compiler, which helps, for example, to silence compiler warnings.
The second commit makes use of this new capability to silence `-Wtrailing-whitespace` warnings and resolves https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-2876950405.
(https://github.com/bitcoin/bitcoin/pull/32648)
The first commit enables modification of the properties of the source files produced by Qt Resource Compiler, which helps, for example, to silence compiler warnings.
The second commit makes use of this new capability to silence `-Wtrailing-whitespace` warnings and resolves https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-2876950405.
👍 theStack approved a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2881862929)
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
(as per `$ git range-diff 35bcd8ee...a189d636`)
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2881862929)
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
(as per `$ git range-diff 35bcd8ee...a189d636`)
💬 hebasto commented on pull request "build: add `-W[leading|trailing]-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-2922854588)
> > GCC 15 now has options to turn these into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable these options.
>
> Concept ACK on that.
>
> > Unfortunately, CMake/Qt will generate code that contains trailing whitespace:
> > ```shell
> > [ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o
> > /root/bitcoin/build/src/qt/bitcoinqt_autoge
...
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-2922854588)
> > GCC 15 now has options to turn these into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable these options.
>
> Concept ACK on that.
>
> > Unfortunately, CMake/Qt will generate code that contains trailing whitespace:
> > ```shell
> > [ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o
> > /root/bitcoin/build/src/qt/bitcoinqt_autoge
...
💬 TheBlueMatt commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2922883838)
> I'm not super familiar with FIBRE -- does it have a chance of being used again? Would dropping unsolicited compact blocks prevent FIBRE from working again?
I believe as of a week of two ago there's work to revive it!
> This is a good point, you would need to be the fastest peer to exploit it in the non-blocksonly case.
This was only half my point - my larger point was that we can't really fix this issue because if you're the fastest peer once you're gonna be the fastest peer again. I'm not
...
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2922883838)
> I'm not super familiar with FIBRE -- does it have a chance of being used again? Would dropping unsolicited compact blocks prevent FIBRE from working again?
I believe as of a week of two ago there's work to revive it!
> This is a good point, you would need to be the fastest peer to exploit it in the non-blocksonly case.
This was only half my point - my larger point was that we can't really fix this issue because if you're the fastest peer once you're gonna be the fastest peer again. I'm not
...
⚠️ Jezmaul opened an issue: "Wallet"
(https://github.com/bitcoin/bitcoin/issues/32649)
https://github.com/bitcoin/bitcoin/blob/3E6RsQwEWiCLvZ3Lq33X1dR43VLTzYK9gg/src/wallet/sqlite.cpp#L719
(https://github.com/bitcoin/bitcoin/issues/32649)
https://github.com/bitcoin/bitcoin/blob/3E6RsQwEWiCLvZ3Lq33X1dR43VLTzYK9gg/src/wallet/sqlite.cpp#L719
✅ fanquake closed an issue: "Wallet"
(https://github.com/bitcoin/bitcoin/issues/32649)
(https://github.com/bitcoin/bitcoin/issues/32649)
💬 Crypt-iQ commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2922992916)
> This was only half my point - my larger point was that we can't really fix this issue because if you're the fastest peer once you're gonna be the fastest peer again. I'm not sure it's worth trying to fix it broadly given the impact it might have (even if small) and the fact that it isn't really a great fix.
Hmm, I agree with this. I guess the solution is a better protocol to avoid fingerprinting? Adding more complexity here sounds a bit scary.
I looked at the WIP fibre patchset here: https:/
...
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2922992916)
> This was only half my point - my larger point was that we can't really fix this issue because if you're the fastest peer once you're gonna be the fastest peer again. I'm not sure it's worth trying to fix it broadly given the impact it might have (even if small) and the fact that it isn't really a great fix.
Hmm, I agree with this. I guess the solution is a better protocol to avoid fingerprinting? Adding more complexity here sounds a bit scary.
I looked at the WIP fibre patchset here: https:/
...
💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-2923023983)
> I.e. be the first to supply the latest valid block at the tip and you'll be selected as high bandwidth (we process unsolicited block messages, so this should be trivial).
Yup I think this means the defense is ineffective.
> Even if you're low-bandwidth can't you just send a headers announcement immediately followed by the compact block? As long as your headers message is the first announcement of the block, the compact block will be requested and since these messages are processed sequen
...
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-2923023983)
> I.e. be the first to supply the latest valid block at the tip and you'll be selected as high bandwidth (we process unsolicited block messages, so this should be trivial).
Yup I think this means the defense is ineffective.
> Even if you're low-bandwidth can't you just send a headers announcement immediately followed by the compact block? As long as your headers message is the first announcement of the block, the compact block will be requested and since these messages are processed sequen
...
💬 Crypt-iQ commented on pull request "p2p: Add mutation check inside compact block's FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#issuecomment-2923073843)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32646#issuecomment-2923073843)
Concept ACK
🤔 1440000bytes reviewed a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2882335536)
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
<details>
<summary>Signature</summary>
<pre>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
-----BEGIN PGP SIGNATURE-----
iHUEARYKAB0WIQSSUwYqT5LWNFkIXKYtIwUgISpZAQUCaDoHIwAKCRAtIwUgISpZ
AXfEAQCLt/Tzit+ZxCmsP0BkCi4zG+4OyFHcvzytQ6SJFQuUjwD/TsjUydeA06Ft
RcJvG/+brpVM8NV3Ga/trp74dg67NQE=
=xtxJ
-----END PGP SIGNATURE-----
</pre>
</details>
Previous review: https://github.com/b
...
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2882335536)
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
<details>
<summary>Signature</summary>
<pre>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
-----BEGIN PGP SIGNATURE-----
iHUEARYKAB0WIQSSUwYqT5LWNFkIXKYtIwUgISpZAQUCaDoHIwAKCRAtIwUgISpZ
AXfEAQCLt/Tzit+ZxCmsP0BkCi4zG+4OyFHcvzytQ6SJFQuUjwD/TsjUydeA06Ft
RcJvG/+brpVM8NV3Ga/trp74dg67NQE=
=xtxJ
-----END PGP SIGNATURE-----
</pre>
</details>
Previous review: https://github.com/b
...