👍 hebasto approved a pull request: "refactor: simplify `FormatSubVersion` using strprintf/Join"
(https://github.com/bitcoin/bitcoin/pull/30098#pullrequestreview-2054844843)
ACK 12d82817bf32396b58c8c65645012def606680b6, I have reviewed the code and it looks OK.
(https://github.com/bitcoin/bitcoin/pull/30098#pullrequestreview-2054844843)
ACK 12d82817bf32396b58c8c65645012def606680b6, I have reviewed the code and it looks OK.
💬 glozow commented on pull request "sign: don't assume we are parsing a sane TapMiniscript":
(https://github.com/bitcoin/bitcoin/pull/29853#issuecomment-2109752682)
backported to 26.x in #29899
(https://github.com/bitcoin/bitcoin/pull/29853#issuecomment-2109752682)
backported to 26.x in #29899
💬 hebasto commented on issue "windows: Newer libevent causes `http_request` fuzz target failure":
(https://github.com/bitcoin/bitcoin/issues/30096#issuecomment-2109759186)
> Is there something for this repo to do here? If that patch should be applied, then this should be a PR? If not, I guess close this?
I'm in the middle of the investigation.
(https://github.com/bitcoin/bitcoin/issues/30096#issuecomment-2109759186)
> Is there something for this repo to do here? If that patch should be applied, then this should be a PR? If not, I guess close this?
I'm in the middle of the investigation.
💬 vasild commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2109763258)
`13f438a667...c5f9afd946`: fix the above and return `std::string` instead of `std::string_view` which internally has a pointer to the `thread_local` variable. This way the API can't the misused to store the reference to the `thread_local` longer than the thread's lifespan.
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2109763258)
`13f438a667...c5f9afd946`: fix the above and return `std::string` instead of `std::string_view` which internally has a pointer to the `thread_local` variable. This way the API can't the misused to store the reference to the `thread_local` longer than the thread's lifespan.
👋 vasild's pull request is ready for review: "util: check for errors after close and read in AutoFile"
(https://github.com/bitcoin/bitcoin/pull/29307)
(https://github.com/bitcoin/bitcoin/pull/29307)
💬 vasild commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2109769504)
Fixed the CI!
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2109769504)
Fixed the CI!
💬 hebasto commented on pull request "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP":
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2109785175)
@maflcko Would you mind having a look into this, please?
People who works on CMake migration are highly anticipating your opinion on use cases for [`APPEND_{CPP,C,CXX,LD}FLAGS`](https://github.com/hebasto/bitcoin/pull/184) in the CI scripts.
cc @theuni
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2109785175)
@maflcko Would you mind having a look into this, please?
People who works on CMake migration are highly anticipating your opinion on use cases for [`APPEND_{CPP,C,CXX,LD}FLAGS`](https://github.com/hebasto/bitcoin/pull/184) in the CI scripts.
cc @theuni
👍 hebasto approved a pull request: "[27.x] Backports and probably finalize"
(https://github.com/bitcoin/bitcoin/pull/30092#pullrequestreview-2054913602)
ACK 9867e72b995e4a838c8e8e0feb159a9bdee8b76f, I have reproduced backporting locally.
(https://github.com/bitcoin/bitcoin/pull/30092#pullrequestreview-2054913602)
ACK 9867e72b995e4a838c8e8e0feb159a9bdee8b76f, I have reproduced backporting locally.
💬 maflcko commented on pull request "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP":
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2109858218)
Personally I like the append approach, because it specifies that everything is the "default" plus the given flags appended. Otherwise there would be ambiguity and hard-to-debug issues which default flags are omitted (or kept) by setting CXXFLAGS. (c.f. https://github.com/bitcoin/bitcoin/pull/29205/files#diff-8b9cb286f7af766e7d4615428ff76c84947644ed42032c54e97e0aa4aeee751e)
Other than that, I think the CI system should not receive more thought. I think the primary concern should be that it is
...
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2109858218)
Personally I like the append approach, because it specifies that everything is the "default" plus the given flags appended. Otherwise there would be ambiguity and hard-to-debug issues which default flags are omitted (or kept) by setting CXXFLAGS. (c.f. https://github.com/bitcoin/bitcoin/pull/29205/files#diff-8b9cb286f7af766e7d4615428ff76c84947644ed42032c54e97e0aa4aeee751e)
Other than that, I think the CI system should not receive more thought. I think the primary concern should be that it is
...
💬 willcl-ark commented on pull request "rpc: move UniValue in blockToJSON":
(https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2109869098)
> I was already working on this and... wow. You've opened a big can of worms here @willcl-ark :). But that's a good thing because it should reduce memusage and increase performance I'd assume.
>
> See here for my first pass. It's a beast. [theuni/bitcoin@`less-univalue-copies` (commits)](https://github.com/theuni/bitcoin/commits/less-univalue-copies/)
Wow! This looks like a great start! Will begin to review...
> Edit: added 2 more commits to avoid copying strings. @willcl-ark let me kno
...
(https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2109869098)
> I was already working on this and... wow. You've opened a big can of worms here @willcl-ark :). But that's a good thing because it should reduce memusage and increase performance I'd assume.
>
> See here for my first pass. It's a beast. [theuni/bitcoin@`less-univalue-copies` (commits)](https://github.com/theuni/bitcoin/commits/less-univalue-copies/)
Wow! This looks like a great start! Will begin to review...
> Edit: added 2 more commits to avoid copying strings. @willcl-ark let me kno
...
💬 maflcko commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1599815167)
de23848eed5437f5e4dc6ccdc38b40cc58738ead: I still don't understand this. Why would fread succeed to read the requested number of bytes, but then mark the stream with an error? If the goal is to somehow detect errors after the read succeeded, I am not sure this should be a goal. Otherwise, it would be good to explain what real user-facing bug this fixes or introduces.
Either this is a refactor, in which case it seems pointless, or it is a behavior change, in which case it needs to explain why
...
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1599815167)
de23848eed5437f5e4dc6ccdc38b40cc58738ead: I still don't understand this. Why would fread succeed to read the requested number of bytes, but then mark the stream with an error? If the goal is to somehow detect errors after the read succeeded, I am not sure this should be a goal. Otherwise, it would be good to explain what real user-facing bug this fixes or introduces.
Either this is a refactor, in which case it seems pointless, or it is a behavior change, in which case it needs to explain why
...
💬 willcl-ark commented on issue "contrib/signet/miner: miner won't consider --nbits parameter":
(https://github.com/bitcoin/bitcoin/issues/30091#issuecomment-2109905916)
@edilmedeiros I don't think there's anything to do here; `contrib/signet/miner` works as expected. Would you agree this issue can be closed now?
If you have some code changes I'd be happy to review them in a future pull request.
(https://github.com/bitcoin/bitcoin/issues/30091#issuecomment-2109905916)
@edilmedeiros I don't think there's anything to do here; `contrib/signet/miner` works as expected. Would you agree this issue can be closed now?
If you have some code changes I'd be happy to review them in a future pull request.
✅ edilmedeiros closed an issue: "contrib/signet/miner: miner won't consider --nbits parameter"
(https://github.com/bitcoin/bitcoin/issues/30091)
(https://github.com/bitcoin/bitcoin/issues/30091)
💬 fanquake commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1599831075)
This is being removed, but from the discussion in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701, [I see](https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701#c3)
> I started investigating this from here:
https://groups.google.com/g/bsdmailinglist/c/22ncTZAbDp4/m/Dii_pII5AwAJ
> I am not sure if that comment has merit. If that is an issue, it is different than the one from this bug report.
So if it's a different issue, unrelated to the one just fixed, or it's still unclear, wh
...
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1599831075)
This is being removed, but from the discussion in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701, [I see](https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701#c3)
> I started investigating this from here:
https://groups.google.com/g/bsdmailinglist/c/22ncTZAbDp4/m/Dii_pII5AwAJ
> I am not sure if that comment has merit. If that is an issue, it is different than the one from this bug report.
So if it's a different issue, unrelated to the one just fixed, or it's still unclear, wh
...
💬 maflcko commented on pull request "refactor: simplify `FormatSubVersion` using strprintf/Join":
(https://github.com/bitcoin/bitcoin/pull/30098#discussion_r1599833349)
why is the `if` needed at all? Can't this whole function be a one-liner?
```cpp
return strprintf("/%s:%s%s/", name, FormatVersion(nClientVersion), Join(comments, "; ");
(https://github.com/bitcoin/bitcoin/pull/30098#discussion_r1599833349)
why is the `if` needed at all? Can't this whole function be a one-liner?
```cpp
return strprintf("/%s:%s%s/", name, FormatVersion(nClientVersion), Join(comments, "; ");
💬 maflcko commented on pull request "kernel: Remove batchpriority from kernel library":
(https://github.com/bitcoin/bitcoin/pull/30083#issuecomment-2109943681)
ACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d 📭
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK d4b17c7d46ad8e2833ade99d
...
(https://github.com/bitcoin/bitcoin/pull/30083#issuecomment-2109943681)
ACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d 📭
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK d4b17c7d46ad8e2833ade99d
...
💬 theStack commented on pull request "refactor: simplify `FormatSubVersion` using strprintf/Join":
(https://github.com/bitcoin/bitcoin/pull/30098#discussion_r1599849410)
> why is the `if` needed at all? Can't this whole function be a one-liner?
>
> ```c++
> return strprintf("/%s:%s%s/", name, FormatVersion(nClientVersion), Join(comments, "; ");
> ```
Using this expression would miss the parentheses around the comments; they should only be there if there are comments at all. The alternative is using a single return expression and the ternary operator, but I found this slightly less readable.
(https://github.com/bitcoin/bitcoin/pull/30098#discussion_r1599849410)
> why is the `if` needed at all? Can't this whole function be a one-liner?
>
> ```c++
> return strprintf("/%s:%s%s/", name, FormatVersion(nClientVersion), Join(comments, "; ");
> ```
Using this expression would miss the parentheses around the comments; they should only be there if there are comments at all. The alternative is using a single return expression and the ternary operator, but I found this slightly less readable.
💬 paplorinc commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#issuecomment-2109949821)
re-ACK 9408a04e424cee0d226bde79171bd4954f9caeb0
(https://github.com/bitcoin/bitcoin/pull/30048#issuecomment-2109949821)
re-ACK 9408a04e424cee0d226bde79171bd4954f9caeb0
💬 maflcko commented on pull request "refactor: simplify `FormatSubVersion` using strprintf/Join":
(https://github.com/bitcoin/bitcoin/pull/30098#issuecomment-2109955724)
utACK 12d82817bf32396b58c8c65645012def606680b6
(https://github.com/bitcoin/bitcoin/pull/30098#issuecomment-2109955724)
utACK 12d82817bf32396b58c8c65645012def606680b6
💬 hebasto commented on issue "windows: Newer libevent causes `http_request` fuzz target failure":
(https://github.com/bitcoin/bitcoin/issues/30096#issuecomment-2109963276)
> > Is there something for this repo to do here? If that patch should be applied, then this should be a PR? If not, I guess close this?
>
> I'm in the middle of the investigation.
The crash is reproducible on Linux as well.
(https://github.com/bitcoin/bitcoin/issues/30096#issuecomment-2109963276)
> > Is there something for this repo to do here? If that patch should be applied, then this should be a PR? If not, I guess close this?
>
> I'm in the middle of the investigation.
The crash is reproducible on Linux as well.