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

💬 instagibbs commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464835996)
if this is a bit of overcounting, that's probably ok?

💬 fanquake commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464840653)
Is the breakage not an actual issue: https://cirrus-ci.com/task/6124189116006400?logs=ci#L3274
```bash
test_framework.authproxy.JSONRPCException: Only 4999872180 coins to cover target value of 5100000840 (-4)
```

💬 instagibbs commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464842951)
I can turn the error on and off locally by adding/removing the string from the error.

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1464847173)
This is on purpose (note the lower case `t`) to fix a documentation typo. I could have added a regex for doing it in one line, but that seems even less readable.

💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-1908048127)
Rebased to include https://github.com/bitcoin/bitcoin/pull/29272

💬 fanquake commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464860895)
The wallet code seems to be the only place where this pattern of `util::Error()` is used. Everywhere actual error strings are always provided.

💬 Ahmatmufid commented on pull request "doc: FreeBSD build doc updates to reflect removal of install_db4.sh":
(https://github.com/bitcoin/bitcoin/pull/26773#issuecomment-1908096016)
Oke"👍👍👍👍