💬 l0rinc commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2585263747)
I think I managed to reproduce the ~2% difference - by not doing an IBD but a `-reindex-chainstate`.
@Sjors, was your datadir completely empty for the runs? My 12% comes from having nothing locally (e.g. no blocks) to being fully synced (i.e. has to include the final flush as well) - to be as close to the user's experience as possible.
<details>
<summary>Details</summary>
```
hyperfine \
--runs 2 \
--parameter-list COMMIT d73f37dda221835b5109ede1b84db2dc7c4b74a1,fe7365584bb3703e5691c
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2585263747)
I think I managed to reproduce the ~2% difference - by not doing an IBD but a `-reindex-chainstate`.
@Sjors, was your datadir completely empty for the runs? My 12% comes from having nothing locally (e.g. no blocks) to being fully synced (i.e. has to include the final flush as well) - to be as close to the user's experience as possible.
<details>
<summary>Details</summary>
```
hyperfine \
--runs 2 \
--parameter-list COMMIT d73f37dda221835b5109ede1b84db2dc7c4b74a1,fe7365584bb3703e5691c
...
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2585284602)
Thank you for the re-newed reviews and your suggestions @ryanofsky and @stickies-v. I hope we can settle on something this time round:
Updated 82706e217f34d1f09cbd30dde6b8ae5ac0385f0a -> 45fe603f8fd94c171f5ce1a38bc20d273baa9b1b ([kernel_cache_sizes_11](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_11) -> [kernel_cache_sizes_12](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_12), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_11.
...
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2585284602)
Thank you for the re-newed reviews and your suggestions @ryanofsky and @stickies-v. I hope we can settle on something this time round:
Updated 82706e217f34d1f09cbd30dde6b8ae5ac0385f0a -> 45fe603f8fd94c171f5ce1a38bc20d273baa9b1b ([kernel_cache_sizes_11](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_11) -> [kernel_cache_sizes_12](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_12), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_11.
...
📝 satscoffee opened a pull request: "Comment coypright date update"
(https://github.com/bitcoin/bitcoin/pull/31642)
doc: updated comments in line 1 from:
// Copyright (c) 2012-2022 The Bitcoin Core developers
to
// Copyright (c) 2012-2025 The Bitcoin Core developers
PR was for comment update only so no testing was performed.
(https://github.com/bitcoin/bitcoin/pull/31642)
doc: updated comments in line 1 from:
// Copyright (c) 2012-2022 The Bitcoin Core developers
to
// Copyright (c) 2012-2025 The Bitcoin Core developers
PR was for comment update only so no testing was performed.
💬 instagibbs commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2585300460)
is this in draft for a reason?
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2585300460)
is this in draft for a reason?
💬 NicolaLS commented on pull request "doc: merge required dependency tables":
(https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2585301823)
I agree that the previous version or this PR did not make sense for clearing up the confusion. I force pushed a new approach and amended the commit message / PR description.
(https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2585301823)
I agree that the previous version or this PR did not make sense for clearing up the confusion. I force pushed a new approach and amended the commit message / PR description.
✅ fanquake closed a pull request: "Comment coypright date update"
(https://github.com/bitcoin/bitcoin/pull/31642)
(https://github.com/bitcoin/bitcoin/pull/31642)
👍 tdb3 approved a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2545082647)
code review re ACK 58b828f7f7ad216c7d7bf2e2ff431a66591e9d5c
The data file is down to 71K, which is much nicer!
`mining_mainnet.py` ran locally without issue.
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2545082647)
code review re ACK 58b828f7f7ad216c7d7bf2e2ff431a66591e9d5c
The data file is down to 71K, which is much nicer!
`mining_mainnet.py` ran locally without issue.
💬 tdb3 commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1912095692)
nit: Is it 2 and above?
```diff
- above 2
+ to at least 2
```
https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/src/consensus/tx_check.cpp#L47-L51
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1912095692)
nit: Is it 2 and above?
```diff
- above 2
+ to at least 2
```
https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/src/consensus/tx_check.cpp#L47-L51
💬 tdb3 commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1912180713)
Wonder if it would be sufficient to grind only the nonce if starting with a testnet genesis block (instead of mainnet), but not sure it's worth digging into. It might allow removing the timestamps from `mainnet_alt.json` (reducing 35KB of the current 71KB), but the current approach is already more manageable that the previous ~1MB.
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1912180713)
Wonder if it would be sufficient to grind only the nonce if starting with a testnet genesis block (instead of mainnet), but not sure it's worth digging into. It might allow removing the timestamps from `mainnet_alt.json` (reducing 35KB of the current 71KB), but the current approach is already more manageable that the previous ~1MB.
💬 tdb3 commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1912123038)
If retouching, looks like we can adjust
```diff
- f'--nbits={hex(DIFF_1_N_BITS)}',
+ f'--nbits={DIFF_1_N_BITS:08x}',
```
Currently the `0x` is included.
Looks like parts of `miner` expect no `0x` though (8 hex digits):
https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/contrib/signet/miner#L465-L467
https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/contrib/signet/miner#L356-L359
```python
>>> f'--nbits={hex(DIFF_1_N
...
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1912123038)
If retouching, looks like we can adjust
```diff
- f'--nbits={hex(DIFF_1_N_BITS)}',
+ f'--nbits={DIFF_1_N_BITS:08x}',
```
Currently the `0x` is included.
Looks like parts of `miner` expect no `0x` though (8 hex digits):
https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/contrib/signet/miner#L465-L467
https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/contrib/signet/miner#L356-L359
```python
>>> f'--nbits={hex(DIFF_1_N
...
💬 tdb3 commented on pull request "rpc: add `relevant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1912183107)
Thanks. Will include in next update.
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1912183107)
Thanks. Will include in next update.
💬 tdb3 commented on pull request "rpc: add `relevant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1912183235)
Will include
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1912183235)
Will include
💬 tdb3 commented on pull request "rpc: add `relevant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1912183772)
Good catch. Verified locally that truncation occurs. Fixed for next update.
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1912183772)
Good catch. Verified locally that truncation occurs. Fixed for next update.
💬 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}) {
```