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

💬 fanquake commented on issue "Newer libevent causes `http_request` fuzz target failure":
(https://github.com/bitcoin/bitcoin/issues/30096#issuecomment-2110029212)
Ok. Edited my comment as well. This can still be closed. Maybe an issue can be filed with vcpkg to only ship stable code in production.

👍 sdaftuar approved a pull request: "test: add conflicting topology test case"
(https://github.com/bitcoin/bitcoin/pull/30066#pullrequestreview-2055185685)
utACK, one style preference/nit for you to consider

💬 sdaftuar commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#discussion_r1599922463)
This may be just a matter of personal preference, so feel free to take it or leave it -- but if I were writing this I'd drop this line which tests the reason for failure, and also drop line 269 which tests that RBF doesn't work, in favor of just having the assertion at line 270 below that the child doesn't make it in. That way, someone changing the behavior in the future is less likely to rip the whole test out (eg if "not child with unconfirmed parents" stops being a rejection reason, or if we
...

💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1599947252)
Nice, will add if I end up needing to retouch!

💬 hebasto commented on issue "Newer libevent causes `http_request` fuzz target failure":
(https://github.com/bitcoin/bitcoin/issues/30096#issuecomment-2110112892)
> Ok, then I think this can be closed. The problem here primarily seems to be that Microsoft/vcpkg is shipping unreleased code into production.

I agree. I opened this issue to document https://github.com/hebasto/bitcoin/pull/199.

Closing.

✅ hebasto closed an issue: "Newer libevent causes `http_request` fuzz target failure"
(https://github.com/bitcoin/bitcoin/issues/30096)

💬 instagibbs commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#discussion_r1599957582)
I'm a little conflicted since without the string checking it's a little harder to tell what the two cases being checked are there? I do agree that dropping the check for the parent RBF is better.

I pushed a small update to make it a bit harder to have the whole case ripped out by someone reading the test, let me know what you think.

💬 vasild commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1599959087)
This was somewhat answered in https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1465152180 above but let me elaborate - the thing that prompted me to do this change is this text from the `fread(3)` manual page:

> The function fread() does not distinguish between end-of-file and error,
> and callers must use feof(3) and ferror(3) to determine which occurred.

I think checking for `ferror(3)` after `fread(3)` is a kind of harmless "belt-and-suspenders". If you are sure that if `detai
...

💬 instagibbs commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#discussion_r1599959394)
done

💬 willcl-ark commented on pull request "rpc: move UniValue in blockToJSON":
(https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2110153539)
@theuni FWIW I do see some _increased_ RSS measurements on your current branch vs master (with my one, single test using listtransactions rpc on a large wallet):


This is also what I saw on my linked "univalue-move" branch above. I didn't investigate why exactly this happens, yet. It seems like `move` might not always be advantageous for us...

💬 vasild commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1599981699)
My understanding is that it is safe to use `thread_local` on FreeBSD for variables that do not have a destructor even in the presence of the above bug (or bugs if they are two separate bugs).

👍 ryanofsky approved a pull request: "kernel: Remove batchpriority from kernel library"
(https://github.com/bitcoin/bitcoin/pull/30083#pullrequestreview-2055370796)
Code review ACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d, just added suggested comment since last review