💬 hebasto commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390237740)
So people on macOS 14.4 with Xcode 15.4 and Apple Clang 1500.3.9.4 based on LLVM 16.0 will fail to compile `fuzz.cpp`, right?

💬 luke-jr commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#discussion_r2390259053)
Seems kind of ugly to lose the package name in the filename. How about `$(package)-$($(package)_version)-github.tar.gz` or something?

💬 maflcko commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390276369)
Yes. My guess is that it should be easy and harmless to upgrade Xcode, but I haven't verified this or tried it.

🤔 starkshade reviewed a pull request: "test: sock: Enable socket pair tests on Windows"
(https://github.com/bitcoin/bitcoin/pull/33506#pullrequestreview-3283180075)
The changes refactor the `CreateSocketPair` function to provide a cross-platform implementation and improve the handling of `Sock` objects using `std::unique_ptr` and move semantics, enhancing test robustness and memory management.

**Key Changes:**
- Introduced a Windows-compatible implementation for `CreateSocketPair` using TCP loopback connections, as `socketpair(2)` is not available on Windows.
- Modified `CreateSocketPair` to return `std::pair<Sock, Sock>` instead of modifying an intege
...

💬 starkshade commented on pull request "test: sock: Enable socket pair tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/33506#discussion_r2390282956)
## ⚠️ Code Issue

**Lines:** 107-117

```cpp
Sock* sock0 = new Sock(s[0]);
Sock* sock1 = new Sock(s[1]);

SendAndRecvMessage(*sock0, *sock1);

Sock* sock0moved = new Sock(std::move(*sock0));
Sock* sock1moved = new Sock(INVALID_SOCKET);
*sock1moved = std::move(*sock1);

delete sock0;
delete sock1;
```

This code snippet demonstrates manual memory management using `new` and `delete` for `Sock` objects. This is error-prone and can lead to memory lea
...

💬 starkshade commented on pull request "test: sock: Enable socket pair tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/33506#discussion_r2390282864)
## ⚠️ Code Issue

**Lines:** 86-89

```cpp
static void CreateSocketPair(int s[2])
{
BOOST_REQUIRE_EQUAL(socketpair(AF_UNIX, SOCK_STREAM, 0, s), 0);
}
```

The original `CreateSocketPair` function modified an array passed by reference. The new implementation returns a `std::pair<Sock, Sock>` by value, which is a better approach for managing socket resources, but the original function signature and implementation are removed, and the new implementation for non-Windows is now the pri
...

💬 starkshade commented on pull request "test: sock: Enable socket pair tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/33506#discussion_r2390283007)
## ⚠️ Code Issue

**Lines:** 147-151

```cpp
int s[2];
CreateSocketPair(s);

Sock sock_send(s[0]);
Sock sock_recv(s[1]);
```

Similar to the `wait` test case, this code initializes `Sock` objects directly from integer file descriptors. This bypasses RAII and could lead to resource management issues. Using `std::move` to transfer ownership is a more robust approach.

🤔 starkshade reviewed a pull request: "RPC: add sendrawtransactiontopeer"
(https://github.com/bitcoin/bitcoin/pull/33507#pullrequestreview-3283182194)
This change introduces a new RPC command, `sendrawtransactiontopeer`, which allows users to send a raw transaction directly to a specified peer without storing it in the local node's mempool. This is a valuable addition for testing transaction propagation and peer-to-peer interactions.

**Key Changes:**
- Added a new RPC command `sendrawtransactiontopeer` to submit a raw transaction to a specific peer.
- The new command takes `hexstring`, `peer_id`, and an optional `maxburnamount` as argumen
...

💬 starkshade commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390284640)
## 💡 Improvement Needed

**Line:** 170

The `sendrawtransactiontopeer` command, similar to `sendmsgtopeer` (line 168), involves network interaction. For clarity and consistency within the `RPC_COMMANDS_SAFE_FOR_FUZZING` list, it would be beneficial to add a comment explaining its safety considerations or potential side effects during fuzzing, especially if its behavior or safety profile is conditional (e.g., when peers are connected versus not connected).

**Example Fix:**
```diff
--- a
...

💬 starkshade commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390284264)
## ⚠️ Code Issue

**Lines:** 130-131

```cpp
{ "sendrawtransactiontopeer", 1, "peer_id" },
{ "sendrawtransactiontopeer", 2, "maxburnamount" },
```

These entries define parameter conversions for `sendrawtransactiontopeer`. However, if `maxfeerate` is also an optional parameter for this command (similar to `sendrawtransaction`), it is missing from this list, potentially causing inconsistencies in RPC help or parameter handling.

💬 starkshade commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390284443)
## ⚠️ Code Issue

**Lines:** 164-164

```cpp
const auto& tx_result = package_result.m_tx_results.find(tx->GetWitnessHash())->second;
```

The code assumes that the transaction hash will always be found in `package_result.m_tx_results` after calling `chainman.ProcessTransaction`. If `ProcessTransaction` encounters an early package-level rejection or for some other reason does not include the transaction's hash in the results map, dereferencing the iterator returned by `find()`
...

💬 starkshade commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390284527)
## ⚠️ Code Issue

**Lines:** 172-172

```cpp
msg_ser.data = TryParseHex<unsigned char>(request.params[0].get_str()).value();
```

The hex string `request.params[0].get_str()` is parsed twice: once by `DecodeHexTx` on line 144 to create `CMutableTransaction`, and again by `TryParseHex` on line 172 to get raw bytes for `CSerializedNetMsg`. While `DecodeHexTx` ensures the string is valid hex, re-parsing the same string is a minor inefficiency. If the raw bytes could be captured
...

💬 starkshade commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390284878)
## 💡 Improvement Needed

**Line:** 455

The `peer_id=0` argument in lines 455, 460, and 468 assumes that `self.nodes[0]` is always assigned peer ID 0 by `self.nodes[2]`. While this is often the case in test setups, relying on a fixed peer ID can make the test brittle if the network topology or peer connection order changes. It would be more robust to dynamically retrieve the `peer_id` of `self.nodes[0]` from `self.nodes[2]`'s `getpeerinfo` before using it.

**Fix:**
```diff
--- a/test/f
...

💬 starkshade commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390284359)
## ⚠️ Code Issue

