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

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

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

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

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

💬 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`?

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

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

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

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

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

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