Skip to content

Conversation

mzumsande
Copy link
Contributor

This test failed for me on master locally:
The reason is that when initiating a v2 connection and being immediately disconnected, a node cannot know if the disconnect happens because the peer only supports v1, or because it has banned you, so it schedules to reconnect with v1. If the test doesn't wait for that, the reconnect can happen at a bad time, resulting in failure in a later connect_nodes call.
Also add the test with --v2transport to the test runner because banning with v2 seems like a useful thing to have test coverage for.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 2, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK delta1, stratospher, achow101
Concept ACK epiccurious

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26863 (test: merge banning test from p2p_disconnect_ban to rpc_setban by brunoerg)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

When initiating a v2 connection and being immediately disconnected,
a node cannot know if the disconnect happens because the peer only
supports v1, or because it has banned you, so it schedules to reconnect with v1.
If the test doesn't wait for that, the reconnect can happen at a bad time,
resulting in failure in a later connect_nodes call.
Also add the test with --v2transport to the test runner.
@mzumsande mzumsande force-pushed the 202402_fix_setban_v2transport branch from 8538c04 to cc87ee4 Compare February 2, 2024 18:24
@mzumsande mzumsande changed the title fix intermittent failure in rpc_setban.py --v2transport, run it in CI test: fix intermittent failure in rpc_setban.py --v2transport, run it in CI Feb 2, 2024
@DrahtBot DrahtBot added the Tests label Feb 2, 2024
@delta1
Copy link

delta1 commented Feb 4, 2024

tested ACK cc87ee4

Copy link
Contributor

@stratospher stratospher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested ACK cc87ee4. nice find!

@edilmedeiros
Copy link
Contributor

edilmedeiros commented Feb 5, 2024

I don't think this is something to stall a merge, but I could not make the test fail in master. I tried with different loads in my machine from other work.

How are you calling it?

❯ python test/functional/rpc_setban.py
2024-02-05T17:48:04.516000Z TestFramework (INFO): PRNG seed is: 2620849867157253531
2024-02-05T17:48:04.517000Z TestFramework (INFO): Initializing test directory /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_tqvcwu8u
2024-02-05T17:48:06.209000Z TestFramework (INFO): Test that a non-IP address can be banned/unbanned
2024-02-05T17:48:06.210000Z TestFramework (INFO): Test the ban list is preserved through restart
2024-02-05T17:48:06.890000Z TestFramework (INFO): Test -bantime
2024-02-05T17:48:07.369000Z TestFramework (INFO): Stopping nodes
2024-02-05T17:48:07.477000Z TestFramework (INFO): Cleaning up /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_tqvcwu8u on exit
2024-02-05T17:48:07.477000Z TestFramework (INFO): Tests successful

@mzumsande
Copy link
Contributor Author

I could not make the test fail in master

❯ python test/functional/rpc_setban.py

You have to call it with v2, see PR title: rpc_setban.py --v2transport.

@edilmedeiros
Copy link
Contributor

edilmedeiros commented Feb 5, 2024

I'm sorry for not being clear in my comment.
I tried that with your code, and it runs something that passes (see below).

My point is that running rpc_setban.py in 5b8c597 does not seem to fail in any way in my box.
The mentioned commit passed the CI tests.
I understand I'm a newcomer, and I'm trying to learn how the tests work by reproducing the error you found instead of just reading the code in the PR.

Yet, I don't see this as a problem to merge.
It might be an issue triggered by some specificity in your box, but not mine.

❯ python test/functional/rpc_setban.py
2024-02-05T18:36:28.549000Z TestFramework (INFO): PRNG seed is: 7243494276342072324
2024-02-05T18:36:28.550000Z TestFramework (INFO): Initializing test directory /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_w4vk2rv2
2024-02-05T18:36:30.718000Z TestFramework (INFO): Test that a non-IP address can be banned/unbanned
2024-02-05T18:36:30.719000Z TestFramework (INFO): Test the ban list is preserved through restart
2024-02-05T18:36:31.403000Z TestFramework (INFO): Test -bantime
2024-02-05T18:36:31.882000Z TestFramework (INFO): Stopping nodes
2024-02-05T18:36:31.993000Z TestFramework (INFO): Cleaning up /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_w4vk2rv2 on exit
2024-02-05T18:36:31.993000Z TestFramework (INFO): Tests successful
❯ python test/functional/rpc_setban.py --v2transport
2024-02-05T18:36:38.617000Z TestFramework (INFO): PRNG seed is: 3082398001401933771
2024-02-05T18:36:38.618000Z TestFramework (INFO): Initializing test directory /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_lj9gxqtg
2024-02-05T18:36:41.687000Z TestFramework (INFO): Test that a non-IP address can be banned/unbanned
2024-02-05T18:36:41.688000Z TestFramework (INFO): Test the ban list is preserved through restart
2024-02-05T18:36:42.371000Z TestFramework (INFO): Test -bantime
2024-02-05T18:36:42.850000Z TestFramework (INFO): Stopping nodes
2024-02-05T18:36:42.961000Z TestFramework (INFO): Cleaning up /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_lj9gxqtg on exit
2024-02-05T18:36:42.961000Z TestFramework (INFO): Tests successful

@epiccurious
Copy link
Contributor

Concept ACK cc87ee4.

@achow101
Copy link
Member

achow101 commented Feb 8, 2024

ACK cc87ee4

@achow101 achow101 merged commit 5cdf313 into bitcoin:master Feb 8, 2024
@mzumsande mzumsande deleted the 202402_fix_setban_v2transport branch February 9, 2024 20:54
kwvg added a commit to kwvg/dash that referenced this pull request Oct 15, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Oct 23, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Oct 23, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Oct 23, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Oct 24, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Oct 24, 2024
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 24, 2024
, 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
@bitcoin bitcoin locked and limited conversation to collaborators Feb 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants