💬 starkshade commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390332796)
## ⚠️ Issue

**Type:** Maintainability | **Severity:** Low

**Line:** 108

The `runs-on` directive was updated from `macos-14` to `macos-15`. This change might unintentionally remove support for macOS 14 if it was a deliberate requirement for specific jobs or environments.

**Fix:**
Verify if macOS 14 is still a required or supported environment. If not, this change is acceptable. If it is, consider adding a separate job or matrix configuration for macOS 14.

💬 starkshade commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390332869)
## ⚠️ Issue

**Type:** Maintainability | **Severity:** Low

**Line:** 126

The `job-name` for the standard job was updated from 'macOS 14 native, arm64, no depends, sqlite only, gui' to 'macOS native, no depends, sqlite only, gui'. This removes specific version and architecture information, potentially making it harder to identify the exact environment the job is running in at a glance.

**Fix:**
Consider re-adding specific version and architecture details to the `job-name` for better c
...

💬 starkshade commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390332963)
## ⚠️ Issue

**Type:** Maintainability | **Severity:** Low

**Line:** 129

The `job-name` for the fuzz job was updated from 'macOS 14 native, arm64, fuzz' to 'macOS native, fuzz'. This removes specific version and architecture information, potentially making it harder to identify the exact environment the job is running in at a glance.

**Fix:**
Consider re-adding specific version and architecture details to the `job-name` for better clarity, e.g., 'macOS 15 native, arm64, fuzz'.

💬 starkshade commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390333036)
## ⚠️ Issue

**Type:** Maintainability | **Severity:** Low

**Line:** 149

The Xcode version specified in the `xcode-select` command was updated from `Xcode_15.0.app` to `Xcode_16.0.app`. The comment referencing Xcode 15 release notes was also updated to reference Xcode 16. This change might be premature if Xcode 16 is not yet stable or widely available for the target macOS version, or if the project has specific compatibility requirements with Xcode 15.

**Fix:**
Ensure that Xcode 16 i
...

💬 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.

💬 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.

💬 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.

💬 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
...

🤔 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
...

💬 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.

💬 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.

💬 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.

🤔 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
...

💬 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
...

💬 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
...

💬 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.


...

💬 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
...

💬 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
...

💬 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
...

🤔 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
...