Bitcoin Core Github
44 subscribers
122K links
Download Telegram
📝 l0rinc opened a pull request: "init,log: Unify block index log line"
(https://github.com/bitcoin/bitcoin/pull/31616)
The line has been present since the beginning.
This PR simply removed the trailing whitespace and capitalizes the line for consistency.

Example logs before the change:
```
2025-01-07T11:58:33Z Verification progress: 99%
2025-01-07T11:58:33Z Verification: No coin database inconsistencies in last 6 blocks (18905 transactions)
2025-01-07T11:58:33Z block index 31892ms
2025-01-07T11:58:33Z Setting NODE_NETWORK on non-prune mode
```
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2575493948)
Updated 224bfeb07f7561470451c95dc0df1db959bc187e -> 9f61eb074438f85ed1a23fb3da94e4c68855d3fb ([kernel_cache_sizes_7](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_7) -> [kernel_cache_sizes_8](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_8), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_7..kernel_cache_sizes_8))

* Fixed comment in the correct commit now.
🤔 hodlinator reviewed a pull request: "refactor: cache block[undo] serialized size for consecutive calls"
(https://github.com/bitcoin/bitcoin/pull/31490#pullrequestreview-2534307845)
Concept ACK cba4378072a6bd43f3e37a7d5e662d3041681566

Removes repetitive calls to `GetSerializeSize()`. And avoids calling `.tell()`.

Nothing blocking.


### PR summary

Repetitive mention of #31539.


### Commit history

Tested splitting up 96c9b578a6b85fac9977cdf25bf2e52d04645bd4 into one which refactors existing code (225d294070fc403e03028b9fab714bd9dc4f3307), and a second commit (5f43ff3f1928e6aaca0edb4f108a03d66c8dac44) which renames the file and adds `SaveBlockBench()`. Keep
...
💬 hodlinator commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1905565819)
nit:
### Terminology

To me read/write and save/load are distinct pairs, so `ReadBlock`/`SaveBlock` seems off. Would have preferred `WriteBlockUndo` and `WriteBlock`.

I liked the explicitness of `ToDisk`/`FromDisk` for cases where that is actually what it's doing, makes code more self-documenting, even if slightly verbose. I see the change was prompted by https://github.com/bitcoin/bitcoin/pull/31490#pullrequestreview-2509562827.

Could the scripted diff commit message at least include
...
💬 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