-
Notifications
You must be signed in to change notification settings - Fork 37.7k
test: assert rpc error for addnode v2transport not enabled #29460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: assert rpc error for addnode v2transport not enabled #29460
Conversation
Added coverage for the addnode rpc when v2transport is not enabled but is set as true when calling addnode rpc
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm ACK 345169a
Checked coverage per https://corecheck.dev/bitcoin/bitcoin/pulls/29460
@@ -12,6 +12,7 @@ | |||
from test_framework.util import ( | |||
assert_equal, | |||
p2p_port, | |||
assert_raises_rpc_error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert_raises_rpc_error | |
assert_raises_rpc_error, |
nit (only if you re-touch for other reasons): Please always add the trailing comma, to avoid having to touch this line again in the future when a new item is added to the list. Also, could sort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 345169a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review ACK 345169a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-review ACK 345169a
…ot enabled 345169a test: assert rpc error for addnode v2transport not enabled (kevkevin) Pull request description: Added coverage for the `addnode` rpc when v2transport is not enabled, but is set as true when calling `addnode` rpc. I ran the following to check if this rpc error message was covered in the functional tests. `grep -nr "v2transport requested but not enabled" ./test/functional --binary-files=without-match` Adds test coverage to this line. https://github.com/bitcoin/bitcoin/blob/master/src/rpc/net.cpp#L339 ACKs for top commit: maflcko: lgtm ACK 345169a brunoerg: utACK 345169a BrandonOdiwuor: Code Review ACK 345169a theStack: Code-review ACK 345169a Tree-SHA512: fb82409485efe25a1193b1dafca8ae694b397a301bb8bcb33c7572d21ff244ee45fbbd4364141e9421733873b343554a34614a59b1450ce0cac5c420203c3d35
, bitcoin#29372, bitcoin#29460, bitcoin#29358, bitcoin#29511, bitcoin#29390, bitcoin#29431, bitcoin-core/gui#788 (BIP324 backports: part 4) 4735b82 merge bitcoin#29431: disconnection scenarios during v2 handshake (Kittywhiskers Van Gogh) cc6b88e merge bitcoin-core/gui#788: update session ID tooltip (Kittywhiskers Van Gogh) 2455862 merge bitcoin#29390: speedup bip324_cipher.py unit test (Kittywhiskers Van Gogh) 062aaf1 merge bitcoin#29511: Fix intermittent failure in rpc_net.py --v2transport (Kittywhiskers Van Gogh) 54972e8 merge bitcoin#29358: use v2 everywhere for P2PConnection if --v2transport is enabled (Kittywhiskers Van Gogh) 4cce72f test: add missing debug log assertion in `p2p_invalid_messages.py` (Kittywhiskers Van Gogh) bd2fe61 merge bitcoin#29460: assert rpc error for addnode v2transport not enabled (Kittywhiskers Van Gogh) 5ee15fa merge bitcoin#29372: fix intermittent failure in `rpc_setban.py --v2transport` (Kittywhiskers Van Gogh) e278818 merge bitcoin#29352: fix intermittent failure in p2p_v2_earlykeyresponse (Kittywhiskers Van Gogh) 6b2a8b5 merge bitcoin#24748: functional tests for v2 P2P encryption (Kittywhiskers Van Gogh) 32500f2 merge bitcoin#27653: add unit test coverage for Python ECDSA implementation (Kittywhiskers Van Gogh) 9f476c6 net: add Dash network message short IDs, allocate range 128 onwards (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6329 * Dash-specific P2P messages have been allocated short IDs after 128 based on a prior suggestion ([source](#6280 (comment))) as there are 255 potential short IDs (ID `0` is reserved for V1 fallback, [source](https://github.com/dashpay/dash/blob/a7bbcc823d800b293e9ec54dff518fa9929c763c/src/net.cpp#L1019)) and upstream uses 28 short IDs (though Dash has left ID `5` blank as we do not implement the `FEEFILTER` message, [source](https://github.com/dashpay/dash/blob/a7bbcc823d800b293e9ec54dff518fa9929c763c/src/net.cpp#L1024)). As it is unlikely that upstream will utilize more than 127 short IDs (and the spec refers to IDs after 32 as "undefined", [source](https://github.com/bitcoin/bips/blob/52894e1aa7af27f4ae7836a391643124c71e2639/bip-0324.mediawiki#v2-bitcoin-p2p-message-structure)), there shouldn't be an adverse effect to utilizing IDs >=128. The unified array of short IDs are accessible through `V2ShortIDs()`. * As there are checks to see if an ID *can* be valid by checking against the array size (which wouldn't work here as we create an array of 256 entries combining both upstream and Dash's allocated short IDs, filling the rest with blank values and we cannot ignore blank values to know if a value is unallocated as the blanking could also signal a reservation, [source](https://github.com/dashpay/dash/blob/a7bbcc823d800b293e9ec54dff518fa9929c763c/src/net.cpp#L1048-L1052)), such a check needs to be done by using `IsValidV2ShortID()`. * `V2ShortIDs()` isn't as elegant as desired as `std::fill` and `std::copy` are not `constexpr` until C++20 ([source](https://en.cppreference.com/w/cpp/algorithm/fill), [source](https://en.cppreference.com/w/cpp/algorithm/copy)) and until we drop C++17 support, we have to be mindful of that. * `masternode connect` will now _attempt_ to establish a P2Pv2 connection if the node *initiating* the connection has opted-in using the new argument (`v2transport`) and the node was started with P2Pv2 enabled (using the launch argument, `-v2transport`). This mirrors changes to behavior to `addconnection` introduced in [bitcoin#24748](bitcoin#24748) * The oversized payload test in `p2p_invalid_messages.py` will expect an excessively large message of size of `3145729` bytes (and in P2Pv2, `3145742` bytes), as opposed to upstream's `4000001` and `4000014` bytes respectively as Dash has a lower protocol limit of 3MiB ([source](https://github.com/dashpay/dash/blob/a7bbcc823d800b293e9ec54dff518fa9929c763c/src/net.h#L80-L81)) vs Bitcoin's 4MB ([source](https://github.com/bitcoin/bitcoin/blob/225718eda89d65a7041c33e1994d3bf7bd71bbdd/src/net.h#L62-L63)) ## Breaking Changes None expected. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 4735b82 UdjinM6: light ACK 4735b82 Tree-SHA512: 4a9d586133633c109e6a8f20a6c6c5e4b24185fb616bcd8568e546b6e9f886e7a8707e811fd070bbe32c40df269f89a56343a24b67242c6147f9df27275af599
Added coverage for the
addnode
rpc when v2transport is not enabled,but is set as true when calling
addnode
rpc.I ran the following to check if this rpc error message
was covered in the functional tests.
grep -nr "v2transport requested but not enabled" ./test/functional --binary-files=without-match
Adds test coverage to this line.
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/net.cpp#L339