💬 starkshade commented on pull request "Mempool: Do not enforce TRUC checks on reorg":
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2390285981)
## ⚠️ Code Issue
**Lines:** 416-419
```typescript
const bool bypass_limits = fuzzed_data_provider.ConsumeBool(); // <- REMOVED from line 416
const auto tx = MakeTransactionRef(mut_tx);
const bool bypass_limits = fuzzed_data_provider.ConsumeBool(); // <- ADDED
ever_bypassed_limits |= bypass_limits; // <- ADDED
```
The `bypass_limits` variable is declared and consumed twice within the same loop iteration. This is redundant and can lead to confusion.
...
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2390285981)
## ⚠️ Code Issue
**Lines:** 416-419
```typescript
const bool bypass_limits = fuzzed_data_provider.ConsumeBool(); // <- REMOVED from line 416
const auto tx = MakeTransactionRef(mut_tx);
const bool bypass_limits = fuzzed_data_provider.ConsumeBool(); // <- ADDED
ever_bypassed_limits |= bypass_limits; // <- ADDED
```
The `bypass_limits` variable is declared and consumed twice within the same loop iteration. This is redundant and can lead to confusion.
...
💬 starkshade commented on pull request "Mempool: Do not enforce TRUC checks on reorg":
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2390286056)
## 💡 Improvement Needed
**Lines:** 1047-1069
The logic for handling sibling eviction and TRUC violations is nested within a check for `!args.m_bypass_limits`. If `args.m_bypass_limits` is true, the entire block related to `SingleTRUCChecks` is skipped. This might lead to unexpected behavior if sibling eviction or TRUC violations should be checked regardless of whether other limits are bypassed. Consider if this conditional logic accurately reflects the intended policy.
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2390286056)
## 💡 Improvement Needed
**Lines:** 1047-1069
The logic for handling sibling eviction and TRUC violations is nested within a check for `!args.m_bypass_limits`. If `args.m_bypass_limits` is true, the entire block related to `SingleTRUCChecks` is skipped. This might lead to unexpected behavior if sibling eviction or TRUC violations should be checked regardless of whether other limits are bypassed. Consider if this conditional logic accurately reflects the intended policy.
💬 starkshade commented on pull request "Mempool: Do not enforce TRUC checks on reorg":
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2390286119)
## 💡 Improvement Needed
**Line:** 177
The test `test_truc_reorg` creates several transactions using `create_self_transfer` and then mines them into a block. However, the `tx_v3_block2` is created with `version=3` but then later used to test an 'overly-large child size' with `target_vsize=1250`. It would be clearer if the variable name or the comment explicitly indicated this purpose, or if a separate transaction was created specifically for the oversized child test.
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2390286119)
## 💡 Improvement Needed
**Line:** 177
The test `test_truc_reorg` creates several transactions using `create_self_transfer` and then mines them into a block. However, the `tx_v3_block2` is created with `version=3` but then later used to test an 'overly-large child size' with `target_vsize=1250`. It would be clearer if the variable name or the comment explicitly indicated this purpose, or if a separate transaction was created specifically for the oversized child test.
💬 starkshade commented on pull request "Mempool: Do not enforce TRUC checks on reorg":
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2390285900)
## ⚠️ Code Issue
**Lines:** 328-328
```cpp
/*bypass_limits=*/false, /*test_accept=*/!single_submit));
```
This code snippet hardcodes the `bypass_limits` parameter to `false`. In a fuzzing context, this parameter should ideally be controlled by the fuzzer's input to explore a wider range of test cases, including those that bypass limits. This limitation reduces the effectiveness of the fuzz test in uncovering potential bugs related to limit enforceme
...
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2390285900)
## ⚠️ Code Issue
**Lines:** 328-328
```cpp
/*bypass_limits=*/false, /*test_accept=*/!single_submit));
```
This code snippet hardcodes the `bypass_limits` parameter to `false`. In a fuzzing context, this parameter should ideally be controlled by the fuzzer's input to explore a wider range of test cases, including those that bypass limits. This limitation reduces the effectiveness of the fuzz test in uncovering potential bugs related to limit enforceme
...
💬 starkshade commented on pull request "Mempool: Do not enforce TRUC checks on reorg":
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2390286216)
## 💡 Improvement Needed
**Line:** 170
The comment `# Testing 2<-3 versions allowed` and `# Testing 3<-2 versions allowed` are present, but the actual transactions created (`tx_v2_block` and `tx_v3_block`) are not directly used to test these specific version transitions in the subsequent mempool checks. The test focuses more on the reorg aspect and the oversized child. Clarifying the intent or adjusting the test to explicitly demonstrate these version transitions would be beneficial.
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2390286216)
## 💡 Improvement Needed
**Line:** 170
The comment `# Testing 2<-3 versions allowed` and `# Testing 3<-2 versions allowed` are present, but the actual transactions created (`tx_v2_block` and `tx_v3_block`) are not directly used to test these specific version transitions in the subsequent mempool checks. The test focuses more on the reorg aspect and the oversized child. Clarifying the intent or adjusting the test to explicitly demonstrate these version transitions would be beneficial.
💬 starkshade commented on pull request "Mempool: Do not enforce TRUC checks on reorg":
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2390286285)
## 💡 Improvement Needed
**Line:** 196
The variable `block` is assigned the result of `self.generateblock` on line 185. However, on line 196, `node.invalidateblock(block["hash"])` is called. It would be more consistent and potentially clearer to use `block` directly if it's a dictionary containing the hash, or to ensure `block` is always a list of block hashes if `generateblock` returns a list.
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2390286285)
## 💡 Improvement Needed
**Line:** 196
The variable `block` is assigned the result of `self.generateblock` on line 185. However, on line 196, `node.invalidateblock(block["hash"])` is called. It would be more consistent and potentially clearer to use `block` directly if it's a dictionary containing the hash, or to ensure `block` is always a list of block hashes if `generateblock` returns a list.
💬 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.