💬 ryanofsky commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793960605)
re: https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793488009

> in [aaaa4fb](https://github.com/bitcoin/bitcoin/commit/aaaa4fb20156b4375d92e1eca4acc90a425a1896): seems more straightforward to just extend `ConstevalStringLiteral` instead of creating a new `util::Original`? This would 1) minimize the diff and 2) I find `ConstevalStringLiteral` a much more helpful name than `Original`.

I think this is only a question of naming since `Original` class is replacing `ConstevalStringLit
...

💬 ryanofsky commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793914825)
re: https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793544349

> Thanks, added unrelated change to fix comment.

Thanks, comment makes sense. I knew there was probably some reason for doing it that way but couldn't figure out what it was.

💬 ryanofsky commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793951571)
re: https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793744937

This is a good catch, but it seems better to just drop the template method instead of trying to fix it up and use it. It just seems to add complexity and it doesn't seem like there is another use-case for it.

💬 ryanofsky commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793948085)
> nit: I would prefer templating this on an enum `{Translateable,Untranslateable}` for readability over the non-obvious `<{true,false}>` boolean

Note: with changes in suggested branch it should no longer be necessary for this to be templated.

💬 ryanofsky commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793952960)
re: https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793707636

Would be reasonable to change this but no more change is actually required here with my suggested branch.

💬 ryanofsky commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793944625)
re: https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793642022

Yes I do think that's better and basically made the same change in my branch, though without the str() method since it seems it makes it a little less clear what return type is.

👍 itornaza approved a pull request: "build: have "make test" depend on "make all""
(https://github.com/bitcoin/bitcoin/pull/31015#pullrequestreview-2358061805)
ACK 2957ca9611916efb570d157b9c7a0b188161660d

Followed the testing steps from the description on macOS 15.0.1 with cmake version 3.30.5 from brew.

1. `cmake -S . -B build`
2. `cmake -C build -j24`

3. `touch src/primitives/transaction.cpp`

<details>
<summary>Confirmed the timestamp change on the file compared to the other ones</summary>

```
transaction.cpp -rw-r--r-- 1 ioannis staff 4170 9 Oct 21:42 transaction.cpp
transaction.h -rw-r--r-- 1 ioannis staff 13730 5 Aug
...

👍 theuni approved a pull request: "build: scripted-diff: drop config/ subdir for bitcoin-config.h"
(https://github.com/bitcoin/bitcoin/pull/30937#pullrequestreview-2358193872)
utACK 09b0161c4a2502564f50fa2b3ce9b19d8fc40d8b

👍 TheCharlatan approved a pull request: "build: scripted-diff: drop config/ subdir for bitcoin-config.h"
(https://github.com/bitcoin/bitcoin/pull/30937#pullrequestreview-2358210092)
ACK 09b0161c4a2502564f50fa2b3ce9b19d8fc40d8b

💬 theuni commented on issue "Disallow building fuzz binary without `-DBUILD_FOR_FUZZING`":
(https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2403337504)
FWIW, I build same as @ryanofsky in order to know that I haven't broken anything.

@dergoegge We could use `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` to avoid registering _any_ targets (rather than attempting to being selective like in #31028), and just exit(0) with a helpful message.

Or we could wrap it in a static lib and only produce a binary if `BUILD_FOR_FUZZING`.

💬 theuni commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2403348003)
If we're going to be peppering the code with these, could we add a safety harness to make sure it's never being used accidentally for a live node?

Maybe somewhere in init.cpp or bitcoind.cpp:
```c++
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
exit(1);
#endif
```

💬 ryanofsky commented on issue "Disallow building fuzz binary without `-DBUILD_FOR_FUZZING`":
(https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2403389988)
I think I'm not clear on what are the drawbacks of the suggestion in https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2400559220. It seems like that would fix the issues, simplify the build, make meaning of the cmake options clearer.

I also like cory's suggestion of building fuzzing code just as a static library instead of as an executable if it really is not useful for any runtime testing.

> Perhaps there is a third approach that still allows to check for build breakage whil
...

💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2403429581)
> If we're going to be peppering the code with these, could we add a safety harness to make sure it's never being used accidentally for a live node?

Seems good. Note that we're already use `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` to bypass `CheckProofOfWork`.

💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2403433476)
Concept ACK

📝 TheCharlatan opened a pull request: "init: Correct coins db cache size setting"
(https://github.com/bitcoin/bitcoin/pull/31064)
The chainstate caches are currently re-balanced on startup even in the non-assumeutxo case, leading to the database being needlessly re-opened and its cache re-allocated.

Similar to `InitCoinsCache` and `m_coinstip_cache_size_bytes`, the `m_coinsdb_cache_size_bytes` should be set in `InitCoinsDB`.

Together with only conservatively setting the cache values when a assumeutxo chainstate is present, this allows for skipping the cache re-balance during initialization in the normal non-assumeutx
...

💬 gissellestarch commented on something "":
(https://github.com/bitcoin/bitcoin/commit/9bd168bf5457c6fd9770769547d8757bf14813b0#r147777706)
gisselle :)

👍 tdb3 approved a pull request: "doc: cmake: prepend "build" to functional/test_runner.py"
(https://github.com/bitcoin/bitcoin/pull/30859#pullrequestreview-2358455238)
ACK c15ef096ce317ae911dd756593adeac9d9a0b5a1

🤔 pablomartin4btc reviewed a pull request: "wallet: optimize migration process, batch db transactions"
(https://github.com/bitcoin/bitcoin/pull/28574#pullrequestreview-2358325707)
ACK a4e1ba722be5dbd71a3d21e5da82aee5409d22bd

<details>
<summary>I've ran the benchmarks locally on SSD and on an external USB 3.2 pendrive and got an improvement of ~46% (i7-1260P, 32GB RAM, Ubuntu 22.04 - using <code>pyperf system tune</code> to get stable results).</summary>

- This PR:

```
./build/src/bench/bench_bitcoin -filter=WalletMigration

| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|---
...

💬 pablomartin4btc commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1794235018)
I ran a bench as well calling `GetPubKey()` only once instead of twice as is currently and didn't gain much out of it, code-wise it could be update it for clarity and practicality anyways but I understand this is not part of the code change and moreover if this is going to be entirely replaced by a refactoring on a follow-up.

💬 jonatack commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1794372847)
After [this push](https://github.com/bitcoin/bitcoin/compare/96f4820f1f274ad371aaa37f8247305183e4642c..cd9297c42447f41b95eea6794535a08341f479dd#diff-ec54487d37183db6e158a11eb3560af15758aba1bd7e253c6b3a58ad3ad16e39R13), ISTM that either the following change is now inadvertently missing, or the PR needs a new title.

```diff
-Running `test/functional/test_runner.py` with the `--coverage` argument tracks which RPCs are
+Running `build/test/functional/test_runner.py` (assuming `build` is your bu
...

🤔 jonatack requested changes to a pull request: "doc: cmake: prepend "build" to functional/test_runner.py"
(https://github.com/bitcoin/bitcoin/pull/30859#pullrequestreview-2358537498)
After [this push](https://github.com/bitcoin/bitcoin/compare/96f4820f1f274ad371aaa37f8247305183e4642c..cd9297c42447f41b95eea6794535a08341f479dd#diff-ec54487d37183db6e158a11eb3560af15758aba1bd7e253c6b3a58ad3ad16e39R13), ISTM that either the following change is now inadvertently missing, or the PR needs a new title and description.

```diff
--- a/test/functional/README.md
+++ b/test/functional/README.md
@@ -10,7 +10,8 @@ that file and modify to fit your needs.

-Running `test/functional/t
...