👍 vasild approved a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1840919344)
ACK 27f260aa6e04f82dad78e9a06d58927546143a27

Wrt the interface addition discussed in https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1285990092, I think it is not perfect but I do not see it as a blocker for this PR. Would be nice to see the discussion resolved in one way or another.

💬 vasild commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1464705680)
> > It does not check whether ferror(3) occurred.
>
> It does.

Where? There is no `ferror(3)` call. From the man 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."

> If the return value of `detail_fread` is not `output.size()`, `operator>>` will fail.

Yes, but it can be equal to the desired size under two conditions: eof, or error. The previous code distinguished between both.

📝 dergoegge opened a pull request: "fuzz: Exit and log stdout for parse_test_list errors"
(https://github.com/bitcoin/bitcoin/pull/29304)
We should log all errors that occur when attempting to print the harness list in the fuzz test runner.

💬 dergoegge commented on pull request "fuzz: Exit and log stdout for parse_test_list errors":
(https://github.com/bitcoin/bitcoin/pull/29304#issuecomment-1907892490)
This is currently a problem in the qa-assets CI, listing the harnesses fails but there is no output: https://cirrus-ci.com/task/5433841842651136?logs=ci#L9235

```
...
+ test/fuzz/test_runner.py -j6 -l DEBUG /ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/ --empty_min_time=60
No fuzz targets found
```

💬 maflcko commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1464749909)
> > If the return value of `detail_fread` is not `output.size()`, `operator>>` will fail.
>
> Yes, but it can be equal to the desired size under two conditions: eof, or error.

No, the eof-error would only be raised if read *past* the desired size, not *to* it. Unless I am missing something?

I am asking, because if there was a bug, it should be fixed, or at least an issue should be filed.

💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1464751330)
An alternative to the mocking would be to use [ `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`](https://llvm.org/docs/LibFuzzer.html#fuzzer-friendly-build-mode) in all places that call `CheckProofOfWork` (i.e. validation, mining) since there is literally zero benefit in having this be a blocker for fuzzing.

💬 maflcko commented on pull request "fuzz: Exit and log stdout for parse_test_list errors":
(https://github.com/bitcoin/bitcoin/pull/29304#discussion_r1464756961)
This will now include stderr, because it was redirected to stdout, no? Seems easier to keep them separate?

💬 maflcko commented on pull request "fuzz: Exit and log stdout for parse_test_list errors":
(https://github.com/bitcoin/bitcoin/pull/29304#discussion_r1464754687)
Why redirect it to stdout, then read stdout, and then print it to stderr? Wouldn't it be easier to just remove this line and thus inherit stderr?

💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1464771476)
https://github.com/dergoegge/bitcoin/commit/48f82613eb5079f4bca4bedad32077cf534a0a92

💬 vasild commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1464774469)
That "must" in the man page is pretty clear: "callers must use feof(3) and ferror(3) to determine which occurred". A `ferror(3)` check can't hurt and it is better to have an extra check that always returns "no error" than a missing check, failing to detect an IO error. The previous code was doing that - a dumb `fread(3)` followed by an unconditional `ferror(3)`.

💬 dergoegge commented on pull request "fuzz: Exit and log stdout for parse_test_list errors":
(https://github.com/bitcoin/bitcoin/pull/29304#discussion_r1464775423)
Inheriting is better, done!

💬 maflcko commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1464781909)
Yes, it is called to determine which error occurred, see https://github.com/bitcoin/bitcoin/blob/e69796c79c0aa202087a13ba62d9fbcc1c8754d4/src/streams.cpp#L27

Again, if there is a bug in the current code in master, it should be fixed.

💬 maflcko commented on pull request "fuzz: Exit and log stderr for parse_test_list errors":
(https://github.com/bitcoin/bitcoin/pull/29304#discussion_r1464788863)
This can also be removed, by simply using `check=True,`, to get the same details, and even a traceback, no?

💬 dergoegge commented on pull request "fuzz: Exit and log stderr for parse_test_list errors":
(https://github.com/bitcoin/bitcoin/pull/29304#discussion_r1464794918)
great, done!

💬 maflcko commented on pull request "fuzz: Exit and log stderr for parse_test_list errors":
(https://github.com/bitcoin/bitcoin/pull/29304#issuecomment-1907966782)
lgtm ACK 9d09c873a50d344e2a9cb35fe246a52688b9fa34

simple +1-1 change

💬 maflcko commented on pull request "fuzz: Exit and log stderr for parse_test_list errors":
(https://github.com/bitcoin/bitcoin/pull/29304#discussion_r1464799071)
For reference, this check is fine to do since fa22966f330 changed the abort into a successful exit.

💬 glozow commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1464806756)
Seems we are missing test coverage for this. Maybe add one to rpc_packages.py?

💬 glozow commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1464807756)
Also, why delete the empty vector case? That should throw a JSONRPCError too, no?

💬 fjahr commented on pull request "contrib: add test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1907998137)
Code looks good. I have been testing this with the `asmap.dat` file from https://github.com/fjahr/asmap-data/pull/6 and I noticed that after adding 60-70 entries the program slows down a lot and sometimes takes over a minute to add the next address. Have you seen this as well and do you know why this is @brunoerg ?

💬 vasild commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1464821741)
I would extend the check to:

```diff
void AutoFile::read(Span<std::byte> dst)
{
- if (detail_fread(dst) != dst.size()) {
+ if (detail_fread(dst) != dst.size() || std::ferror(m_file)) {
throw std::ios_base::failure(feof() ? "AutoFile::read: end of file" : "AutoFile::read: fread failed");
```