**Lines:** 141-141

```cpp
const CAmount max_burn_amount = request.params[2].isNull() ? 0 : AmountFromValue(request.params[2]);
```

The `maxburnamount` argument has a `Default` value specified in `RPCHelpMan`. However, the handler explicitly checks `request.params[2].isNull()` and sets `max_burn_amount` to `0` if true. This implies that passing `null` for `maxburnamount` is treated differently from omitting the argument (which would use `DEFAULT_MAX_BURN_AM
...

💬 starkshade commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390284784)
## 💡 Improvement Needed

**Line:** 455

For consistency and clarity, it is recommended to use the `hexstring` keyword argument for all calls to `sendrawtransactiontopeer`, matching the usage in lines 460, 464, and 468. This makes the code more readable and explicit about the argument's purpose.

**Fix:**
```diff
--- a/test/functional/rpc_rawtransaction.py
+++ b/test/functional/rpc_rawtransaction.py
@@ -452,4 +452,4 @@
address = getnewdestination()[2]
outputs = {add
...

🤔 starkshade reviewed a pull request: "Mempool: Do not enforce TRUC checks on reorg"
(https://github.com/bitcoin/bitcoin/pull/33504#pullrequestreview-3283184463)
The changes primarily focus on refining the handling of transaction limits and TRUC (Transaction Replacement Under Coinjoin) invariants within the mempool. Specifically, the `bypass_limits` flag in `AcceptToMemoryPool` is being more strictly controlled in fuzzing tests and is now conditionally applied to TRUC checks. This aims to improve the robustness of fuzzing and ensure TRUC invariants are only checked when limits are not bypassed.

**Key Changes:**
- Modified `package_eval.cpp` to hardco
...

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

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

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

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