🤔 l0rinc reviewed a pull request: "contrib: turn off compression of macOS SDK to fix determinism (across distros)"
(https://github.com/bitcoin/bitcoin/pull/32009#pullrequestreview-3503804718)
I don't yet have experience with this part of the code, left a few nits.
I arrived here while investigating an upgrade to Xcode 16.3 in https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2550717279 and
noticed a couple of potential issues around the `argparse` usage and the `-o`
handling.

I have tested the following changes locally:

```patch
diff --git a/contrib/macdeploy/gen-sdk.py b/contrib/macdeploy/gen-sdk.py
index 426d82e46c..3bf9154887 100755
--- a/contrib/macdeploy/gen-sdk.py
++
...

💬 l0rinc commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#discussion_r2559089068)
are these file extensions? I wasn't familiar with them, but e.g. https://medium.com/@mail2ashislaha/swift-objective-c-interoperability-static-libraries-modulemap-etc-39caa77ce1fc indicates they're extensions, in which case:

```suggestion
if tarinfo.name and tarinfo.name.endswith((".swiftmodule", ".modulemap")):
```

💬 l0rinc commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#discussion_r2559130969)
are we reassigning `out_sdkt_path` here? Based on the condition checking `args.out_sdkt`, shouldn't this rather be:
```suggestion
if args.out_sdkt:
out_sdkt_path = pathlib.Path(args.out_sdkt[0])
```

or maybe if we remove `nargs=1` from above we could do:
```suggestion
if args.out_sdkt:
out_sdkt_path = pathlib.Path(args.out_sdkt)
```

Was this working before? I don't really have experience with this, so I can't tell...

💬 l0rinc commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#discussion_r2558987253)
should we adjust the help description as well?
```suggestion
parser.add_argument("-o", metavar='OUTSDKTAR', nargs=1, dest='out_sdkt', required=False)
```

> usage: gen-sdk.py [-h] [-o OUTSDKTAR] XCODEAPP

👍 willcl-ark approved a pull request: "depends: sqlite 3.50.4; switch to autosetup"
(https://github.com/bitcoin/bitcoin/pull/32655#pullrequestreview-3504114386)
tACK 1db74914706fcfafb22646288458604a4a7b6282

This looks good to me. Tested a few random wallet operations in addition to the tests, which all seem ok.

Noting that whilst `--disable-dynamic-extensions` was dropped from configure options it's added as a preprocessor directive directly (although I think it could have been re-added to configure options as `--disable-load-extension`, but both are equivalent).

🤔 l0rinc reviewed a pull request: "RFC: bench: Add multi thread benchmarking"
(https://github.com/bitcoin/bitcoin/pull/33740#pullrequestreview-3504147480)
Not sure whether the concept of script validation has to creep into the benchmarking parameters, maybe we could generalize it to `-thread-count` or `-par` or something. Left a few nits.

💬 l0rinc commented on pull request "RFC: bench: Add multi thread benchmarking":
(https://github.com/bitcoin/bitcoin/pull/33740#discussion_r2559279775)
Script checks are currently the main source of threading, but not necessarily limited to this (e.g. https://github.com/bitcoin/bitcoin/pull/31132/commits or https://github.com/bitcoin/bitcoin/pull/26966 or https://github.com/bitcoin/bitcoin/pull/33689 or https://github.com/bitcoin/bitcoin/pull/32747), so I would suggest untangling multithreading from script validation here. If you insist this being about script validation (which sounds a bit too specific to include in a general benchmark), ignor
...

💬 l0rinc commented on pull request "RFC: bench: Add multi thread benchmarking":
(https://github.com/bitcoin/bitcoin/pull/33740#discussion_r2559261336)
hmmm, this looks like a refactor gone wrong, was this `arg.find("-worker-threads=") == 0` originally?

```suggestion
if (arg.starts_with("-worker-threads=")) {
```

so I guess `if (!found) {` should also be adjusted after this

---

Alternatively, instead of finding and overwriting, what if we unconditionally erased and pushed back the actual thread count, something like:
```C++
std::vector current_setup_args {args.setup_args};
if (threads > 0) {
std::erase_if(current_set
...

💬 l0rinc commented on pull request "RFC: bench: Add multi thread benchmarking":
(https://github.com/bitcoin/bitcoin/pull/33740#discussion_r2559321265)
What's the purpose of a vector over a range? Are we planning on testing threads `2,4,8,16` instead of `1,2,3,4,5,6...`?
If not, consider making it a range, something like:
```C++
constexpr int DEFAULT_MAX_BENCH_THREADS{16};
const auto [start_threads, end_threads]{is_thread_scaling ?
std::pair{1, DEFAULT_MAX_BENCH_THREADS} : std::pair{0, 0}};
```
and maybe iterate as
```C++
for (int threads{start_threads}; threads <= end_threads; ++threads) {
...
}
```

💬 l0rinc commented on pull request "RFC: bench: Add multi thread benchmarking":
(https://github.com/bitcoin/bitcoin/pull/33740#discussion_r2559283533)
What should happen when `worker_threads > MAX_SCRIPTCHECK_THREADS`?

💬 l0rinc commented on pull request "RFC: bench: Add multi thread benchmarking":
(https://github.com/bitcoin/bitcoin/pull/33740#discussion_r2559324072)
Might be out of scope for this PR, but instead of inlining `DEFAULT_BENCH_FILTER` here, can we move it over to `src/bench/bench.cpp`

```suggestion
bool is_thread_scaling = args.scale_threads && (args.regex_filter != DEFAULT_BENCH_FILTER);
```

🤔 rkrux reviewed a pull request: "doc: clarify and cleanup macOS fuzzing notes"
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3504404111)
ACK c34bc01b2ff2fc91ed4020288c5fa15f0c5b075e

⚠️ hulxv opened an issue: "RFC: Replacing `tinyformat` with `{fmt}`"
(https://github.com/bitcoin/bitcoin/issues/33942)
# Description

Bitcoin Core has used tinyformat (via its `strprintf` wrapper) for text formatting for many years, a choice that reflected the library’s simplicity and its ability to be dropped into a large C++ codebase with minimal friction. Over time, the C++ standard and the ecosystem around formatting have evolved: `{fmt}` has become the de facto modern formatting library and served as the basis for `std::format` in later C++ standards. At the same time, a security and maintenance review of T
...

💬 rkrux commented on pull request "wallet: don't consider unconfirmed TRUC coins with ancestors":
(https://github.com/bitcoin/bitcoin/pull/33528#discussion_r2559474078)
I see, you might be correct. I hadn't checked the function implementation and assumed this based on the function doc that gave this impression to me.

https://github.com/bitcoin/bitcoin/blob/238c1c8933b1f7479a9bca2b7cb207d26151c39d/src/interfaces/chain.h#L224-L225

https://github.com/bitcoin/bitcoin/blob/238c1c8933b1f7479a9bca2b7cb207d26151c39d/src/txmempool.h#L592-L598

🚀 fanquake merged a pull request: "Cluster mempool"
(https://github.com/bitcoin/bitcoin/pull/33629)

💬 maflcko commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2559477612)
> `[[unlikely]]`

Will this actually help in a large function without a loop, where a single if is annotated?

Generally, I have the impression that `[[likely]]`, and `[[unlikely]]` should not be used. Possibly in rare cases, where there is a really hot loop, and all release compilers agree that the attribute will help.

In other cases, if the attributes are used too liberal, it seems an accidental pessimization is plausible: It is unclear what the meaning of the attributes are, if nested scopes
...

🚀 fanquake merged a pull request: "ci: Use latest Xcode that the minimum macOS version allows"
(https://github.com/bitcoin/bitcoin/pull/33932)

💬 maflcko commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2559487359)
nit: Could use named args consistently, like below? `/*part_offset=*/0, /*part_size=*/std::nullopt);`

💬 l0rinc commented on pull request "[IBD] specialize CheckBlock's input & coinbase checks":
(https://github.com/bitcoin/bitcoin/pull/31682#issuecomment-3574992541)
Rebased after #33629, removed the conflicting [`1783fbe` (#31682)](https://github.com/bitcoin/bitcoin/pull/31682/commits/1783fbe04dd4938300dadf2553a9bb20accf2ccc) for simplicity

🚀 fanquake merged a pull request: "depends: sqlite 3.50.4; switch to autosetup"
(https://github.com/bitcoin/bitcoin/pull/32655)