💬 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
...

💬 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
...

💬 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
...

💬 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.

✅ edilmedeiros closed an issue: "contrib/signet/miner: miner won't consider --nbits parameter"
(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
...

💬 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, "; ");

💬 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
...

💬 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.

💬 paplorinc commented on pull request "crypto: add `NUMS_H` const":
(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

💬 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.

💬 fanquake commented on issue "Newer libevent causes `http_request` fuzz target failure":
(https://github.com/bitcoin/bitcoin/issues/30096#issuecomment-2109973619)
> The crash is reproducible on Linux as well.

Can you post steps to reproduce. What version of libevent.

💬 maflcko commented on issue "Newer libevent causes `http_request` fuzz target failure":
(https://github.com/bitcoin/bitcoin/issues/30096#issuecomment-2109974965)
It passed here: https://cirrus-ci.com/task/5157535865372672?logs=ci#L4223

💬 hebasto commented on issue "Newer libevent causes `http_request` fuzz target failure":
(https://github.com/bitcoin/bitcoin/issues/30096#issuecomment-2110013579)
> > The crash is reproducible on Linux as well.
>
> Can you post steps to reproduce.

Compiling with depends in [this](https://github.com/hebasto/bitcoin/tree/240514-libevent-fuzz.0) branch. Then:
```
FUZZ=http_request ./src/test/fuzz/fuzz /home/hebasto/git/bitcoin/qa-assets/fuzz_seed_corpus/http_request
```

> What version of libevent.

https://github.com/libevent/libevent/commit/4d85d28acdbb83bb60e500e9345bab757b64d6d1

👍 instagibbs approved a pull request: "p2p: index TxOrphanage by wtxid, allow entries with same txid"
(https://github.com/bitcoin/bitcoin/pull/30000#pullrequestreview-2055134670)
ACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22

reviewed via `git range-diff master 16483fee7c6d93722bfb79fce9efbe841ec13d6a 0fb17bf61a40b73a2b81a18e70b3de180c917f22`

💬 fanquake commented on issue "Newer libevent causes `http_request` fuzz target failure":
(https://github.com/bitcoin/bitcoin/issues/30096#issuecomment-2110019250)
Ok, then I think this can be closed. The problem here primarily seems to be that Microsoft/vcpkg is shipping unreleased code into production. As pointed out by Marco above, in any case, it's working fine in this repo when using the very latest upstream code.

👍 theStack approved a pull request: "crypto: add `NUMS_H` const"
(https://github.com/bitcoin/bitcoin/pull/30048#pullrequestreview-2055138857)
Code-review ACK 9408a04e424cee0d226bde79171bd4954f9caeb0

💬 theStack commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1599896066)
nit, as one-liner (feel free to ignore):
```suggestion
XOnlyPubKey H{(HashWriter{} << Span(G_uncompressed)).GetSHA256()};
```

💬 maflcko commented on issue "Newer libevent causes `http_request` fuzz target failure":
(https://github.com/bitcoin/bitcoin/issues/30096#issuecomment-2110022854)
(I deleted my comment, because the fuzz CI config does not use depends, but the `libevent-dev` from Ubuntu)