backport: Merge bitcoin/bitcoin#27480, 27664, 27334, 27930, 27831 #6688
Conversation
c8e0664 doc: Improve documentation of rpcallowip rpchelp (willcl-ark) Pull request description: Closes bitcoin#21070 v21.0 introduced a behaviour changed noted in bitcoin#21070 where using a config value `rpcallowip=::0` no longer also permitted ipv4 ip addresses. The rpc_bind.py functional test covers this new behaviour already by checking that the list of bind addresses exactly matches what is expected so this commit only updates the documentation. ACKs for top commit: achow101: ACK c8e0664 pinheadmz: ACK c8e0664 jonatack: ACK c8e0664 Tree-SHA512: 332060cf0df0427c6637a9fd1e0783ce0b0940abdb41b0df13f03bfbdc28af067cec8f0b1bbc4e47b3d54fa1b2f110418442b05b39d5e7c7e0b96744ddd7c003
e9dcac1 add `lief` to `spelling.ignore-words` (brunoerg) 258f930 test: fix spelling in `interface_usdt_utxocache` (brunoerg) Pull request description: Add `lief` to `spelling.ignore-words` since it's the name of a Python lib and fix spelling error in `interface_usdt_utxocache` (s/eariler/earlier) ACKs for top commit: fanquake: ACK e9dcac1 Tree-SHA512: f42cdbb74144c5d289d70bb9bac1179650bb32594678e15f4e18e4b2f68009999d60ff69494d4e68650d82fc1838e67515ed2f922ee84db98735ef906911ec40
WalkthroughThis set of changes introduces new templated benchmark functions for creating vectors of ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
test/functional/interface_usdt_validation.py (1)
100-115:⚠️ Potential issueInconsistent callback implementation - assertions still present.
The callback function still contains assertion logic (lines 105-114) and modifies shared state (
expected_blocks,blocks_checked), which contradicts the stated goal of moving assertions outside callbacks for proper error propagation.Based on the AI summary and the addition of the events processing loop below, the callback should only append events to the list:
def handle_blockconnected(_, data, __): - nonlocal events, blocks_checked + nonlocal events event = ctypes.cast(data, ctypes.POINTER(Block)).contents self.log.info(f"handle_blockconnected(): {event}") - block_hash = bytes(event.hash[::-1]).hex() - block = expected_blocks[block_hash] - assert_equal(block["hash"], block_hash) - assert_equal(block["height"], event.height) - assert_equal(len(block["tx"]), event.transactions) - assert_equal(len([tx["vin"] for tx in block["tx"]]), event.inputs) - assert_equal(0, event.sigops) # no sigops in coinbase tx - # only plausibility checks - assert(event.duration > 0) - del expected_blocks[block_hash] - blocks_checked += 1 + events.append(event)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/bench/prevector.cpp(2 hunks)src/init.cpp(1 hunks)src/prevector.h(2 hunks)src/support/allocators/secure.h(2 hunks)src/support/allocators/zeroafterfree.h(1 hunks)src/validation.h(2 hunks)test/functional/interface_usdt_net.py(3 hunks)test/functional/interface_usdt_utxocache.py(3 hunks)test/functional/interface_usdt_validation.py(3 hunks)test/lint/spelling.ignore-words.txt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
test/functional/interface_usdt_utxocache.py (1)
test/functional/test_framework/util.py (1)
assert_equal(64-69)
test/functional/interface_usdt_validation.py (1)
test/functional/test_framework/util.py (1)
assert_equal(64-69)
test/functional/interface_usdt_net.py (1)
test/functional/test_framework/util.py (1)
assert_equal(64-69)
src/support/allocators/zeroafterfree.h (2)
src/support/allocators/secure.h (4)
n(28-35)n(28-28)p(37-43)p(37-37)src/support/cleanse.h (1)
memory_cleanse(13-13)
src/support/allocators/secure.h (1)
src/support/allocators/zeroafterfree.h (2)
n(24-27)n(24-24)
🔇 Additional comments (25)
test/lint/spelling.ignore-words.txt (1)
13-13:✅ Verification successful
Validate the inclusion of "lief" in the ignore list.
The word “lief” has been added to the spelling ignore list. Please confirm that this term is intentionally used in the codebase (e.g., in test outputs or documentation) and not a typo. To verify its usage, you can run:
🏁 Script executed:
#!/bin/bash # Verify occurrences of "lief" in the repository rg -n "lief"Length of output: 8563
Confirmed “lief” is a library name and safe to ignore
Verified that “lief” appears consistently as the Python module name for the LIEF library across the repository, for example:
import liefin multiplecontrib/devtools/*.pyscripts- Package references in
contrib/guix/manifest.scmandci-slim.Dockerfile(lief==0.13.2)- The term is never a typo or stray word in source or docs
No changes needed.
src/init.cpp (1)
748-748: LGTM! Good documentation improvement.This change enhances the help text for
-rpcallowipby explicitly mentioning that "all ipv4 (0.0.0.0/0)" and "all ipv6 (::/0)" are valid values. The addition makes the documentation more comprehensive and user-friendly by clearly stating these common use cases that were previously implicit.test/functional/interface_usdt_utxocache.py (3)
193-202: LGTM! Improved error handling for BPF callback assertions.The try-except wrapper correctly addresses the issue where
AssertionErrorexceptions don't propagate properly from C callback functions. This ensures test failures are logged and the success counter only increments when all assertions pass.
268-277: LGTM! Consistent error handling pattern applied.The same try-except pattern is correctly applied to both
handle_utxocache_addandhandle_utxocache_spentfunctions, ensuring consistent error handling across all BPF callback handlers.Also applies to: 284-293
378-378: Good catch on the typo fix.Fixed "eariler" to "earlier" in the comment.
src/prevector.h (2)
289-293: LGTM! Improved move constructor with explicit semantics.The move constructor now uses explicit move semantics instead of swap, which is more efficient and provides better exception guarantees. The
noexceptspecification and proper cleanup (settingother._size = 0) ensure the moved-from object is left in a valid state.
303-311: LGTM! Proper move assignment with resource cleanup.The move assignment operator correctly:
- Frees existing indirect storage before move (preventing leaks)
- Uses explicit move semantics for
_union- Resets the source object to valid state
- Provides
noexceptguaranteeThis is a significant improvement over the previous swap-based approach.
src/validation.h (2)
41-41: LGTM! Required header for static assertions.The
<type_traits>include is necessary for thestd::is_nothrow_*type traits used in the static assertions below.
341-344: LGTM! Important exception safety guarantees for container usage.These static assertions ensure
CScriptCheckhas proper noexcept move semantics, which is crucial for efficient usage instd::vectorand other standard containers. This aligns with the broader PR improvements to move semantics and exception safety.src/bench/prevector.cpp (3)
110-119: LGTM! Well-designed benchmark for direct allocation.This benchmark tests the performance of moving default-constructed
prevectorobjects (which use direct allocation) into astd::vector. The use ofemplace_back()will exercise the move constructor improvements made inprevector.h.
122-132: LGTM! Comprehensive benchmark for indirect allocation.This benchmark tests the performance of moving
prevectorobjects that use heap allocation (size 29 > N=28) into astd::vector. The forced allocation withprevector<28, T>(29, T{})ensures testing of the indirect storage path and the improved move assignment operator.
154-155: LGTM! Proper integration with existing test framework.The new benchmarks are correctly integrated using the existing
PREVECTOR_TESTmacro, ensuring they will be tested with both trivial and non-trivial types as intended.src/support/allocators/secure.h (3)
21-26: LGTM! Modern allocator interface implementation.The refactoring correctly removes inheritance from
std::allocator<T>and implements the minimal allocator interface directly. Thevalue_typetypedef and simplified constructors follow modern C++ allocator best practices.
45-54: Correct implementation of allocator comparison operators.The equality and inequality operators are properly implemented as friend templates. Returning
truefor equality andfalsefor inequality is correct for stateless allocators, as all instances are considered equivalent.
28-28:✅ Verification successful
Verify removal of hint parameter doesn't break compatibility.
The
allocatemethod signature was simplified by removing the unusedhintparameter. This is correct for modern allocators, but ensure this doesn't break any existing usage.
🏁 Script executed:
#!/bin/bash # Description: Check if any code uses the allocate method with hint parameter # Expected: No usage of allocate with 2 parameters for secure_allocator rg -A 3 -B 3 "secure_allocator.*allocate.*," --type cppLength of output: 349
Allocate signature change is backward-compatible
A repository-wide search for two-argument calls toallocate(e.g.allocate(n, hint)) returned no hits forsecure_allocator. Removing the unusedhintparameter will not break existing code.src/support/allocators/zeroafterfree.h (4)
15-22: LGTM! Consistent allocator modernization.The refactoring correctly removes inheritance from
std::allocator<T>and implements the interface directly, consistent with the changes insecure.h. Thenoexceptspecification on the default constructor is appropriate for this stateless allocator.
24-27: Correct use of temporary allocator instance.The
allocatemethod correctly uses a temporarystd::allocator<T>{}instance to perform allocation. This approach maintains the same allocation behavior while avoiding inheritance.
33-34: Verify memory cleansing still occurs before deallocation.The
deallocatemethod correctly uses a temporarystd::allocator<T>{}instance and maintains the criticalmemory_cleansecall on line 32. The security property of zeroing memory before freeing is preserved.
36-45: Consistent comparison operator implementation.The equality and inequality operators follow the same pattern as
secure.h, correctly implementing stateless allocator semantics where all instances are considered equivalent.test/functional/interface_usdt_validation.py (3)
91-91: Good approach to collect events outside callbacks.Adding an
eventslist to collect callback data is the correct approach to ensure assertion errors propagate properly from ctypes callbacks to the Python test framework.
127-138: Excellent refactoring to process events outside callbacks.Moving the assertion logic to process collected events after polling ensures that assertion errors are properly caught and reported by the test framework. The logic correctly processes each event and maintains the same validation checks.
142-142: Proper cleanup timing after event processing.Moving
bpf.cleanup()to after the event verification loop ensures all events are processed before cleanup, which is the correct order of operations.test/functional/interface_usdt_net.py (3)
122-122: Consistent approach for event collection.Adding the
eventslist follows the same pattern as the validation test refactoring, ensuring proper assertion error propagation.
142-148: Clean callback refactoring to collect events.The callbacks are properly refactored to only collect events with direction information (inbound/outbound) rather than performing assertions directly. This ensures ctypes callback assertion errors propagate correctly.
159-161: Proper event processing after collection.The event processing loop correctly calls
check_p2p_messagefor each collected event, ensuring all assertions are performed in the main test context where errors will be properly caught and reported.
| assert_equal(block_1_coinbase_txid, bytes(event.txid[::-1]).hex()) | ||
| assert_equal(0, event.index) # prevout index | ||
| assert_equal(EARLY_BLOCK_HEIGHT, event.height) | ||
| assert_equal(50 * COIN, event.value) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the corrected expected value.
The expected value was changed from 500 * COIN to 50 * COIN. According to the AI summary, this corrects an incorrect expectation.
🏁 Script executed:
#!/bin/bash
# Description: Verify the correct coinbase value for early blocks in Dash
# Expected: Should find references to 50 COIN as the early block reward
# Search for coinbase reward values in the codebase
rg -A 3 -B 3 "50.*COIN|COIN.*50" --type cpp --type py
# Look for consensus parameters related to block rewards
rg -A 5 -B 5 "GetBlockSubsidy|nSubsidy" src/Length of output: 53354
I’d like to inspect what EARLY_BLOCK_HEIGHT is set to in this test to confirm which block’s subsidy is being checked:
🏁 Script executed:
#!/bin/bash
grep -n -C3 "EARLY_BLOCK_HEIGHT" test/functional/interface_usdt_utxocache.pyLength of output: 828
Fix early block reward expectation
The test is running on regtest, where the initial block subsidy is 500 COIN—not 50 COIN. Please update the expected value accordingly:
• test/functional/interface_usdt_utxocache.py:197
- assert_equal(50 * COIN, event.value)
+ assert_equal(500 * COIN, event.value)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert_equal(50 * COIN, event.value) | |
| assert_equal(500 * COIN, event.value) |
🤖 Prompt for AI Agents
In test/functional/interface_usdt_utxocache.py at line 197, the expected
coinbase value is incorrectly set to 50 * COIN for the regtest environment,
where the initial block subsidy is actually 500 * COIN. Update the assertion to
expect 500 * COIN instead of 50 * COIN to match the correct early block reward
on regtest.
There was a problem hiding this comment.
Hi @UdjinM6 , Accidentally pushed additional commits to this branch after UTACK. I've removed them now and am requesting a review.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
src/bench/prevector.cpp
Outdated
| }); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: 27334: one extra line
…e ctor for `prevector` bfb9291 util: implement prevector's move ctor & move assignment (Martin Leitner-Ankerl) fffc86f test: CScriptCheck is used a lot in std::vector, make sure that's efficient (Martin Leitner-Ankerl) 81f6797 util: prevector's move ctor and move assignment is `noexcept` (Martin Leitner-Ankerl) d380d28 bench: Add benchmark for prevector usage in std::vector (Martin Leitner-Ankerl) Pull request description: `prevector`'s move assignment and move constructor were not `noexcept`, which makes it inefficient to use inside STL containers like `std::vector`. That's the case e.g. for `CScriptCheck`. This PR adds `noexcept`, and also implements the move assignment & ctor, which makes it quite a bit more efficient to use prevector in an std::vector. The PR also adds a benchmark which grows an `std::vector` by adding `prevector` objects to it. merge-base: | ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 6,440.29 | 155,272.42 | 0.2% | 40,713.01 | 20,473.84 | 1.989 | 7,132.01 | 0.2% | 0.44 | `PrevectorFillVectorDirectNontrivial` | 3,213.19 | 311,217.35 | 0.7% | 35,373.01 | 10,214.07 | 3.463 | 6,945.00 | 0.2% | 0.43 | `PrevectorFillVectorDirectTrivial` | 34,749.70 | 28,777.23 | 0.1% | 364,396.05 | 110,521.94 | 3.297 | 78,568.37 | 0.1% | 0.43 | `PrevectorFillVectorIndirectNontrivial` | 32,535.05 | 30,736.09 | 0.4% | 353,823.31 | 103,464.53 | 3.420 | 79,871.80 | 0.2% | 0.40 | `PrevectorFillVectorIndirectTrivial` util: prevector's move ctor and move assignment is `noexcept`: | ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 6,603.87 | 151,426.40 | 0.2% | 23,734.01 | 21,009.63 | 1.130 | 2,445.01 | 0.3% | 0.44 | `PrevectorFillVectorDirectNontrivial` | 1,980.93 | 504,813.15 | 0.1% | 13,784.00 | 6,304.32 | 2.186 | 2,258.00 | 0.3% | 0.44 | `PrevectorFillVectorDirectTrivial` | 19,110.54 | 52,327.15 | 0.1% | 139,816.41 | 51,987.72 | 2.689 | 28,512.18 | 0.1% | 0.43 | `PrevectorFillVectorIndirectNontrivial` | 12,334.37 | 81,074.27 | 0.7% | 125,655.12 | 39,253.58 | 3.201 | 27,854.46 | 0.2% | 0.44 | `PrevectorFillVectorIndirectTrivial` util: implement prevector's move ctor & move assignment | ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 5,262.66 | 190,018.01 | 0.2% | 20,157.01 | 16,745.26 | 1.204 | 2,445.01 | 0.3% | 0.44 | `PrevectorFillVectorDirectNontrivial` | 1,687.07 | 592,744.35 | 0.2% | 12,742.00 | 5,368.02 | 2.374 | 2,258.00 | 0.3% | 0.44 | `PrevectorFillVectorDirectTrivial` | 17,930.80 | 55,769.95 | 0.1% | 136,237.69 | 47,903.31 | 2.844 | 28,512.02 | 0.2% | 0.42 | `PrevectorFillVectorIndirectNontrivial` | 11,893.75 | 84,077.78 | 0.2% | 126,182.02 | 37,852.91 | 3.333 | 28,152.01 | 0.1% | 0.44 | `PrevectorFillVectorIndirectTrivial` As can be seen, mostly thanks to just `noexcept` the benchmark becomes about 2 times faster because `std::vector` can now use move operations instead of having to fall back to copy everything I had a look at how this change affects the other benchmarks, and they are all pretty much the same, the only noticable difference is `CCheckQueueSpeedPrevectorJob` goes from 364.56ns down to 346.21ns. ACKs for top commit: achow101: ACK bfb9291 jonatack: > Tested Approach ACK [bfb9291](bitcoin@bfb9291), ~ACK modulo re-verifying the implementation of the last commit. john-moffett: Approach ACK bfb9291 theStack: Tested and light code-review ACK bfb9291 Tree-SHA512: 242995d7cb2f8ebfa73177aa690a505f189d91edeb8cea3e34a41ca507c8cb17c65fe2a4e196fdafc5c6e89b2b2222627bfc9f5c316517de0857b7b5e9c58225
…locator 07c59ed Don't derive secure_allocator from std::allocator (Casey Carter) Pull request description: Giving the C++ Standard Committee control of the public interface of your type means they will break it. C++23 adds a new `allocate_at_least` member to `std::allocator`. Very bad things happen when, say, `std::vector` uses `allocate_at_least` from `secure_allocator`'s base to allocate memory which it then tries to free with `secure_allocator::deallocate`. (Discovered by microsoft/STL#3712, which will be reverted by microsoft/STL#3819 before it ships.) ACKs for top commit: jonatack: re-ACK 07c59ed no change since my previous ACK apart from squashing the commits achow101: ACK 07c59ed john-moffett: ACK 07c59ed Reviewed and tested. Performance appears unaffected in my environment. Tree-SHA512: 23606c40414d325f5605a9244d4dd50907fdf5f2fbf70f336accb3a2cb98baa8acd2972f46eab1b7fdec1d28a843a96b06083cd2d09791cda7c90ee218e5bbd5
…ssertions in bcc callback functions 61f4b9b Manage exceptions in bcc callback functions (virtu) Pull request description: Address bitcoin#27380 (and similar future issues) by handling failed `assert_equal()` assertions in bcc callback functions ### Problem Exceptions are not propagated in ctype callback functions used by bcc. This means an AssertionError exception raised by `assert_equal()` to signal a failed assertion is not getting caught and properly logged. Instead, the error is logged to stdout and execution of the callback stops. The current workaround to check whether all `assert_equal()` assertions in a callback succeeded is to increment a success counter after the assertions (which only gets incremented if none exception is raised and stops execution). Then, outside the callback, the success counter can be used to check whether a callback executed successfully. One issue with the described workaround is that when an exception occurs, there is no way of telling which of the `assert_equal()` statements caused the exception; moreover, there is no way of inspecting how the pieces of data that got compared in `assert_equal()` differed (often a crucial clue when debugging what went wrong). This problem is happening in bitcoin#27380: Sporadically, in the `mempool:rejected` test, execution does not reach the end of the callback function and the success counter is not incremented. Thus, the test fails when comparing the counter to its expected value of one. Without knowing which of the asserts failed any why it failed, this issue is hard to debug. ### Solution Two fixes come to mind. The first involves having the callback function make event data accessible outside the callback and inspecting the event using `assert_equal()` outside the callback. This solution still requires a counter in the callback in order to tell whether a callback was actually executed or if instead the call to perf_buffer_poll() timed out. The second fix entails wrapping all relevant `assert_equal()` statements inside callback functions into try-catch blocks and manually logging AssertionErrors. While not as elegant in terms of design, this approach can be more pragmatic for more complex tests (e.g., ones involving multiple events, events of different types, or the order of events). The solution proposed here is to select the most pragmatic fix on a case-by-case basis: Tests in `interface_usdt_net.py`, `interface_usdt_mempool.py` and `interface_usdt_validation.py` have been refactored to use the first approach, while the second approach was chosen for `interface_usdt_utxocache.py` (partly to provide a reference for the second approach, but mainly because the utxocache tests are the most intricate tests, and refactoring them to use the first approach would negatively impact their readability). Lastly, `interface_usdt_coinselection.py` was kept unchanged because it does not use `assert_equal()` statements inside callback functions. ACKs for top commit: 0xB10C: Reviewed the changes since my last review. ACK 61f4b9b. I've tested that the combined log contains both exceptions by modifying `interface_usdt_utxocache.py`. willcl-ark: utACK 61f4b9b stickies-v: utACK 61f4b9b Tree-SHA512: 85cdaabf370d4f09a9eab6af9ce7c796cd9d08cb91f38f021f71adda34c5f643331022dd09cadb95be2185dad6016c95cbb8942e41e4fbd566a49bf431c5141a
|
Hello @PastaPastaPasta requesting review |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/net_processing.cpp (1)
597-597: Excellent architectural refactoring using Options patternThis refactoring successfully consolidates peer manager configuration into a single Options struct, which provides several architectural benefits:
- Improved testability - Configuration can be easily injected during testing without relying on global state
- Better API design - Adding new options won't require changing constructor signatures
- Centralized configuration - All command-line argument reading happens in one place (likely in peerman_args.cpp)
- Type safety - Configuration validation can happen at Options construction time
This follows the best practice of dependency injection and makes the codebase more maintainable.
Also applies to: 783-783, 1943-1946, 1971-1971
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Makefile.am(2 hunks)src/init.cpp(3 hunks)src/net_processing.cpp(13 hunks)src/net_processing.h(1 hunks)src/node/peerman_args.cpp(1 hunks)src/node/peerman_args.h(1 hunks)src/node/txreconciliation.h(0 hunks)src/test/denialofservice_tests.cpp(4 hunks)src/test/util/setup_common.cpp(2 hunks)src/wallet/walletdb.cpp(0 hunks)src/wallet/walletdb.h(0 hunks)test/functional/p2p_invalid_messages.py(3 hunks)test/functional/p2p_leak_tx.py(2 hunks)
💤 Files with no reviewable changes (3)
- src/node/txreconciliation.h
- src/wallet/walletdb.h
- src/wallet/walletdb.cpp
✅ Files skipped from review due to trivial changes (4)
- src/Makefile.am
- src/test/denialofservice_tests.cpp
- src/node/peerman_args.h
- src/node/peerman_args.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/init.cpp
🧰 Additional context used
🪛 Ruff (0.11.9)
test/functional/p2p_invalid_messages.py
196-196: Undefined name from_hex
(F821)
🪛 Pylint (3.3.7)
test/functional/p2p_invalid_messages.py
[error] 196-196: Undefined variable 'from_hex'
(E0602)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
🔇 Additional comments (22)
src/test/util/setup_common.cpp (2)
24-24: LGTM: New include properly supports Options-based configuration.The include for
node/peerman_args.his correctly added to support the newApplyArgsManOptionsfunction used in the PeerManager initialization.
308-313: LGTM: PeerManager initialization successfully refactored to use Options struct.The change from passing a single boolean flag to using a structured
Optionsobject is a good architectural improvement. TheApplyArgsManOptionsfunction properly centralizes the argument parsing logic.test/functional/p2p_invalid_messages.py (1)
7-7: LGTM: Random import supports the new test functionality.The
randommodule is correctly imported to support random header selection in the new non-continuous headers test.src/net_processing.h (2)
57-64: LGTM: Options struct provides excellent architectural improvement.The new
Optionsstruct consolidates multiple configuration parameters into a single, well-structured interface. This is much more maintainable and extensible than passing individual parameters. The default values are appropriately set using existing constants.
73-73: LGTM: Updated method signature supports the new Options-based configuration.The
makemethod signature correctly accepts the newOptionsstruct instead of the previous boolean flag, maintaining consistency with the architectural refactoring.test/functional/p2p_leak_tx.py (5)
5-5: LGTM: Updated docstring reflects broader test scope.The change from "Test P2P transaction leaking" to "Test transaction upload" better reflects the expanded scope of the test which now covers both transaction upload verification and leak prevention.
26-30: LGTM: Instance variables improve code organization.Adding
self.gen_nodeandself.miniwalletas instance variables eliminates code duplication and makes the test methods cleaner by avoiding repetitive setup.
32-33: LGTM: Test method separation improves focus and maintainability.Splitting the monolithic test into
test_tx_in_block()andtest_notfound_on_unannounced_tx()creates focused, single-responsibility test methods that are easier to understand and maintain.
35-50: LGTM: Well-structured test for transaction upload verification.The test correctly verifies that transactions in blocks can be uploaded to peers, which is beneficial for compact block relay. The logic flow is clear: generate transaction, wait for inventory announcement, mine block, request transaction, and verify response.
51-77: LGTM: Robust test for preventing transaction leaks.The test methodology is sound - it repeatedly creates transactions and requests them from an inbound peer until receiving a "notfound" response, confirming that unannounced transactions are properly protected from leaking. The MAX_REPEATS approach handles the probabilistic nature of the test effectively.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 67-76: Unnecessary "else" after "break", remove the "else" and de-indent the code inside it
(R1723)
src/net_processing.cpp (12)
597-597: Good refactoring to use Options structThe change from individual parameters to an Options struct improves API extensibility and maintainability.
624-624: Correct adaptation of getter to Options structThe method correctly returns the value from the new Options struct, maintaining the same functionality.
783-783: Well-designed member variable consolidationReplacing individual configuration members with a const Options struct improves encapsulation and maintainability.
1241-1241: Consistent usage of Options structThe condition correctly uses the new Options struct member while preserving the original logic.
1766-1772: Excellent centralization of configurationReplacing direct command-line argument access with Options struct values improves testability and removes global state dependencies. This makes the code more modular and easier to unit test.
1943-1946: Clean Options pattern implementationThe factory method and constructor correctly propagate the Options struct, maintaining a clean and consistent API.
Also applies to: 1956-1956, 1971-1971
1975-1975: Proper centralization of txreconciliation configUsing the Options struct value instead of reading command-line arguments directly is consistent with the refactoring pattern.
3051-3051: Correct Options usage in block fetching logicThe condition properly uses the Options struct member while maintaining the original behavior.
3682-3682: Proper Options usage in version handshakeThe txreconciliation logic correctly uses the Options struct member during version message processing.
4544-4544: Good centralization of orphan tx limitUsing the Options struct value for orphan transaction limits removes global state dependency and improves testability.
5391-5391: Proper centralization of message capture configThe message capture configuration correctly uses the Options struct, removing direct command-line argument access.
5743-5743: Consistent Options usage in tx rejection logicThe transaction rejection logic properly uses the Options struct member.
| def test_noncontinuous_headers_msg(self): | ||
| self.log.info("Test headers message with non-continuous headers sequence is logged as misbehaving") | ||
| block_hashes = self.generate(self.nodes[0], 10) | ||
| block_headers = [] | ||
| for block_hash in block_hashes: | ||
| block_headers.append(from_hex(CBlockHeader(), self.nodes[0].getblockheader(block_hash, False))) | ||
|
|
||
| # continuous headers sequence should be fine | ||
| MISBEHAVING_NONCONTINUOUS_HEADERS_MSGS = ['Misbehaving', 'non-continuous headers sequence'] | ||
| peer = self.nodes[0].add_p2p_connection(P2PInterface()) | ||
| with self.nodes[0].assert_debug_log([], unexpected_msgs=MISBEHAVING_NONCONTINUOUS_HEADERS_MSGS): | ||
| peer.send_and_ping(msg_headers(block_headers)) | ||
|
|
||
| # delete arbitrary block header somewhere in the middle to break link | ||
| del block_headers[random.randrange(1, len(block_headers)-1)] | ||
| with self.nodes[0].assert_debug_log(expected_msgs=MISBEHAVING_NONCONTINUOUS_HEADERS_MSGS): | ||
| peer.send_and_ping(msg_headers(block_headers)) | ||
| self.nodes[0].disconnect_p2ps() | ||
|
|
There was a problem hiding this comment.
Fix undefined variable and improve test robustness.
The test logic for validating non-continuous headers behavior is sound, but there's a critical issue:
- for block_hash in block_hashes:
- block_headers.append(from_hex(CBlockHeader(), self.nodes[0].getblockheader(block_hash, False)))
+ for block_hash in block_hashes:
+ header_hex = self.nodes[0].getblockheader(block_hash, False)
+ header = CBlockHeader()
+ header.deserialize(BytesIO(bytes.fromhex(header_hex)))
+ block_headers.append(header)You'll need to add this import at the top:
+from io import BytesIOThe test methodology is excellent - verifying that continuous headers don't trigger misbehavior logs, then confirming that non-continuous headers do trigger the expected misbehavior response.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_noncontinuous_headers_msg(self): | |
| self.log.info("Test headers message with non-continuous headers sequence is logged as misbehaving") | |
| block_hashes = self.generate(self.nodes[0], 10) | |
| block_headers = [] | |
| for block_hash in block_hashes: | |
| block_headers.append(from_hex(CBlockHeader(), self.nodes[0].getblockheader(block_hash, False))) | |
| # continuous headers sequence should be fine | |
| MISBEHAVING_NONCONTINUOUS_HEADERS_MSGS = ['Misbehaving', 'non-continuous headers sequence'] | |
| peer = self.nodes[0].add_p2p_connection(P2PInterface()) | |
| with self.nodes[0].assert_debug_log([], unexpected_msgs=MISBEHAVING_NONCONTINUOUS_HEADERS_MSGS): | |
| peer.send_and_ping(msg_headers(block_headers)) | |
| # delete arbitrary block header somewhere in the middle to break link | |
| del block_headers[random.randrange(1, len(block_headers)-1)] | |
| with self.nodes[0].assert_debug_log(expected_msgs=MISBEHAVING_NONCONTINUOUS_HEADERS_MSGS): | |
| peer.send_and_ping(msg_headers(block_headers)) | |
| self.nodes[0].disconnect_p2ps() | |
| # at the top of the file | |
| from io import BytesIO | |
| def test_noncontinuous_headers_msg(self): | |
| self.log.info("Test headers message with non-continuous headers sequence is logged as misbehaving") | |
| block_hashes = self.generate(self.nodes[0], 10) | |
| block_headers = [] | |
| for block_hash in block_hashes: | |
| header_hex = self.nodes[0].getblockheader(block_hash, False) | |
| header = CBlockHeader() | |
| header.deserialize(BytesIO(bytes.fromhex(header_hex))) | |
| block_headers.append(header) | |
| # continuous headers sequence should be fine | |
| MISBEHAVING_NONCONTINUOUS_HEADERS_MSGS = ['Misbehaving', 'non-continuous headers sequence'] | |
| peer = self.nodes[0].add_p2p_connection(P2PInterface()) | |
| with self.nodes[0].assert_debug_log([], unexpected_msgs=MISBEHAVING_NONCONTINUOUS_HEADERS_MSGS): | |
| peer.send_and_ping(msg_headers(block_headers)) | |
| # delete arbitrary block header somewhere in the middle to break link | |
| del block_headers[random.randrange(1, len(block_headers)-1)] | |
| with self.nodes[0].assert_debug_log(expected_msgs=MISBEHAVING_NONCONTINUOUS_HEADERS_MSGS): | |
| peer.send_and_ping(msg_headers(block_headers)) | |
| self.nodes[0].disconnect_p2ps() |
🧰 Tools
🪛 Ruff (0.11.9)
196-196: Undefined name from_hex
(F821)
🪛 Pylint (3.3.7)
[error] 196-196: Undefined variable 'from_hex'
(E0602)
🤖 Prompt for AI Agents
In test/functional/p2p_invalid_messages.py around lines 191 to 209, the variable
'random' is used but not imported, causing a NameError. To fix this, add 'import
random' at the top of the file. This will ensure the deletion of a random block
header works correctly and the test runs without errors.
backport