💬 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.
💬 starkshade commented on pull request "[29.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390343981)
## 💡 Improvement Needed
**Lines:** 75-80
The list of contributors should be alphabetized for better readability and consistency.
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390343981)
## 💡 Improvement Needed
**Lines:** 75-80
The list of contributors should be alphabetized for better readability and consistency.
💬 starkshade commented on pull request "[29.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390344059)
## 💡 Improvement Needed
**Line:** 167
The `GetTarget` function is now called with `blockindex` instead of `tip`. While this likely aligns with the intent to report the target difficulty of the specific `blockindex` being serialized, it's crucial to verify that this change does not inadvertently alter the expected output if `tip` was being used for a specific historical or comparative purpose. Ensure `GetTarget(blockindex, pow_limit)` accurately reflects the target difficulty for the `bloc
...
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390344059)
## 💡 Improvement Needed
**Line:** 167
The `GetTarget` function is now called with `blockindex` instead of `tip`. While this likely aligns with the intent to report the target difficulty of the specific `blockindex` being serialized, it's crucial to verify that this change does not inadvertently alter the expected output if `tip` was being used for a specific historical or comparative purpose. Ensure `GetTarget(blockindex, pow_limit)` accurately reflects the target difficulty for the `bloc
...
💬 starkshade commented on pull request "[29.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390344151)
## 💡 Improvement Needed
**Line:** 1497
There appears to be a typo in the documentation string. 'final_scriptwitness' should likely be 'final_scriptSig' and 'final_scriptWitness' for consistency with common naming conventions and the preceding field.
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390344151)
## 💡 Improvement Needed
**Line:** 1497
There appears to be a typo in the documentation string. 'final_scriptwitness' should likely be 'final_scriptSig' and 'final_scriptWitness' for consistency with common naming conventions and the preceding field.
💬 starkshade commented on pull request "[29.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390344300)
## 💡 Improvement Needed
**Lines:** 4035-4036
The added numerical values `1183846025` and `2713372123` should be verified for correctness and relevance within the context of the `mainnet_alt.json` test data. If these are part of a sequence or specific data points, ensure their accuracy and purpose are well-documented or self-evident.
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390344300)
## 💡 Improvement Needed
**Lines:** 4035-4036
The added numerical values `1183846025` and `2713372123` should be verified for correctness and relevance within the context of the `mainnet_alt.json` test data. If these are part of a sequence or specific data points, ensure their accuracy and purpose are well-documented or self-evident.
💬 starkshade commented on pull request "[29.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390345078)
## ⚠️ Issue
**Type:** Maintainability
**Severity:** Low
**Line:** 63
The `mine` function's `block.nBits` assignment logic is conditional on `height < 2016`. This logic is also implicitly checked later in `run_test` when asserting `mining_info['next']['bits']` and `mining_info['next']['target']`. Duplicating this logic increases the risk of inconsistencies if the retargeting rules change.
**Fix:**
Abstract the retargeting logic into a dedicated helper function or constant to ensur
...
(https://github.com/bitcoin/bitcoin/pull/33474#discussion_r2390345078)
## ⚠️ Issue
**Type:** Maintainability
**Severity:** Low
**Line:** 63
The `mine` function's `block.nBits` assignment logic is conditional on `height < 2016`. This logic is also implicitly checked later in `run_test` when asserting `mining_info['next']['bits']` and `mining_info['next']['target']`. Duplicating this logic increases the risk of inconsistencies if the retargeting rules change.
**Fix:**
Abstract the retargeting logic into a dedicated helper function or constant to ensur
...
💬 maflcko commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390346800)
@starkshade Stop spamming LLM generated slop that you don't understand yourself, or even have looked at yourself.
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390346800)
@starkshade Stop spamming LLM generated slop that you don't understand yourself, or even have looked at yourself.
🤔 starkshade reviewed a pull request: "[28.x] backports + 28.3rc1"
(https://github.com/bitcoin/bitcoin/pull/33476#pullrequestreview-3283265620)
This is a significant release candidate update for Bitcoin Core, primarily focusing on adjustments to transaction fee policies and related configurations. It includes changes to default minimum relay and block creation feerates, updates versioning information, and modifies documentation and tests to reflect these policy changes. The changes appear to be well-contained within the policy and testing domains.
**Key Changes:**
- Updated version to 28.3.0rc1.
- Lowered the default minimum block
...
(https://github.com/bitcoin/bitcoin/pull/33476#pullrequestreview-3283265620)
This is a significant release candidate update for Bitcoin Core, primarily focusing on adjustments to transaction fee policies and related configurations. It includes changes to default minimum relay and block creation feerates, updates versioning information, and modifies documentation and tests to reflect these policy changes. The changes appear to be well-contained within the policy and testing domains.
**Key Changes:**
- Updated version to 28.3.0rc1.
- Lowered the default minimum block
...