💬 TheCharlatan commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1746748313)
How about making the parameter `&&` to avoid accidental copies?
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1746748313)
How about making the parameter `&&` to avoid accidental copies?
💬 ismaelsadeeq commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1746787689)
It was deliberately added in other to allow caller to decide whether to move or make a copy see https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746002806
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1746787689)
It was deliberately added in other to allow caller to decide whether to move or make a copy see https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746002806
💬 TheCharlatan commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1746797435)
Ah, I see. Please resolve :)
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1746797435)
Ah, I see. Please resolve :)
💬 vasild commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2333640042)
> @vasild it's probably easier to reverse those two commits: first move the implementation out, and then split the class.
`e995ffa5c3...e7cf8e8fc6`: done, same as before, the cumulative diff is identical before and after:
```sh
$ diff -u <(git diff e59097a0a5~2..e59097a0a5) <(git diff e995ffa5c~3..e995ffa5c)
$ diff -u <(git diff e59097a0a5~2..e59097a0a5) <(git diff e7cf8e8fc~3..e7cf8e8fc)
$
```
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2333640042)
> @vasild it's probably easier to reverse those two commits: first move the implementation out, and then split the class.
`e995ffa5c3...e7cf8e8fc6`: done, same as before, the cumulative diff is identical before and after:
```sh
$ diff -u <(git diff e59097a0a5~2..e59097a0a5) <(git diff e995ffa5c~3..e995ffa5c)
$ diff -u <(git diff e59097a0a5~2..e59097a0a5) <(git diff e7cf8e8fc~3..e7cf8e8fc)
$
```
💬 ismaelsadeeq commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1746805357)
Thanks
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1746805357)
Thanks
💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333643144)
Thanks for clarifying! I think not counting "invalid" ones would be wrong, because tinyformat treats them as valid. Silently not counting them is another bug in the Python code, because it will lead to a `tinyformat: Too many conversion specifiers in format string`. See https://godbolt.org/z/4ncfqvnjY
I didn't really want to give a full list of all bugs in the Python code, but I've modified the PR summary to mention this bug as well. Let me know if this sounds acceptable.
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333643144)
Thanks for clarifying! I think not counting "invalid" ones would be wrong, because tinyformat treats them as valid. Silently not counting them is another bug in the Python code, because it will lead to a `tinyformat: Too many conversion specifiers in format string`. See https://godbolt.org/z/4ncfqvnjY
I didn't really want to give a full list of all bugs in the Python code, but I've modified the PR summary to mention this bug as well. Let me know if this sounds acceptable.
💬 fanquake commented on pull request "ci: Use clang-19 in msan tasks":
(https://github.com/bitcoin/bitcoin/pull/30639#issuecomment-2333665854)
Given that final has been pushed back a bit, also happy to put this in at `rc4`, and bump it again later (if needed).
(https://github.com/bitcoin/bitcoin/pull/30639#issuecomment-2333665854)
Given that final has been pushed back a bit, also happy to put this in at `rc4`, and bump it again later (if needed).
💬 maflcko commented on issue "ci: failure in win64 unit tests":
(https://github.com/bitcoin/bitcoin/issues/30792#issuecomment-2333669861)
Yeah, possibly. I wonder why there is a presumed increase in those errors recently.
(https://github.com/bitcoin/bitcoin/issues/30792#issuecomment-2333669861)
Yeah, possibly. I wonder why there is a presumed increase in those errors recently.
💬 maflcko commented on pull request "ci: Use clang-19 in msan tasks":
(https://github.com/bitcoin/bitcoin/pull/30639#issuecomment-2333672420)
I'd say to keep testing minimal my preference would be to just touch it once.
(The aarch64 build has to be done manually every time)
(https://github.com/bitcoin/bitcoin/pull/30639#issuecomment-2333672420)
I'd say to keep testing minimal my preference would be to just touch it once.
(The aarch64 build has to be done manually every time)
💬 maflcko commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2333681810)
Would be nice to change the title from "... transactions v2" to something else. Otherwise, it may be a bit confusing in the context of "v2 transactions" (transactions with version number set to `2`, not `3`), or "v2 transport" (the P2P thing).
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2333681810)
Would be nice to change the title from "... transactions v2" to something else. Otherwise, it may be a bit confusing in the context of "v2 transactions" (transactions with version number set to `2`, not `3`), or "v2 transport" (the P2P thing).
💬 fanquake commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2333693862)
Fixing this looks good. I guess going forward it currently has the caveat that it's expected to work most correctly in the CI (on the same hardware used by the CI). i.e this it fail for any dev trying to run the script locally, on aarch64:
```bash
[ 25%] Built target bitcoin_crypto_arm_shani
[100%] Built target bitcoin_crypto
gmake: *** No rule to make target 'bitcoin_crypto_x86_shani'. Stop.
```
or if a dev compiles on x86_64, but doesn't match the CI config, they may see incorrect outpu
...
(https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2333693862)
Fixing this looks good. I guess going forward it currently has the caveat that it's expected to work most correctly in the CI (on the same hardware used by the CI). i.e this it fail for any dev trying to run the script locally, on aarch64:
```bash
[ 25%] Built target bitcoin_crypto_arm_shani
[100%] Built target bitcoin_crypto
gmake: *** No rule to make target 'bitcoin_crypto_x86_shani'. Stop.
```
or if a dev compiles on x86_64, but doesn't match the CI config, they may see incorrect outpu
...
🚀 fanquake merged a pull request: "contrib: fix check-deps.sh to check for weak symbols"
(https://github.com/bitcoin/bitcoin/pull/30415)
(https://github.com/bitcoin/bitcoin/pull/30415)
👍 stickies-v approved a pull request: "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage"
(https://github.com/bitcoin/bitcoin/pull/30618#pullrequestreview-2285610478)
ACK 3437d4e1bdec7963d3b49bcc396838602a5b00fc
> Do you think it's important in this case?
No it's fine as-is too, it's just that we start from a string so producing another, seemingly similar one might be unintuitive to someone less familiar with the code. Could add something like this, but no strong view
<details>
<summary>git diff on 3437d4e1bd</summary>
```diff
diff --git a/src/test/fuzz/hex.cpp b/src/test/fuzz/hex.cpp
index 6a85fd9fdb..6a4a381588 100644
--- a/src/test/fuzz/hex
...
(https://github.com/bitcoin/bitcoin/pull/30618#pullrequestreview-2285610478)
ACK 3437d4e1bdec7963d3b49bcc396838602a5b00fc
> Do you think it's important in this case?
No it's fine as-is too, it's just that we start from a string so producing another, seemingly similar one might be unintuitive to someone less familiar with the code. Could add something like this, but no strong view
<details>
<summary>git diff on 3437d4e1bd</summary>
```diff
diff --git a/src/test/fuzz/hex.cpp b/src/test/fuzz/hex.cpp
index 6a85fd9fdb..6a4a381588 100644
--- a/src/test/fuzz/hex
...
💬 stickies-v commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1746798164)
Dereferencing could be UB when `FromHex()` is buggy, so for robustness would use `.value()` here.
```suggestion
BOOST_CHECK_EQUAL(get_valid_opts({("-minimumchainwork=" + minimum_chainwork).c_str()}).minimum_chain_work, UintToArith256(uint256::FromHex(minimum_chainwork).value()));
```
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1746798164)
Dereferencing could be UB when `FromHex()` is buggy, so for robustness would use `.value()` here.
```suggestion
BOOST_CHECK_EQUAL(get_valid_opts({("-minimumchainwork=" + minimum_chainwork).c_str()}).minimum_chain_work, UintToArith256(uint256::FromHex(minimum_chainwork).value()));
```
💬 stickies-v commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1746799817)
> Let's see what you think of my latest push:
Yeah I like the current state of it, thanks for addressing this and can be marked as resolved now!
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1746799817)
> Let's see what you think of my latest push:
Yeah I like the current state of it, thanks for addressing this and can be marked as resolved now!
💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333713098)
Actually, according to `git grep --extended-regexp '%[0-9]\$' -- '*.cpp' '*.h'` positional args are used. Let me fix that up.
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333713098)
Actually, according to `git grep --extended-regexp '%[0-9]\$' -- '*.cpp' '*.h'` positional args are used. Let me fix that up.
💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2333731246)
Guix Build (aarch64):
```bash
f21ba8dde52ecf7fd1e76b5f6efe7d620a7a0baf8099eb6d25d09161a73f2030 guix-build-6c9000cfbfab/output/aarch64-linux-gnu/SHA256SUMS.part
5e89a83e025bfbc37118a1ca0d386b0ffbde22bd3357b06d7e4c3587e10b0ed6 guix-build-6c9000cfbfab/output/aarch64-linux-gnu/bitcoin-6c9000cfbfab-aarch64-linux-gnu-debug.tar.gz
d0b7acb03204f077bc88d0d9008041c1aa854a6e0fd244210099ae2f2b8b4ca3 guix-build-6c9000cfbfab/output/aarch64-linux-gnu/bitcoin-6c9000cfbfab-aarch64-linux-gnu.tar.gz
e46198
...
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2333731246)
Guix Build (aarch64):
```bash
f21ba8dde52ecf7fd1e76b5f6efe7d620a7a0baf8099eb6d25d09161a73f2030 guix-build-6c9000cfbfab/output/aarch64-linux-gnu/SHA256SUMS.part
5e89a83e025bfbc37118a1ca0d386b0ffbde22bd3357b06d7e4c3587e10b0ed6 guix-build-6c9000cfbfab/output/aarch64-linux-gnu/bitcoin-6c9000cfbfab-aarch64-linux-gnu-debug.tar.gz
d0b7acb03204f077bc88d0d9008041c1aa854a6e0fd244210099ae2f2b8b4ca3 guix-build-6c9000cfbfab/output/aarch64-linux-gnu/bitcoin-6c9000cfbfab-aarch64-linux-gnu.tar.gz
e46198
...
💬 l0rinc commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1746891613)
Thanks, fixed
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1746891613)
Thanks, fixed
💬 l0rinc commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#issuecomment-2333742666)
> Could add something like this, but no strong view
Done, thanks!
(https://github.com/bitcoin/bitcoin/pull/30618#issuecomment-2333742666)
> Could add something like this, but no strong view
Done, thanks!
💬 hebasto commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746893985)
`WITH_NATPMP` is `OFF` because the `libnatpmp` package is not available in [vcpkg](https://vcpkg.io/en/packages).
`WITH_QRENCODE` is `OFF` because I faced build errors for this [package](https://vcpkg.io/en/package/libqrencode) or its dependencies in the past (I can't recall the exact issue). I've tested it recently and I got another build error, which is weird, because it happens in the main build system:
```
Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE)
```
It needs to be
...
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1746893985)
`WITH_NATPMP` is `OFF` because the `libnatpmp` package is not available in [vcpkg](https://vcpkg.io/en/packages).
`WITH_QRENCODE` is `OFF` because I faced build errors for this [package](https://vcpkg.io/en/package/libqrencode) or its dependencies in the past (I can't recall the exact issue). I've tested it recently and I got another build error, which is weird, because it happens in the main build system:
```
Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE)
```
It needs to be
...