💬 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"👍👍👍👍

💬 furszy commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464899774)
The empty `util::Error()` is used to retrieve a "no solution found for the provided inputs set". If you specify an error message, then the wallet coin selection procedure treats it as a real error and bubble it up when no other selection algorithm succeeded.

The goal of this distinction is to differentiate between errors that should be bubble up and coin selection algorithms partial solutions. Remember that we are running each coin selection algorithm up to 7 times. Extending the inputs set o
...