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