💬 stickies-v commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2441355825)
re-ACK 303661871debadee5f67bd7e4cd0cccc85344ef2, thanks for adding the test.
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2441355825)
re-ACK 303661871debadee5f67bd7e4cd0cccc85344ef2, thanks for adding the test.
💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1818920353)
> Would avoid stripping const for these slightly longer lived variables, just for peace of mind.
Done, thanks.
> Not super happy about the memcmp though
agree, `memcmp` is ugly, but `std::ranges::equal` should work here.
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1818920353)
> Would avoid stripping const for these slightly longer lived variables, just for peace of mind.
Done, thanks.
> Not super happy about the memcmp though
agree, `memcmp` is ugly, but `std::ranges::equal` should work here.
💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1818920957)
Perfection! ✨👌✨
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1818920957)
Perfection! ✨👌✨
💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1818921051)
astute observation ;)
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1818921051)
astute observation ;)
💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1818921173)
Good idea, this will simplify the string generation as well since it requires a `FuzzedDataProvider` anyway
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1818921173)
Good idea, this will simplify the string generation as well since it requires a `FuzzedDataProvider` anyway
💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1818921361)
Done, thanks
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1818921361)
Done, thanks
💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1818923703)
Is this a blocker for anyone? If anyone has strong feelings about this, please let me know, we can either restructure here or provide it in another PR. Resolving for now.
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1818923703)
Is this a blocker for anyone? If anyone has strong feelings about this, please let me know, we can either restructure here or provide it in another PR. Resolving for now.
💬 maflcko commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2441368688)
I switched https://maflcko.github.io/b-c-cov/ to GCC, just to get something working again. But the config is open source and pull requests with changes are welcome: https://github.com/maflcko/b-c-cov
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2441368688)
I switched https://maflcko.github.io/b-c-cov/ to GCC, just to get something working again. But the config is open source and pull requests with changes are welcome: https://github.com/maflcko/b-c-cov
👍 brunoerg approved a pull request: "test: Fuzz the human-readable part of bech32 as well"
(https://github.com/bitcoin/bitcoin/pull/30623#pullrequestreview-2398850907)
code review ACK 9b7023d31a3ec95f66b45f0ecb47e79762d74854
(https://github.com/bitcoin/bitcoin/pull/30623#pullrequestreview-2398850907)
code review ACK 9b7023d31a3ec95f66b45f0ecb47e79762d74854
✅ dergoegge closed a pull request: "ci: Split out native fuzz jobs for macOS and windows"
(https://github.com/bitcoin/bitcoin/pull/31073)
(https://github.com/bitcoin/bitcoin/pull/31073)
💬 dergoegge commented on pull request "ci: Split out native fuzz jobs for macOS and windows":
(https://github.com/bitcoin/bitcoin/pull/31073#issuecomment-2441373851)
I still think this would make sense but there seems to be no interest. Also #30950 has been resolved.
(https://github.com/bitcoin/bitcoin/pull/31073#issuecomment-2441373851)
I still think this would make sense but there seems to be no interest. Also #30950 has been resolved.
💬 fanquake commented on pull request "depends: Specify CMake generator explicitly":
(https://github.com/bitcoin/bitcoin/pull/31171#issuecomment-2441405718)
> However, this assumption can be wrong in environments where the [CMAKE_GENERATOR](https://cmake.org/cmake/help/latest/envvar/CMAKE_GENERATOR.html) variable is set.
Generally, this holds for many CMake variables. Wouldn't a better approach be facilitating it working, rather than guaranteeing these environments never get the expected bahaviour.
(https://github.com/bitcoin/bitcoin/pull/31171#issuecomment-2441405718)
> However, this assumption can be wrong in environments where the [CMAKE_GENERATOR](https://cmake.org/cmake/help/latest/envvar/CMAKE_GENERATOR.html) variable is set.
Generally, this holds for many CMake variables. Wouldn't a better approach be facilitating it working, rather than guaranteeing these environments never get the expected bahaviour.
👍 rkrux approved a pull request: "netinfo: add peer services column and outbound-only option"
(https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2398918550)
tACK 87532fe55856efc063cf81244800da37a015ba75
Successful make and functional tests. I used the `-netinfo` command with various verbosities and the `outonly` option. Also checked for verbosities greater than 4.
Asked a question for clarity.
(https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2398918550)
tACK 87532fe55856efc063cf81244800da37a015ba75
Successful make and functional tests. I used the `-netinfo` command with various verbosities and the `outonly` option. Also checked for verbosities greater than 4.
Asked a question for clarity.
💬 rkrux commented on pull request "netinfo: add peer services column and outbound-only option":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1818964134)
Can `NODE_NONE` ever be a value here?
https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L312
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1818964134)
Can `NODE_NONE` ever be a value here?
https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L312
👍 tdb3 approved a pull request: "test: fix intermittent failure in p2p_seednode.py, don't connect to random IPs"
(https://github.com/bitcoin/bitcoin/pull/31142#pullrequestreview-2398927299)
cr and light test ACK 6c9fe7b73ea1572b8b56c716ab13d9866f91c6e9
Nice addition of the non-working proxy.
(https://github.com/bitcoin/bitcoin/pull/31142#pullrequestreview-2398927299)
cr and light test ACK 6c9fe7b73ea1572b8b56c716ab13d9866f91c6e9
Nice addition of the non-working proxy.
💬 hebasto commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2441444414)
Rebased due to the conflict with the merged https://github.com/bitcoin/bitcoin/pull/31130.
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2441444414)
Rebased due to the conflict with the merged https://github.com/bitcoin/bitcoin/pull/31130.
📝 fanquake opened a pull request: "build: increase minimum supported Windows to 10.0"
(https://github.com/bitcoin/bitcoin/pull/31172)
Follows up to https://github.com/bitcoin/bitcoin/pull/31048#discussion_r1803165670.
We definitely cannot claim that Bitcoin Core is "supported and extensively tested on" on Windows 7.
Note that #30997 is also increasing the minimum required Windows version (for the GUI) to 10.
(https://github.com/bitcoin/bitcoin/pull/31172)
Follows up to https://github.com/bitcoin/bitcoin/pull/31048#discussion_r1803165670.
We definitely cannot claim that Bitcoin Core is "supported and extensively tested on" on Windows 7.
Note that #30997 is also increasing the minimum required Windows version (for the GUI) to 10.
💬 fanquake commented on pull request "build: Bump minimum supported macOS to 13.0":
(https://github.com/bitcoin/bitcoin/pull/31048#discussion_r1818970819)
Yea, and this is already the case with the current binary. See #31172.
(https://github.com/bitcoin/bitcoin/pull/31048#discussion_r1818970819)
Yea, and this is already the case with the current binary. See #31172.
💬 hebasto commented on pull request "cmake: Add `FindZeroMQ` module":
(https://github.com/bitcoin/bitcoin/pull/30903#issuecomment-2441463955)
My Guix build:
```
aarch64
fba7e542bc6a35c65a179f9c8cd3ae2b0f8b12cd5895fa255a2d344d576d0215 guix-build-915640e191b6/output/aarch64-linux-gnu/SHA256SUMS.part
c9c1d47120de9a45aa5da1c17cc8f4860f2268bfe413d38e1b6f67ea96bd4f09 guix-build-915640e191b6/output/aarch64-linux-gnu/bitcoin-915640e191b6-aarch64-linux-gnu-debug.tar.gz
c1775debe30e5a28d981720582c5e6f36ea33939169a429edc48c4bb8ebc3688 guix-build-915640e191b6/output/aarch64-linux-gnu/bitcoin-915640e191b6-aarch64-linux-gnu.tar.gz
870a9d4f
...
(https://github.com/bitcoin/bitcoin/pull/30903#issuecomment-2441463955)
My Guix build:
```
aarch64
fba7e542bc6a35c65a179f9c8cd3ae2b0f8b12cd5895fa255a2d344d576d0215 guix-build-915640e191b6/output/aarch64-linux-gnu/SHA256SUMS.part
c9c1d47120de9a45aa5da1c17cc8f4860f2268bfe413d38e1b6f67ea96bd4f09 guix-build-915640e191b6/output/aarch64-linux-gnu/bitcoin-915640e191b6-aarch64-linux-gnu-debug.tar.gz
c1775debe30e5a28d981720582c5e6f36ea33939169a429edc48c4bb8ebc3688 guix-build-915640e191b6/output/aarch64-linux-gnu/bitcoin-915640e191b6-aarch64-linux-gnu.tar.gz
870a9d4f
...
💬 hebasto commented on pull request "build: Rename `PACKAGE_*` variables to `CLIENT_*`":
(https://github.com/bitcoin/bitcoin/pull/31042#issuecomment-2441473829)
Rebased due to the conflict with the merged https://github.com/bitcoin/bitcoin/pull/31130.
(https://github.com/bitcoin/bitcoin/pull/31042#issuecomment-2441473829)
Rebased due to the conflict with the merged https://github.com/bitcoin/bitcoin/pull/31130.