💬 starkshade commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390333113)
## 💡 Improvement Needed
**Line:** 1
The minimum macOS version is being increased to 14.0. It is recommended to ensure that all dependencies and build tools are compatible with macOS 14.0 before this change is merged.
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390333113)
## 💡 Improvement Needed
**Line:** 1
The minimum macOS version is being increased to 14.0. It is recommended to ensure that all dependencies and build tools are compatible with macOS 14.0 before this change is merged.
💬 starkshade commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390333226)
## 💡 Improvement Needed
**Line:** 39
The macOS version requirement has been updated from 13+ to 14+. This change should be reflected in the documentation to ensure users are aware of the minimum supported macOS version.
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390333226)
## 💡 Improvement Needed
**Line:** 39
The macOS version requirement has been updated from 13+ to 14+. This change should be reflected in the documentation to ensure users are aware of the minimum supported macOS version.
💬 starkshade commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390333310)
## 💡 Improvement Needed
**Line:** 6
The `LSMinimumSystemVersion` key has been updated to '14'. This implies the application now requires macOS Sonoma or a later version. Verify that this update is intentional and that the application has been tested against macOS 14 and is compatible with its features and APIs. If the application is intended to support older macOS versions, this change might inadvertently restrict its user base.
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390333310)
## 💡 Improvement Needed
**Line:** 6
The `LSMinimumSystemVersion` key has been updated to '14'. This implies the application now requires macOS Sonoma or a later version. Verify that this update is intentional and that the application has been tested against macOS 14 and is compatible with its features and APIs. If the application is intended to support older macOS versions, this change might inadvertently restrict its user base.
💬 starkshade commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390333382)
## ⚠️ Code Issue
**Lines:** 78-78
```cpp
const auto [it, ins]{FuzzTargets().try_emplace(name, std::move(target), std::move(opts))};
```
This line uses an overload of `try_emplace` that directly constructs the value in-place using the provided arguments (`std::move(target)`, `std::move(opts)`). This bypasses the explicit construction of `FuzzTarget` that was present in the removed code. While this might be intended for efficiency, it's important to ensure that the `FuzzTarget` construc
...
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390333382)
## ⚠️ Code Issue
**Lines:** 78-78
```cpp
const auto [it, ins]{FuzzTargets().try_emplace(name, std::move(target), std::move(opts))};
```
This line uses an overload of `try_emplace` that directly constructs the value in-place using the provided arguments (`std::move(target)`, `std::move(opts)`). This bypasses the explicit construction of `FuzzTarget` that was present in the removed code. While this might be intended for efficiency, it's important to ensure that the `FuzzTarget` construc
...
🤔 starkshade reviewed a pull request: "CMake: Add dynamic test discovery"
(https://github.com/bitcoin/bitcoin/pull/33483#pullrequestreview-3283251838)
This change introduces a new CMake module, `DiscoverTests.cmake`, and refactors the test discovery mechanism in `src/bench/CMakeLists.txt` and `src/test/CMakeLists.txt` to utilize this new module. The primary goal is to automate the discovery and registration of tests, reducing manual configuration and improving maintainability.
**Key Changes:**
- Introduced a new CMake function `discover_tests` in `cmake/module/DiscoverTests.cmake` to abstract test discovery logic.
- Replaced manual test r
...
(https://github.com/bitcoin/bitcoin/pull/33483#pullrequestreview-3283251838)
This change introduces a new CMake module, `DiscoverTests.cmake`, and refactors the test discovery mechanism in `src/bench/CMakeLists.txt` and `src/test/CMakeLists.txt` to utilize this new module. The primary goal is to automate the discovery and registration of tests, reducing manual configuration and improving maintainability.
**Key Changes:**
- Introduced a new CMake function `discover_tests` in `cmake/module/DiscoverTests.cmake` to abstract test discovery logic.
- Replaced manual test r
...
💬 starkshade commented on pull request "CMake: Add dynamic test discovery":
(https://github.com/bitcoin/bitcoin/pull/33483#discussion_r2390334653)
## 💡 Improvement Needed
**Line:** 187
The `DISCOVERY_MATCH` regex `[^ ].*\*$` might be too broad and could potentially match unintended test names if they follow this pattern. It's advisable to refine the regex to be more specific to the expected test suite naming conventions.
(https://github.com/bitcoin/bitcoin/pull/33483#discussion_r2390334653)
## 💡 Improvement Needed
**Line:** 187
The `DISCOVERY_MATCH` regex `[^ ].*\*$` might be too broad and could potentially match unintended test names if they follow this pattern. It's advisable to refine the regex to be more specific to the expected test suite naming conventions.
💬 starkshade commented on pull request "CMake: Add dynamic test discovery":
(https://github.com/bitcoin/bitcoin/pull/33483#discussion_r2390334719)
## 💡 Improvement Needed
**Line:** 188
The `TEST_NAME_REPLACEMENT` uses `bitcoin.test.\1`. If the `DISCOVERY_MATCH` regex captures more than one group, or if the captured group is not intended to be part of the test name, this could lead to unexpected test names. Ensure the regex and replacement are precisely aligned.
(https://github.com/bitcoin/bitcoin/pull/33483#discussion_r2390334719)
## 💡 Improvement Needed
**Line:** 188
The `TEST_NAME_REPLACEMENT` uses `bitcoin.test.\1`. If the `DISCOVERY_MATCH` regex captures more than one group, or if the captured group is not intended to be part of the test name, this could lead to unexpected test names. Ensure the regex and replacement are precisely aligned.
💬 starkshade commented on pull request "CMake: Add dynamic test discovery":
(https://github.com/bitcoin/bitcoin/pull/33483#discussion_r2390334787)
## 💡 Improvement Needed
**Line:** 189
The `TEST_ARGS_REPLACEMENT` hardcodes `--log_level=test_suite -- DEBUG_LOG_OUT`. If different test suites require different logging configurations or debug output settings, this static replacement will not accommodate those variations. Consider making these parameters more dynamic or configurable if needed.
(https://github.com/bitcoin/bitcoin/pull/33483#discussion_r2390334787)
## 💡 Improvement Needed
**Line:** 189
The `TEST_ARGS_REPLACEMENT` hardcodes `--log_level=test_suite -- DEBUG_LOG_OUT`. If different test suites require different logging configurations or debug output settings, this static replacement will not accommodate those variations. Consider making these parameters more dynamic or configurable if needed.
🤔 starkshade reviewed a pull request: "Rollback for dumptxoutset without invalidating blocks"
(https://github.com/bitcoin/bitcoin/pull/33477#pullrequestreview-3283254825)
The changes refactor the `dumptxoutset` RPC to improve its handling of historical UTXO snapshots. It introduces a new function `CreateRolledBackUTXOSnapshot` and removes RAII classes `NetworkDisable` and `TemporaryRollback`, simplifying the rollback logic. The test file `rpc_dumptxoutset.py` is updated to reflect these changes.
**Key Changes:**
- Introduced `CreateRolledBackUTXOSnapshot` function to encapsulate the logic for creating UTXO snapshots from historical states.
- Removed `Network
...
(https://github.com/bitcoin/bitcoin/pull/33477#pullrequestreview-3283254825)
The changes refactor the `dumptxoutset` RPC to improve its handling of historical UTXO snapshots. It introduces a new function `CreateRolledBackUTXOSnapshot` and removes RAII classes `NetworkDisable` and `TemporaryRollback`, simplifying the rollback logic. The test file `rpc_dumptxoutset.py` is updated to reflect these changes.
**Key Changes:**
- Introduced `CreateRolledBackUTXOSnapshot` function to encapsulate the logic for creating UTXO snapshots from historical states.
- Removed `Network
...
💬 starkshade commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2390336989)
## 💡 Improvement Needed
**Type:** Performance
**Line:** 3211
The `CreateRolledBackUTXOSnapshot` function flushes the `rollback_cache` every 500 blocks during the rollback process. Similar to the UTXO set copying, frequent flushes can impact performance. For deep rollbacks, this could become a significant bottleneck.
**Suggestion:**
Evaluate the impact of the `rollback_cache` flush frequency on the overall rollback performance. Consider increasing the flush interval or implementing
...
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2390336989)
## 💡 Improvement Needed
**Type:** Performance
**Line:** 3211
The `CreateRolledBackUTXOSnapshot` function flushes the `rollback_cache` every 500 blocks during the rollback process. Similar to the UTXO set copying, frequent flushes can impact performance. For deep rollbacks, this could become a significant bottleneck.
**Suggestion:**
Evaluate the impact of the `rollback_cache` flush frequency on the overall rollback performance. Consider increasing the flush interval or implementing
...
💬 starkshade commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2390336820)
## 💡 Improvement Needed
**Type:** Maintainability
**Lines:** 2992-3098
The `dumptxoutset` RPC function has been refactored to call `CreateRolledBackUTXOSnapshot` for rollback scenarios. However, the `dumptxoutset` function itself still contains a mix of responsibilities, including handling the 'latest' snapshot type and performing pruning checks. This could be further improved by extracting these distinct pieces of logic into their own dedicated helper functions or classes, leading to
...
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2390336820)
## 💡 Improvement Needed
**Type:** Maintainability
**Lines:** 2992-3098
The `dumptxoutset` RPC function has been refactored to call `CreateRolledBackUTXOSnapshot` for rollback scenarios. However, the `dumptxoutset` function itself still contains a mix of responsibilities, including handling the 'latest' snapshot type and performing pruning checks. This could be further improved by extracting these distinct pieces of logic into their own dedicated helper functions or classes, leading to
...
💬 starkshade commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2390336871)
## ⚠️ Issue
**Type:** Bug
**Severity:** High
**Lines:** 3112-3117
The `TemporaryUTXODatabase` RAII class is intended to clean up temporary UTXO database directories. However, if `fs::remove_all(m_path)` throws an exception, the cleanup fails, and the destructor logs an informational message but continues execution. This could lead to orphaned temporary directories on unclean shutdowns, requiring manual intervention. The current error handling is insufficient to guarantee cleanup.
...
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2390336871)
## ⚠️ Issue
**Type:** Bug
**Severity:** High
**Lines:** 3112-3117
The `TemporaryUTXODatabase` RAII class is intended to clean up temporary UTXO database directories. However, if `fs::remove_all(m_path)` throws an exception, the cleanup fails, and the destructor logs an informational message but continues execution. This could lead to orphaned temporary directories on unclean shutdowns, requiring manual intervention. The current error handling is insufficient to guarantee cleanup.
...
💬 starkshade commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2390337143)
## ⚠️ Code Issue
**Lines:** 22-31
```python
def check_expected_network(self, node, active):
rev_file = node.blocks_path / "rev00000.dat"
bogus_file = node.blocks_path / "bogus.dat"
rev_file.rename(bogus_file)
assert_raises_rpc_error(
-1, 'Could not roll back to requested height.', node.dumptxoutset, 'utxos.dat', rollback=99)
assert_equal(node.getnetworkinfo()['networkactive'], active)
# Cleanup
bogus_file.re
...
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2390337143)
## ⚠️ Code Issue
**Lines:** 22-31
```python
def check_expected_network(self, node, active):
rev_file = node.blocks_path / "rev00000.dat"
bogus_file = node.blocks_path / "bogus.dat"
rev_file.rename(bogus_file)
assert_raises_rpc_error(
-1, 'Could not roll back to requested height.', node.dumptxoutset, 'utxos.dat', rollback=99)
assert_equal(node.getnetworkinfo()['networkactive'], active)
# Cleanup
bogus_file.re
...
💬 starkshade commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2390337064)
## 💡 Improvement Needed
**Type:** Maintainability
**Line:** 3137
The temporary database used in `CreateRolledBackUTXOSnapshot` has a hardcoded cache size of 16MB (`size_t(1) << 24`). This fixed value might not be optimal across all system configurations and usage patterns. Hardcoding such values can make it difficult to tune performance without code modification.
**Suggestion:**
Consider making the cache size configurable. This could be achieved by defining it as a constant at a hi
...
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2390337064)
## 💡 Improvement Needed
**Type:** Maintainability
**Line:** 3137
The temporary database used in `CreateRolledBackUTXOSnapshot` has a hardcoded cache size of 16MB (`size_t(1) << 24`). This fixed value might not be optimal across all system configurations and usage patterns. Hardcoding such values can make it difficult to tune performance without code modification.
**Suggestion:**
Consider making the cache size configurable. This could be achieved by defining it as a constant at a hi
...
💬 starkshade commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2390336930)
## 💡 Improvement Needed
**Type:** Performance
**Lines:** 3174-3176
The `CreateRolledBackUTXOSnapshot` function flushes the `temp_cache` every 100,000 coins. While this aims to prevent excessive memory usage, frequent flushes can introduce performance overhead due to repeated disk I/O operations. This might be suboptimal for very large UTXO sets.
**Suggestion:**
Profile the performance of the UTXO set copying process with different flush intervals. Consider increasing the flush inte
...
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2390336930)
## 💡 Improvement Needed
**Type:** Performance
**Lines:** 3174-3176
The `CreateRolledBackUTXOSnapshot` function flushes the `temp_cache` every 100,000 coins. While this aims to prevent excessive memory usage, frequent flushes can introduce performance overhead due to repeated disk I/O operations. This might be suboptimal for very large UTXO sets.
**Suggestion:**
Profile the performance of the UTXO set copying process with different flush intervals. Consider increasing the flush inte
...
🤔 starkshade reviewed a pull request: "[29.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33474#pullrequestreview-3283265665)
This set of changes appears to be a minor release update (rc2) for Bitcoin Core, focusing on bug fixes, documentation updates, and improvements to functional tests related to mining and block data. The changes are generally small and targeted, with no obvious signs of introducing significant regressions.
**Key Changes:**
- Updated release notes to reflect version 29.2rc2.
- Fixed an issue in `getblock` RPC where the target for the tip block was incorrectly returned.
- Corrected a case typo
...
(https://github.com/bitcoin/bitcoin/pull/33474#pullrequestreview-3283265665)
This set of changes appears to be a minor release update (rc2) for Bitcoin Core, focusing on bug fixes, documentation updates, and improvements to functional tests related to mining and block data. The changes are generally small and targeted, with no obvious signs of introducing significant regressions.
**Key Changes:**
- Updated release notes to reflect version 29.2rc2.
- Fixed an issue in `getblock` RPC where the target for the tip block was incorrectly returned.
- Corrected a case typo
...
💬 starkshade commented on pull request "[29.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390343646)
## 💡 Improvement Needed
**Line:** 1
The release notes should clearly indicate the version number being released. The current entry is for '29.2rc2', but the URL points to 'test.rc2'. This could be confusing if the intent is to release a stable version.
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390343646)
## 💡 Improvement Needed
**Line:** 1
The release notes should clearly indicate the version number being released. The current entry is for '29.2rc2', but the URL points to 'test.rc2'. This could be confusing if the intent is to release a stable version.
💬 starkshade commented on pull request "[29.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390343894)
## 💡 Improvement Needed
**Line:** 57
The release notes are missing a dedicated section for documentation changes. While a specific documentation fix is listed under 'Notable changes', it would be more organized to have a separate 'Doc' heading.
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390343894)
## 💡 Improvement Needed
**Line:** 57
The release notes are missing a dedicated section for documentation changes. While a specific documentation fix is listed under 'Notable changes', it would be more organized to have a separate 'Doc' heading.
💬 starkshade commented on pull request "[29.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390343746)
## 💡 Improvement Needed
**Line:** 3
The URL for the release binaries should accurately reflect the version number. The current URL points to 'test.rc2', which might not be the intended stable release URL.
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390343746)
## 💡 Improvement Needed
**Line:** 3
The URL for the release binaries should accurately reflect the version number. The current URL points to 'test.rc2', which might not be the intended stable release URL.
💬 starkshade commented on pull request "[29.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390343812)
## 💡 Improvement Needed
**Line:** 46
The release notes are missing a dedicated section for RPC changes. While a specific RPC fix is listed under 'Notable changes', it would be more organized to have a separate 'RPC' heading.
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390343812)
## 💡 Improvement Needed
**Line:** 46
The release notes are missing a dedicated section for RPC changes. While a specific RPC fix is listed under 'Notable changes', it would be more organized to have a separate 'RPC' heading.