💬 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
...
💬 starkshade commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390343614)
## 💡 Improvement Needed
**Line:** 3
The version number `_CLIENT_VERSION_MINOR` is hardcoded. For better maintainability and to avoid manual updates across different configuration files, consider using a more dynamic versioning strategy or a dedicated version management tool.
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390343614)
## 💡 Improvement Needed
**Line:** 3
The version number `_CLIENT_VERSION_MINOR` is hardcoded. For better maintainability and to avoid manual updates across different configuration files, consider using a more dynamic versioning strategy or a dedicated version management tool.
💬 starkshade commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390343984)
## 💡 Improvement Needed
**Line:** 4
The version number `v28.3.0rc1` in the NAME section should be updated to match the version specified in the `.TH` directive (line 2). This ensures consistency throughout the manual page.
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390343984)
## 💡 Improvement Needed
**Line:** 4
The version number `v28.3.0rc1` in the NAME section should be updated to match the version specified in the `.TH` directive (line 2). This ensures consistency throughout the manual page.
💬 starkshade commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390343881)
## 💡 Improvement Needed
**Line:** 2
The version number `v28.3.0rc1` and the date `September 2025` appear to be inconsistent. If this is a release candidate, the date should reflect the development stage. If it's a stable release, the version number should be updated. This inconsistency can confuse users about the software's current status.
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390343881)
## 💡 Improvement Needed
**Line:** 2
The version number `v28.3.0rc1` and the date `September 2025` appear to be inconsistent. If this is a release candidate, the date should reflect the development stage. If it's a stable release, the version number should be updated. This inconsistency can confuse users about the software's current status.
💬 starkshade commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390344184)
## 💡 Improvement Needed
**Line:** 708
The default value for `-minrelaytxfee` has been decreased from `0.00001` to `0.000001`. This change in a critical fee parameter warrants a more detailed explanation in the documentation regarding the reasoning and potential impact on transaction relaying and mining. Consider adding a brief note about the implications of this reduced default.
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390344184)
## 💡 Improvement Needed
**Line:** 708
The default value for `-minrelaytxfee` has been decreased from `0.00001` to `0.000001`. This change in a critical fee parameter warrants a more detailed explanation in the documentation regarding the reasoning and potential impact on transaction relaying and mining. Consider adding a brief note about the implications of this reduced default.
💬 starkshade commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390344408)
## 💡 Improvement Needed
**Line:** 4
The version number in the NAME section should be updated to `v28.3.0rc1` to accurately reflect the current release of `bitcoin-tx`.
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390344408)
## 💡 Improvement Needed
**Line:** 4
The version number in the NAME section should be updated to `v28.3.0rc1` to accurately reflect the current release of `bitcoin-tx`.
💬 starkshade commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390344078)
## 💡 Improvement Needed
**Line:** 18
The version number `v28.3.0rc1` in the DESCRIPTION section should be updated to match the version specified in the `.TH` directive (line 2) and the NAME section (line 4). This ensures consistency throughout the manual page.
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390344078)
## 💡 Improvement Needed
**Line:** 18
The version number `v28.3.0rc1` in the DESCRIPTION section should be updated to match the version specified in the `.TH` directive (line 2) and the NAME section (line 4). This ensures consistency throughout the manual page.