🚀 hebasto merged a pull request: "ci: use latest versions of lint deps"
(https://github.com/bitcoin/bitcoin/pull/33487)
(https://github.com/bitcoin/bitcoin/pull/33487)
👍 vasild approved a pull request: "depends: Update URL for `qrencode` package source tarball"
(https://github.com/bitcoin/bitcoin/pull/33494#pullrequestreview-3282991826)
ACK 980e4987db0450cabb8761433001841f7e9f4c73
I confirm the diff from the OP. The old archive is not available anymore at https://fukuchi.org/works/qrencode/qrencode-4.1.1.tar.gz, so I downloaded it from https://bitcoincore.org/depends-sources/qrencode-4.1.1.tar.gz instead. The new one is at https://github.com/fukuchi/libqrencode/archive/refs/tags/v4.1.1.tar.gz.
Then I extracted each archive, did
```sh
for f in $(find ./ -type f |sort) ; do sha256sum "$f" ; done
```
and compared the resulting d
...
(https://github.com/bitcoin/bitcoin/pull/33494#pullrequestreview-3282991826)
ACK 980e4987db0450cabb8761433001841f7e9f4c73
I confirm the diff from the OP. The old archive is not available anymore at https://fukuchi.org/works/qrencode/qrencode-4.1.1.tar.gz, so I downloaded it from https://bitcoincore.org/depends-sources/qrencode-4.1.1.tar.gz instead. The new one is at https://github.com/fukuchi/libqrencode/archive/refs/tags/v4.1.1.tar.gz.
Then I extracted each archive, did
```sh
for f in $(find ./ -type f |sort) ; do sha256sum "$f" ; done
```
and compared the resulting d
...
💬 vasild commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3350439700)
`82e46a7248...2ae966b222`: address suggestions, thanks!
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3350439700)
`82e46a7248...2ae966b222`: address suggestions, thanks!
💬 vasild commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#discussion_r2390185247)
Right! Changed to `target_addr`.
(https://github.com/bitcoin/bitcoin/pull/33454#discussion_r2390185247)
Right! Changed to `target_addr`.
💬 vasild commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#discussion_r2390186651)
I like `proxy_override`, changed.
(https://github.com/bitcoin/bitcoin/pull/33454#discussion_r2390186651)
I like `proxy_override`, changed.
💬 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?
(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?
(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.
(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
...
(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
...
(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
...
(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.
(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
...
(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
...
(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.
(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()`
...
(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
...
(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
...
(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
...
(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
...
(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
...