💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754104713)
I think this docstring could be beefed up. To give it a start:

```
/**
* @brief A wrapper for a compile-time partially validated format string
*
* This struct can be used to enforce partial compile-time validation of format strings, to reduce
* the likelihood of tinyformat throwing exceptions at run-time. Validation is partial to try and
* prevent the most common errors while avoiding re-implementing the entire parsing logic.
*
* @note Counting of `*` dynamic width and precisi
...

💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1753962403)
I don't think we need the extra `it_num` here, just seems to add code and iterations?
```suggestion
unsigned maybe_num{0};
while ('0' <= *it && *it <= '9') {
maybe_num *= 10;
maybe_num += *it - '0';
++it;
};

if (*it == '$') {
// Positional specifier
if (maybe_num == 0) throw "Positional format specifier must indicate at least 1";
count_pos =
...

💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1753990981)
phrasing nit
```suggestion
if (maybe_num == 0) throw "Positional format specifier must have position of at least 1";
```

💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1753972269)
This comment is a bit misleading, numbers can also be used for flags and width, e.g. `%02d`.

💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754015054)
I think we can then also remove the `("src/dbwrapper.cpp", "vsnprintf(p, limit - p, format, backup_ap)"),` in `FALSE_POSITIVES` in `run-lint-format-strings.py`?

💬 fjahr commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1752283015)
I guess this would be my preferred way of handling it (untested). But it's a behavior change propagate the reading from disk error to the user.

```suggestion
if (tx->IsCoinBase() || !blockindex || WITH_LOCK(::cs_main, return !(blockindex->nStatus & BLOCK_HAVE_MASK))) {
TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate());
return result;
}

CBlockUndo blockUndo;
CBlock block;
if (!chainman.m_blockman.UndoReadFromDisk(blockUndo, *blockin
...

🤔 fjahr reviewed a pull request: "rpc, rest: Improve block rpc error handling, check header before attempting to read block data."
(https://github.com/bitcoin/bitcoin/pull/30410#pullrequestreview-2293020242)
@mzumsande Sorry, I had written the comment below already earlier but failed to submit it because of github being github. But it seems like it's in line with what you wrote.

💬 tdb3 commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2343545371)
Thanks for reviewing.

> Out of the scope of this PR, having played around `start`, `status`, `abort` subcommands, I'd add some more context in the results of the first 2. Perhaps as "good first issue".
> * for `status` subcommand, instead of returning no results when there are no scans in progress:
> ```
> ./build/src/bitcoin-cli -datadir=${AU_DATADIR} scanblocks status
> {
> "status": "No scan in progress"
> }
> ```
> * for `start` subcommand, when the `abort` has been
...

💬 maflcko commented on pull request "ci: Print inner env":
(https://github.com/bitcoin/bitcoin/pull/30869#issuecomment-2343548630)
Example output: https://cirrus-ci.com/task/6430334932221952?logs=ci#L360

<details><summary>output</summary>

```
SHELL=/bin/bash
CCACHE_TEMPDIR=/tmp/.ccache-temp
CIRRUS_OS=linux
FILE_ENV=./ci/test/00_setup_env_native_tsan.sh
CIRRUS_PR_LABELS=Tests
CCACHE_MAXSIZE=200M
CIRRUS_ARCH=amd64
BASE_OUTDIR=/ci_container_base/ci/scratch/out
RUN_UNIT_TESTS=true
TSAN_OPTIONS=suppressions=/ci_container_base/test/sanitizer_suppressions/tsan:halt_on_error=1
CIRRUS_CHANGE_TIMESTAMP=1726052712753
...

💬 kevkevinpal commented on pull request "build: Fix `ENABLE_WALLET` option":
(https://github.com/bitcoin/bitcoin/pull/30867#issuecomment-2343587823)
ACK [0037d53](https://github.com/bitcoin/bitcoin/pull/30867/commits/0037d53d1a21ec8a5a97a83ab716d68030446021)

I was able to reproduce the steps in the description and it is now working fine

👋 maflcko's pull request is ready for review: "ci: Print inner env, Make ccache config more flexible"
(https://github.com/bitcoin/bitcoin/pull/30869)

💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2343610033)
There are a bunch of places where `CCACHE_MAXSIZE` is hardcoded, making it impossible to set a larger size. This needs to be fixed first, so I did that in https://github.com/bitcoin/bitcoin/pull/30869 (among other stuff).

Assuming .5GB of caches per task, 10 tasks per push, 100 pushes per day, and a cache duration of 20 days, means that 10TB of storage should be sufficient.

👍 ryanofsky approved a pull request: "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage"
(https://github.com/bitcoin/bitcoin/pull/30618#pullrequestreview-2296826968)
Code review ACK 6da47f6ba5450e7f2b2ab2a64c7cfd354f67a1a9. Nice change to make tests easier to read and setup_common code better organized

💬 ryanofsky commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1754374879)
In commit "Use BOOST_CHECK_EQUAL for optional, arith_uint256, uint256, uint160" (57c1f9fb9580456f98c239e1a8aa9161c7014fc6)

In general I like using `auto` to avoid verbosity and repetition when types are obvious. But in this case I think using `std::string` makes code easier to read because `"0x123..."` literals in our codebase are more commonly used to create different types like c strings, arrays, vectors, and blobs. Using `auto` is less clear because it makes `std::string` type a surprise u
...

💬 ryanofsky commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1754442046)
In commit "Compare FromUserHex result against other hex validators and parsers" (6da47f6ba5450e7f2b2ab2a64c7cfd354f67a1a9)

The three lines removed here are not equivalent to the similar lines added below because these are checking `random_hex_string`, while lines below are checking `result->ToString()`, these strings are not the same because `random_hex_string` can have mixed case. Would suggest not dropping these lines.

💬 ryanofsky commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1754400746)
In commit "Use BOOST_CHECK_EQUAL for optional, arith_uint256, uint256, uint160" (57c1f9fb9580456f98c239e1a8aa9161c7014fc6)

Here and below I also think `const std::string` would be more helpful than `auto`

💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2343642089)
@brunoerg @maflcko review ping :)

👍 brunoerg approved a pull request: "fuzz: Test headers pre-sync through p2p"
(https://github.com/bitcoin/bitcoin/pull/30661#pullrequestreview-2296997010)
ACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0

📝 kevkevinpal opened a pull request: "docs: updated developer notes for --with-sanitizers to -DSANITIZERS"
(https://github.com/bitcoin/bitcoin/pull/30870)
In the developer notes we are incorrectly using the Autotools `--with-sanitizers` configure flag which we should now be using `cmake -B build -DSANITIZERS=<values>` instead now

🤔 l0rinc requested changes to a pull request: "util: Use consteval checked format string in FatalErrorf, LogConnectFailure"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2296466018)
Thanks for your patience and for striving to eliminate developer frustration.
I like the concept a lot, but have some preferences regarding parser intuitiveness, hope you'll consider my suggestions.