💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1753929285)
I think this test should have a comment that the current behaviour is because our counting implementation is incorrect, not because it's desired behaviour. Otherwise this could be confusing to developers improving it in the future.

Or even better, add a separate group of tests with behaviour we know would be an improvement, but is not currently implemented:

```cpp
// The current implementation ignores `*` dynamic width/precision
// arguments to minimize code complexity, b
...

💬 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