📝 maflcko opened a pull request: "ci: Print inner env"
(https://github.com/bitcoin/bitcoin/pull/30869)
The outer env is printed when the `/tmp/env-$USER-$CONTAINER_NAME` is created. There is also a separate env printed when building the container image (usually with default values).

To confirm that the inner container env is correctly derived from the outer env, and not from the default build env, print it a third time.

💬 petertodd commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2343346386)
This could probably be done in a separate pull-req. But we can also remove the `bip125-replaceable` code from the wallet at some point. There's quite a bit of complexity there tracking it; I was just looking at `run_rbf_opt_in_test` in `test/functional/wallet_listtransactions.py` myself for Libre Relay.

🤔 pablomartin4btc reviewed a pull request: "rpc: add `revelant_blocks` to `scanblocks status`"
(https://github.com/bitcoin/bitcoin/pull/30713#pullrequestreview-2296520203)
tACK 39ee30c31f5b33a084f420ed49170e669a8f6440

Release notes has been added since my last review.

<details>
<summary>Tested on mainnet.</summary>

```
/build/src/bitcoin-cli scanblocks start '["addr(bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t)"]' 850263
{
"from_height": 850263,
"to_height": 860861,
"relevant_blocks": [
"00000000000000000002b3c2a395905df9d12ce30d2237f064cdafacb81eb33d",
"00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a6090
...

💬 hebasto commented on pull request "build: Use CMake's default permissions in macOS `deploy` target":
(https://github.com/bitcoin/bitcoin/pull/30838#issuecomment-2343387310)
> Concept ACK - looks like switching this to use CMakes default permissions, rather than ours, does fix the issue, but I guess could use more discussion given: [#30815 (comment)](https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2336699084).

Continuation of the discussion:
- https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2341060674
- https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2343184608

💬 fanquake commented on issue "build: reproducibility issue with macOS Guix builds":
(https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2343430934)
> We are already using a tool-level mitigation (for tar). Therefore, using another tool-level mitigation (for CMake) seems consistent.

Why is it necessary though? Aren't we already setting `umask`:
https://github.com/bitcoin/bitcoin/blob/0725a374941355349bb4bc8a79dad1affb27d3b9/contrib/guix/libexec/build.sh#L9-L17

💬 hebasto commented on issue "build: reproducibility issue with macOS Guix builds":
(https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2343439211)
> Why is it necessary though? Aren't we already setting `umask`:

Does it work for directories shared via `--share=...` options?

🤔 stickies-v reviewed a pull request: "util: Use consteval checked format string in FatalErrorf"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2296299754)
Approach ACK and code LGTM faca9a821963d629987f6c2a2f7d13b1bf01162c , left some suggestions that would be nice.

PR description also needs a bit of updating for the 2 new commits re forbidden functions and covering `LogConnectFailure`. The 2 new commits don't seem necessary for this PR but they were trivial to review and relevant to the goal so I'm okay keeping them.

💬 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.