💬 tdb3 commented on pull request "rpc: add `relevant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1912184018)
Agreed. Comment will be added.
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1912184018)
Agreed. Comment will be added.
💬 tdb3 commented on pull request "rpc: add `relevant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1912184109)
Thanks, fixed.
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1912184109)
Thanks, fixed.
💬 tdb3 commented on pull request "rpc: add `relevant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2585385724)
> I think we could have a setmockcondition RPC that acts like the existing setmocktime RPC and lets tests toggle global state that could be useful for testing.
Great idea. Playing around with it locally.
(https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2585385724)
> I think we could have a setmockcondition RPC that acts like the existing setmocktime RPC and lets tests toggle global state that could be useful for testing.
Great idea. Playing around with it locally.
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2585393698)
Updated 45fe603f8fd94c171f5ce1a38bc20d273baa9b1b -> cf20c1b9e0b46cb8c4d4f93a054afd9c1f85e4e0 ([kernel_cache_sizes_12](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_12) -> [kernel_cache_sizes_13](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_13), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_12..kernel_cache_sizes_13))
* Fixed an issue with an include, a cast, and a signed/unsigned comparison.
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2585393698)
Updated 45fe603f8fd94c171f5ce1a38bc20d273baa9b1b -> cf20c1b9e0b46cb8c4d4f93a054afd9c1f85e4e0 ([kernel_cache_sizes_12](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_12) -> [kernel_cache_sizes_13](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_13), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_12..kernel_cache_sizes_13))
* Fixed an issue with an include, a cast, and a signed/unsigned comparison.
👍 hodlinator approved a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2545133420)
re-ACK cf20c1b9e0b46cb8c4d4f93a054afd9c1f85e4e0
Happy with the `_MiB` user-defined literals. :)
Nothing blocking. Should probably at least fix the typo.
---
mega-nit: Could possibly have used `CheckedLeftShift()` or `SaturatingLeftShift()` in intermediate commits. But end state doesn't have naked shifts on touched lines, which is fine.
```diff
- kernel::CacheSizes cache_sizes{nDefaultDbCache << 20};
+ kernel::CacheSizes cache_sizes{CheckedLeftShift<size_t>(nDefaultDbCache, 20).val
...
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2545133420)
re-ACK cf20c1b9e0b46cb8c4d4f93a054afd9c1f85e4e0
Happy with the `_MiB` user-defined literals. :)
Nothing blocking. Should probably at least fix the typo.
---
mega-nit: Could possibly have used `CheckedLeftShift()` or `SaturatingLeftShift()` in intermediate commits. But end state doesn't have naked shifts on touched lines, which is fine.
```diff
- kernel::CacheSizes cache_sizes{nDefaultDbCache << 20};
+ kernel::CacheSizes cache_sizes{CheckedLeftShift<size_t>(nDefaultDbCache, 20).val
...
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912175146)
nit: Give a hint at why?
```suggestion
throw std::overflow_error("Too many mebibytes to fit into size_t bytes");
```
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912175146)
nit: Give a hint at why?
```suggestion
throw std::overflow_error("Too many mebibytes to fit into size_t bytes");
```
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912185410)
nit: Could modernize cast.
```suggestion
if (std::make_unsigned_t<Input>(i) > Output{std::numeric_limits<Output>::max() >> shift}) {
```
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912185410)
nit: Could modernize cast.
```suggestion
if (std::make_unsigned_t<Input>(i) > Output{std::numeric_limits<Output>::max() >> shift}) {
```
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912181339)
nit: Could have less churn on the line by making `InitAndLoadChainstate()` take `const` in the same commit where this line is first touched.
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912181339)
nit: Could have less churn on the line by making `InitAndLoadChainstate()` take `const` in the same commit where this line is first touched.
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912179390)
nit: Typo
```suggestion
// Convert -dbcache from MiB units to bytes. The total cache is floored by MIN_DB_CACHE and capped by max size_t value.
```
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912179390)
nit: Typo
```suggestion
// Convert -dbcache from MiB units to bytes. The total cache is floored by MIN_DB_CACHE and capped by max size_t value.
```
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912212037)
nit in commit 4eebb638916ecf43cfd9237c7e8bb8da7bc397d7:
Seems like we could use `std::min` without the template parameter here from the start (it's removed later without parameter types changing, resulting in line churn).
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912212037)
nit in commit 4eebb638916ecf43cfd9237c7e8bb8da7bc397d7:
Seems like we could use `std::min` without the template parameter here from the start (it's removed later without parameter types changing, resulting in line churn).
💬 andrewtoth commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2585403343)
I benchmarked this PR rebased onto 35bf426e02210c1bbb04926f4ca2e0285fbfcd11 up to block 878k two times each, and I saw a 9% speedup. This was using `dbcache=30000`.
```
hyperfine --export-markdown ~/bench.md --show-output --parameter-list commit 1298bae74a1f690fd6cc0e029e490537cbeb301b,35bf426e02210c1bbb04926f4ca2e0285fbfcd11 --prepare 'git checkout {commit} && \
cmake --build build -j $(nproc); \
rm -rf /home/user/.bitcoin; \
sync; sudo /sbin/sysctl vm.drop_caches=3;' -M 2 'echo '{commit
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2585403343)
I benchmarked this PR rebased onto 35bf426e02210c1bbb04926f4ca2e0285fbfcd11 up to block 878k two times each, and I saw a 9% speedup. This was using `dbcache=30000`.
```
hyperfine --export-markdown ~/bench.md --show-output --parameter-list commit 1298bae74a1f690fd6cc0e029e490537cbeb301b,35bf426e02210c1bbb04926f4ca2e0285fbfcd11 --prepare 'git checkout {commit} && \
cmake --build build -j $(nproc); \
rm -rf /home/user/.bitcoin; \
sync; sudo /sbin/sysctl vm.drop_caches=3;' -M 2 'echo '{commit
...
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912234840)
Using non-narrowing conversion should give you a compiler error. Allowing type narrowing here should be safe.
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912234840)
Using non-narrowing conversion should give you a compiler error. Allowing type narrowing here should be safe.
👍 hodlinator approved a pull request: "doc: improve dependencies documentation"
(https://github.com/bitcoin/bitcoin/pull/31634#pullrequestreview-2545217904)
ACK d0ca5c53635916868e7d5da913faf3d9a8d706b7
Good to clarify that only one of the compilers is required, and re-introduce that section!
Thanks for updating the PR summary.
### Commit message
Suggest removing repetition of initial sentence and fixing grammar of last line, + less important other changes.
```diff
-doc: improve dependencies documentation
+doc: Improve dependencies documentation
-This commit improves the dependencies documentation by:
-- moving "Clang" and "GCC"
...
(https://github.com/bitcoin/bitcoin/pull/31634#pullrequestreview-2545217904)
ACK d0ca5c53635916868e7d5da913faf3d9a8d706b7
Good to clarify that only one of the compilers is required, and re-introduce that section!
Thanks for updating the PR summary.
### Commit message
Suggest removing repetition of initial sentence and fixing grammar of last line, + less important other changes.
```diff
-doc: improve dependencies documentation
+doc: Improve dependencies documentation
-This commit improves the dependencies documentation by:
-- moving "Clang" and "GCC"
...
💬 hodlinator commented on pull request "doc: improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1912232236)
nit: Not yet touched by this PR, but could optionally add some context.
```suggestion
| Linux Kernel (if building that platform) | [link](https://www.kernel.org/) | N/A | [3.17.0](https://github.com/bitcoin/bitcoin/pull/27699) | Yes |
```
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1912232236)
nit: Not yet touched by this PR, but could optionally add some context.
```suggestion
| Linux Kernel (if building that platform) | [link](https://www.kernel.org/) | N/A | [3.17.0](https://github.com/bitcoin/bitcoin/pull/27699) | Yes |
```
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2585406915)
Thanks for the ACK @hodlinator :)
Updated cf20c1b9e0b46cb8c4d4f93a054afd9c1f85e4e0 -> 0e2887262e1a3a4a5ab0382f9e8713b135408a77 ([kernel_cache_sizes_13](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_13) -> [kernel_cache_sizes_14](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_14), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_13..kernel_cache_sizes_14))
* Addressed @hodlinator's [comment](https://github.com/bitcoin/bitcoin/pu
...
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2585406915)
Thanks for the ACK @hodlinator :)
Updated cf20c1b9e0b46cb8c4d4f93a054afd9c1f85e4e0 -> 0e2887262e1a3a4a5ab0382f9e8713b135408a77 ([kernel_cache_sizes_13](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_13) -> [kernel_cache_sizes_14](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_14), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_13..kernel_cache_sizes_14))
* Addressed @hodlinator's [comment](https://github.com/bitcoin/bitcoin/pu
...
👍 hodlinator approved a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2545245484)
re-ACK 0e2887262e1a3a4a5ab0382f9e8713b135408a77
Thanks for addressing my comments. :rocket:
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2545245484)
re-ACK 0e2887262e1a3a4a5ab0382f9e8713b135408a77
Thanks for addressing my comments. :rocket:
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912238290)
What the.. could have sworn my suggestion compiled without warnings, but now I get them. Sorry.
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912238290)
What the.. could have sworn my suggestion compiled without warnings, but now I get them. Sorry.
💬 RonieLabr70031 commented on pull request "kernel: chainparams updates for 26.x":
(https://github.com/bitcoin/bitcoin/pull/28591#issuecomment-2585644697)
# BIP-44 Derivation Paths
| BTC | Bitcoin | GetPublicKey | bc1q7799s3qgqpuvfyux7u3vgrd2lus66sc0gmeaxj | SignTx | Derivation | Note |
|-------------------|------------------|------------------------|--------------------|--------------------|---------------|----------------|
| Bitcoin | secp256k1 | `m/p'/c'/a'` | `m/p'/c'/a'/h/i` | `m/p'/c'/a'/h/i` | BIP-32 | [21](#Bitcoin) |
| Ethereum
...
(https://github.com/bitcoin/bitcoin/pull/28591#issuecomment-2585644697)
# BIP-44 Derivation Paths
| BTC | Bitcoin | GetPublicKey | bc1q7799s3qgqpuvfyux7u3vgrd2lus66sc0gmeaxj | SignTx | Derivation | Note |
|-------------------|------------------|------------------------|--------------------|--------------------|---------------|----------------|
| Bitcoin | secp256k1 | `m/p'/c'/a'` | `m/p'/c'/a'/h/i` | `m/p'/c'/a'/h/i` | BIP-32 | [21](#Bitcoin) |
| Ethereum
...
💬 hebasto commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2585683053)
> The new code in `test/functional/wallet_encryption.py` [fails](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/12720574600/job/35462426268) on NetBSD...
The root cause of the issue is that the `-Wl,-z,separate-code` linker option is incompatible with NetBSD's dynamic linker. Building with `-DENABLE_HARDENING=OFF` or `-DAPPEND_LDFLAGS="-Wl,-z,noseparate-code"` fixes the issue.
(https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2585683053)
> The new code in `test/functional/wallet_encryption.py` [fails](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/12720574600/job/35462426268) on NetBSD...
The root cause of the issue is that the `-Wl,-z,separate-code` linker option is incompatible with NetBSD's dynamic linker. Building with `-DENABLE_HARDENING=OFF` or `-DAPPEND_LDFLAGS="-Wl,-z,noseparate-code"` fixes the issue.
💬 Andrewsr84 commented on issue "cmake: Compiling for test coverage (low-priority workaround exists)":
(https://github.com/bitcoin/bitcoin/issues/31638#issuecomment-2585699638)
Thank for the help
(https://github.com/bitcoin/bitcoin/issues/31638#issuecomment-2585699638)
Thank for the help