💬 starkshade commented on pull request "Mempool: Do not enforce TRUC checks on reorg":
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2390286385)
## ⚠️ Issue
**Type:** Maintainability | **Severity:** Low
**Line:** 167
The test logic for checking TRUC rules during a reorg is spread across multiple lines and could be more clearly organized. The creation of transactions and the subsequent block generation are interleaved with mempool checks, making it harder to follow the test's progression.
**Fix:**
Consider grouping transaction creation, block generation, and mempool checks into distinct logical blocks within the test function
...
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2390286385)
## ⚠️ Issue
**Type:** Maintainability | **Severity:** Low
**Line:** 167
The test logic for checking TRUC rules during a reorg is spread across multiple lines and could be more clearly organized. The creation of transactions and the subsequent block generation are interleaved with mempool checks, making it harder to follow the test's progression.
**Fix:**
Consider grouping transaction creation, block generation, and mempool checks into distinct logical blocks within the test function
...
💬 davidgumberg commented on pull request "test: sock: Enable socket pair tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/33506#discussion_r2390291883)
Doesn't make sense, LLM comment, marking resolved
(https://github.com/bitcoin/bitcoin/pull/33506#discussion_r2390291883)
Doesn't make sense, LLM comment, marking resolved
🤔 starkshade reviewed a pull request: "p2p: implement sender-initiated package relay"
(https://github.com/bitcoin/bitcoin/pull/33500#pullrequestreview-3283190939)
This is a significant refactoring and feature addition focused on implementing transaction package relay, a new mechanism for more efficient transaction propagation in Bitcoin. The changes introduce new message types, update network protocol handling, and integrate with existing transaction management systems. Extensive modifications are present in network-related files and test cases, indicating a substantial effort to enable and test this new functionality.
**Key Changes:**
- Introduction
...
(https://github.com/bitcoin/bitcoin/pull/33500#pullrequestreview-3283190939)
This is a significant refactoring and feature addition focused on implementing transaction package relay, a new mechanism for more efficient transaction propagation in Bitcoin. The changes introduce new message types, update network protocol handling, and integrate with existing transaction management systems. Extensive modifications are present in network-related files and test cases, indicating a substantial effort to enable and test this new functionality.
**Key Changes:**
- Introduction
...
💬 starkshade commented on pull request "p2p: implement sender-initiated package relay":
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390290335)
## 💡 Improvement Needed
**Line:** 913
The array size has been increased from 33 to 35. The comment at line 910 states that only implemented message types need to be listed. If `NetMsgType::SENDPACKAGES` and `NetMsgType::PKGTXNS` (added on lines 943-944) are not implemented, they should not be added to this list. If they are implemented, the comment should be updated to reflect this.
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390290335)
## 💡 Improvement Needed
**Line:** 913
The array size has been increased from 33 to 35. The comment at line 910 states that only implemented message types need to be listed. If `NetMsgType::SENDPACKAGES` and `NetMsgType::PKGTXNS` (added on lines 943-944) are not implemented, they should not be added to this list. If they are implemented, the comment should be updated to reflect this.
💬 starkshade commented on pull request "p2p: implement sender-initiated package relay":
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390290505)
## ⚠️ Code Issue
**Lines:** 326-328
```typescript
bool SupportsVersion(node::PackageRelayVersions version) {
return m_package_versions_supported & version;
}
```
The `SupportsVersion` method's return statement `m_package_versions_supported & version` might not be as clear as it could be. It relies on the caller understanding that a non-zero result implies the version is supported. A more explicit check, such as `(m_package_versions_supported & version) == version`, would make th
...
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390290505)
## ⚠️ Code Issue
**Lines:** 326-328
```typescript
bool SupportsVersion(node::PackageRelayVersions version) {
return m_package_versions_supported & version;
}
```
The `SupportsVersion` method's return statement `m_package_versions_supported & version` might not be as clear as it could be. It relies on the caller understanding that a non-zero result implies the version is supported. A more explicit check, such as `(m_package_versions_supported & version) == version`, would make th
...
💬 starkshade commented on pull request "p2p: implement sender-initiated package relay":
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390290209)
## 💡 Improvement Needed
**Line:** 548
The description for the '-packagerelay' argument mentions it is '[EXPERIMENTAL]'. Consider if this flag should be exposed as a user-facing option or if it should be an internal feature controlled by other means, given its experimental nature.
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390290209)
## 💡 Improvement Needed
**Line:** 548
The description for the '-packagerelay' argument mentions it is '[EXPERIMENTAL]'. Consider if this flag should be exposed as a user-facing option or if it should be an internal feature controlled by other means, given its experimental nature.
💬 starkshade commented on pull request "p2p: implement sender-initiated package relay":
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390290761)
## ⚠️ Code Issue
**Lines:** 4902-4902
```typescript
const auto package_result{WITH_LOCK(cs_main, return ProcessNewPackage(m_chainman.ActiveChainstate(), m_mempool, package_to_validate.value().m_txns, /*test_accept=*/false, /*client_maxfeerate=*/std::nullopt))};
```
The `ProcessNewPackage` function is invoked to validate the received package. However, the current implementation might not sufficiently validate the dependency chain of transactions within the package. An attacker could po
...
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390290761)
## ⚠️ Code Issue
**Lines:** 4902-4902
```typescript
const auto package_result{WITH_LOCK(cs_main, return ProcessNewPackage(m_chainman.ActiveChainstate(), m_mempool, package_to_validate.value().m_txns, /*test_accept=*/false, /*client_maxfeerate=*/std::nullopt))};
```
The `ProcessNewPackage` function is invoked to validate the received package. However, the current implementation might not sufficiently validate the dependency chain of transactions within the package. An attacker could po
...
💬 starkshade commented on pull request "p2p: implement sender-initiated package relay":
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390290413)
## 💡 Improvement Needed
**Lines:** 945-947
The comment "Unimplemented message types that are assigned in BIP324:" is followed by two empty strings. If these empty strings represent specific unimplemented message types, it would be more maintainable to explicitly name them. If they are not intended to be placeholders for future implementation, they should be removed to avoid confusion.
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390290413)
## 💡 Improvement Needed
**Lines:** 945-947
The comment "Unimplemented message types that are assigned in BIP324:" is followed by two empty strings. If these empty strings represent specific unimplemented message types, it would be more maintainable to explicitly name them. If they are not intended to be placeholders for future implementation, they should be removed to avoid confusion.
💬 starkshade commented on pull request "p2p: implement sender-initiated package relay":
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390290650)
## ⚠️ Code Issue
**Lines:** 2531-2531
```typescript
auto ancestors{WITH_LOCK(m_mempool.cs, return m_mempool.AssumeCalculateMemPoolAncestors(__func__, *entry, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false))};
```
The `AssumeCalculateMemPoolAncestors` function is called with `fSearchForParents` set to `false`. This parameter controls whether the function should search for parent transactions that are not currently in the mempool. By setting it to `false`, the function will
...
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390290650)
## ⚠️ Code Issue
**Lines:** 2531-2531
```typescript
auto ancestors{WITH_LOCK(m_mempool.cs, return m_mempool.AssumeCalculateMemPoolAncestors(__func__, *entry, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false))};
```
The `AssumeCalculateMemPoolAncestors` function is called with `fSearchForParents` set to `false`. This parameter controls whether the function should search for parent transactions that are not currently in the mempool. By setting it to `false`, the function will
...
💬 starkshade commented on pull request "p2p: implement sender-initiated package relay":
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390290899)
## 💡 Improvement Needed
**Line:** 85
The comment `//! Whether this node enable package relay and enforces package-related protocol` is slightly redundant. The variable name `m_enable_package_relay` already indicates that it enables package relay. Consider making the comment more concise by focusing on the enforcement aspect, e.g., `//! Whether this node enforces package-related protocol`.
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390290899)
## 💡 Improvement Needed
**Line:** 85
The comment `//! Whether this node enable package relay and enforces package-related protocol` is slightly redundant. The variable name `m_enable_package_relay` already indicates that it enables package relay. Consider making the comment more concise by focusing on the enforcement aspect, e.g., `//! Whether this node enforces package-related protocol`.
💬 starkshade commented on pull request "p2p: implement sender-initiated package relay":
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390291044)
## ⚠️ Code Issue
**Lines:** 28-28
```cpp
if (auto value{argsman.GetBoolArg("-packagerelay", DEFAULT_ENABLE_PACKAGE_RELAY)}) options.m_enable_package_relay = value;
```
This code dereferences the `std::optional<bool>` returned by `GetBoolArg` without checking if it contains a value. If the argument is not provided, `value` will be `std::nullopt`, and dereferencing it (`*value`) will result in undefined behavior.
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390291044)
## ⚠️ Code Issue
**Lines:** 28-28
```cpp
if (auto value{argsman.GetBoolArg("-packagerelay", DEFAULT_ENABLE_PACKAGE_RELAY)}) options.m_enable_package_relay = value;
```
This code dereferences the `std::optional<bool>` returned by `GetBoolArg` without checking if it contains a value. If the argument is not provided, `value` will be `std::nullopt`, and dereferencing it (`*value`) will result in undefined behavior.
💬 starkshade commented on pull request "p2p: implement sender-initiated package relay":
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390291305)
## 💡 Improvement Needed
**Line:** 35
Add a comment explaining the rationale behind the specific value of `MAX_SENDER_INIT_PKG_SIZE` (currently 2). This will help future developers understand the reasoning for this limit and assist in potential future adjustments.
(https://github.com/bitcoin/bitcoin/pull/33500#discussion_r2390291305)
## 💡 Improvement Needed
**Line:** 35
Add a comment explaining the rationale behind the specific value of `MAX_SENDER_INIT_PKG_SIZE` (currently 2). This will help future developers understand the reasoning for this limit and assist in potential future adjustments.
💬 optout21 commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#discussion_r2390323798)
Would `proxy_override` be a more precise description of the semantics of the `proxy` parameter here as well, similar to how it's named in `ConnectNode`?
(https://github.com/bitcoin/bitcoin/pull/33454#discussion_r2390323798)
Would `proxy_override` be a more precise description of the semantics of the `proxy` parameter here as well, similar to how it's named in `ConnectNode`?
💬 hebasto commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390329377)
Upgrading Xcode 15.4 on macOS 14.4 requires updating macOS itself, at least to 14.5.
Therefore, the effective minimum supported macOS version becomes 14.5, not 14.0.
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390329377)
Upgrading Xcode 15.4 on macOS 14.4 requires updating macOS itself, at least to 14.5.
Therefore, the effective minimum supported macOS version becomes 14.5, not 14.0.
🤔 starkshade reviewed a pull request: "p2p: Mitigate GETADDR fingerprinting by setting address timestamps to a fixed value"
(https://github.com/bitcoin/bitcoin/pull/33498#pullrequestreview-3283248777)
The changes introduce a privacy enhancement by randomizing the timestamps of addresses served from the `getaddr` cache, mitigating potential correlation attacks on dual-homed nodes. A new functional test validates this behavior.
**Key Changes:**
- Implemented randomization of the `nTime` field for `CAddress` objects returned from the `getaddr` cache in `src/net.cpp`.
- Timestamps are now set to a random point in the past, specifically between 8 and 13 days prior to the current time, to prev
...
(https://github.com/bitcoin/bitcoin/pull/33498#pullrequestreview-3283248777)
The changes introduce a privacy enhancement by randomizing the timestamps of addresses served from the `getaddr` cache, mitigating potential correlation attacks on dual-homed nodes. A new functional test validates this behavior.
**Key Changes:**
- Implemented randomization of the `nTime` field for `CAddress` objects returned from the `getaddr` cache in `src/net.cpp`.
- Timestamps are now set to a random point in the past, specifically between 8 and 13 days prior to the current time, to prev
...
💬 starkshade commented on pull request "p2p: Mitigate GETADDR fingerprinting by setting address timestamps to a fixed value":
(https://github.com/bitcoin/bitcoin/pull/33498#discussion_r2390332398)
## ❌ Code Review
```javascript
const auto new_timestamp = now - std::chrono::minutes(8 * 60 * 24 + FastRandomContext().randrange(5 * 60 * 24));
```
This line contains magic numbers (`8`, `60`, `24`, `5`) that represent durations in minutes, hours, and days. While the surrounding comments provide context, using `std::chrono::days` or named constants would make the code more self-documenting and less prone to errors if the duration logic needs to be changed in the future. Additionally, the
...
(https://github.com/bitcoin/bitcoin/pull/33498#discussion_r2390332398)
## ❌ Code Review
```javascript
const auto new_timestamp = now - std::chrono::minutes(8 * 60 * 24 + FastRandomContext().randrange(5 * 60 * 24));
```
This line contains magic numbers (`8`, `60`, `24`, `5`) that represent durations in minutes, hours, and days. While the surrounding comments provide context, using `std::chrono::days` or named constants would make the code more self-documenting and less prone to errors if the duration logic needs to be changed in the future. Additionally, the
...
💬 starkshade commented on pull request "p2p: Mitigate GETADDR fingerprinting by setting address timestamps to a fixed value":
(https://github.com/bitcoin/bitcoin/pull/33498#discussion_r2390332469)
## 💡 Improvement Needed
**Line:** 140
The test implicitly assumes that the `addr` messages received will contain at least one address. If `last_message["addr"].addrs` is empty, subsequent access to `[0]` (e.g., on lines 146, 147, 150) will raise an `IndexError`. While unlikely in this specific test context, adding explicit checks for non-empty address lists would make the test more robust and prevent potential failures if the node's `getaddr` behavior changes or if the test environment le
...
(https://github.com/bitcoin/bitcoin/pull/33498#discussion_r2390332469)
## 💡 Improvement Needed
**Line:** 140
The test implicitly assumes that the `addr` messages received will contain at least one address. If `last_message["addr"].addrs` is empty, subsequent access to `[0]` (e.g., on lines 146, 147, 150) will raise an `IndexError`. While unlikely in this specific test context, adding explicit checks for non-empty address lists would make the test more robust and prevent potential failures if the node's `getaddr` behavior changes or if the test environment le
...
🤔 starkshade reviewed a pull request: "build: Drop support for EOL macOS 13"
(https://github.com/bitcoin/bitcoin/pull/33489#pullrequestreview-3283249372)
The changes primarily update the project's macOS build environment to target newer versions of macOS and Xcode. This includes updating CI configurations, minimum system version requirements, and Xcode toolchain paths. A minor change is also made to the fuzzing framework registration.
**Key Changes:**
- Updated CI workflow to use 'macos-15' runner.
- Updated Xcode toolchain to 'Xcode_16.0.app'.
- Increased minimum macOS version requirement from 13.0 to 14.0 in 'darwin.mk'.
- Updated minimu
...
(https://github.com/bitcoin/bitcoin/pull/33489#pullrequestreview-3283249372)
The changes primarily update the project's macOS build environment to target newer versions of macOS and Xcode. This includes updating CI configurations, minimum system version requirements, and Xcode toolchain paths. A minor change is also made to the fuzzing framework registration.
**Key Changes:**
- Updated CI workflow to use 'macos-15' runner.
- Updated Xcode toolchain to 'Xcode_16.0.app'.
- Increased minimum macOS version requirement from 13.0 to 14.0 in 'darwin.mk'.
- Updated minimu
...
💬 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.
(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
...
(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
...