💬 hodlinator commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1905437524)
nit:
```suggestion
static CBlock CreateTestBlock()
```

💬 hodlinator commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1905481382)
nit: Could add missing newline before EOF.

💬 hodlinator commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1905456335)
nit: Newer code shouldn't need to explicitly log `__func__` as we have `-logsourcelocations`.
```suggestion
LogError("OpenUndoFile failed");
```

💬 l0rinc commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1905586719)
Thanks @hodlinator!
The terminology was requested by @ryanofsky and @TheCharlatan - I don't have strong feelings about them either way. What do other reviewers thing?

💬 vasild commented on pull request "ci: build msan's libc++ with _LIBCPP_ABI_BOUNDED_*":
(https://github.com/bitcoin/bitcoin/pull/31612#issuecomment-2575517874)
Alright, changed that to `-DLIBCXX_ABI_DEFINES="_LIBCPP_ABI_BOUNDED_ITERATORS;_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STD_ARRAY;_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING;_LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR;_LIBCPP_ABI_BOUNDED_UNIQUE_PTR"` when compiling libc++.

💬 hebasto commented on pull request "test: Move `script_assets_tests` into its own suite":
(https://github.com/bitcoin/bitcoin/pull/31576#issuecomment-2575517966)
See the use case in https://github.com/hebasto/bitcoin-core-nightly/pull/27.

💬 vasild commented on pull request "ci: build msan's libc++ with _LIBCPP_ABI_BOUNDED_*":
(https://github.com/bitcoin/bitcoin/pull/31612#issuecomment-2575533490)
Hmm, once libc++ itself is compiled with `_LIBCPP_ABI_BOUNDED_*` does the user program have to be compiled with those as well, e.g. by passing `-D_LIBCPP_ABI_BOUNDED_ITERATORS` to the compiler? Or will those extra checks be enabled regardless of how the user program is compiled?

👍 danielabrozzoni approved a pull request: "test: importdescriptors is not available for non-descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/31609#pullrequestreview-2534610818)
Nice!

ACK 6b6b559d0f80246024a5b2fc8bcf357b86171472

👍 hodlinator approved a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2534616962)
re-ACK 9f61eb074438f85ed1a23fb3da94e4c68855d3fb

Fixed all my remaining nits.

### Tested a few `-dbcache` values

```
₿ build/src/bitcoind -dbcache=-1
...
2025-01-07T15:13:09Z Cache configuration:
2025-01-07T15:13:09Z * Using 0.5 MiB for block index database
2025-01-07T15:13:09Z * Using 1.8 MiB for chain state database
2025-01-07T15:13:09Z * Using 1.8 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
```
Corresponds to
```C++
//! min. -dbcache (MiB)
stat
...

💬 maflcko commented on pull request "test: importdescriptors is not available for non-descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/31609#issuecomment-2575595665)
No objection, but this will just create a conflict with the bdb removal, at which point the test will be deleted as well?

💬 maflcko commented on pull request "ci: build msan's libc++ with _LIBCPP_ABI_BOUNDED_*":
(https://github.com/bitcoin/bitcoin/pull/31612#issuecomment-2575608467)
review ACK fb37acd932b06c2386d07efe88a65ee3ac9aaa24

💬 JoesSon72 commented on pull request "fuzz: Expand script verification flag testing to segwit v0 and tapscript":
(https://github.com/bitcoin/bitcoin/pull/31460#issuecomment-2575617679)
GoMining.com

💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2575638533)
Sorry to invalidate the ACK again,
Updated 9f61eb074438f85ed1a23fb3da94e4c68855d3fb -> 578654c686e394d08bac7c3bcddbfd90b46eaa62 ([kernel_cache_sizes_8](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_8) -> [kernel_cache_sizes_9](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_9), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_8..kernel_cache_sizes_9))

* Forgot to drop the newlines in `LogInfo`, again. Fixed that now. -_-

👍 hodlinator approved a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2534714225)
re-ACK 578654c686e394d08bac7c3bcddbfd90b46eaa62

💬 Sjors commented on pull request "depends: Avoid hardcoding `host_prefix` in toolchain file":
(https://github.com/bitcoin/bitcoin/pull/31358#issuecomment-2575656384)
I tested on Intel macOS 13.7.2 that #31050 still happens on master @ 433412fd8478923dfdb20044f74c5d1e19fa8dd8. And I can confirm that this PR fixes it! 🎉

I also tested building and running the GUI.

It does issue a warning though:

```
CMake Warning:
Manually-specified variables were not used by the project:

CMAKE_TOOLCHAIN_FILE
```

💬 Sjors commented on issue "macOS 13.7 depends build can't find qt (symlink)":
(https://github.com/bitcoin/bitcoin/issues/31050#issuecomment-2575658439)
Update: it does! https://github.com/bitcoin/bitcoin/pull/31358#issuecomment-2575656384

💬 brunoerg commented on pull request "descriptor: check whitespace in `ParsePubkeyInner`":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2575659682)
> As you are forbidding something that was previously allowed, this breaks compatibility with earlier versions. Wallets containing any key with a whitespace in the descriptor will fail to parse during the load procedure and shut down abruptly. Will either need a descriptor upgrade procedure or a compatibility + warning window.

Good point. Moved it to draft to work on these things.

📝 hebasto opened a pull request: "build, test: Build `db_tests.cpp` regardless of `USE_BDB`"
(https://github.com/bitcoin/bitcoin/pull/31617)
When the building of `db_tests.cpp` was made conditional on `USE_BDB` in commit a58b719cf75e2d97205ec260bcff0d4780fe4fb8, all `db_tests` were indeed specific to BDB wallets.

However, the tests have since been [extended](https://github.com/bitcoin/bitcoin/commit/ba616b932cb9e9adb7eb9f1826caa62ce422a22d) to include SQLite wallets as well.

On the master branch @ 433412fd8478923dfdb20044f74c5d1e19fa8dd8, tests specific to SQLite wallets are not built and run if configured with `WITH_BDB=OFF` (
...

💬 TheCharlatan commented on pull request "init,log: Unify block index log line":
(https://github.com/bitcoin/bitcoin/pull/31616#discussion_r1905705466)
I keep on forgetting that we can drop the newline now too -_-. All these log lines here after (and including) `"Loading block index..."` are just kind of bad to be honest. We are definitely doing more here than loading the block index. I'm not even sure if the measured time here provides anything useful given that it varies quite a bit form run to run and we already log timestamps by default. Maybe this should rather just say `"Loaded block index and chainstate"`?

💬 mzumsande commented on pull request "Ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r1905647128)
I think that this won't achieve the desired result: If the assumevalid block is not in the block index (which will be the case if IBD crashed midway before that block was downloaded, and a reindex is done) we'll abort above in line 2499 and never get to this spot.