rpc, test: add sendmsgtopeer rpc and a test for net-level deadlock situation #28287

pull mzumsande wants to merge 3 commits into bitcoin:master from mzumsande:202208_test_sendmsg changing 6 files +121 −0
  1. mzumsande commented at 6:29 pm on August 17, 2023: contributor

    This adds a sendmsgtopeer rpc (for testing only) that allows a node to send a message (provided in hex) to a peer. While we would usually use a p2p object instead of a node for this in the test framework, that isn’t possible in situations where this message needs to trigger an actual interaction of multiple nodes.

    Use this rpc to add test coverage for the bug fixed in #27981 (that just got merged): The test lets two nodes (almost) simultaneously send a single large (4MB) p2p message to each other, which would have caused a deadlock previously (making this test fail), but succeeds now.

    As can be seen from the discussion in #27981, it was not easy to reproduce this bug without sendmsgtopeer. I would imagine that sendmsgtopeer could also be helpful in various other test constellations.

  2. DrahtBot commented at 6:29 pm on August 17, 2023: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK ajtowns, sipa, achow101
    Concept ACK russeree
    Stale ACK brunoerg

    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:

    • #27511 (rpc: Add test-only RPC getaddrmaninfo for new/tried table address count by stratospher)
    • #26988 (cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency by stratospher)

    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.

  3. mzumsande force-pushed on Aug 17, 2023
  4. DrahtBot added the label CI failed on Aug 17, 2023
  5. mzumsande force-pushed on Aug 17, 2023
  6. brunoerg commented at 7:05 pm on August 17, 2023: contributor
    Concept ACK
  7. ajtowns commented at 4:54 am on August 18, 2023: contributor
    Concept ACK. Probably should add some basic functional tests to rpc_net.py with a P2PInterface checking that it works – what happens if msg_type is 0 bytes, 13 bytes (> protocol.h:COMMAND_SIZE); what if msg is empty or more than 4MB (net.h:MAX_PROTOCOL_MESSAGE_LENGTH) or 32MB (serialize.h:MAX_SIZE)?
  8. in test/functional/p2p_net_deadlock.py:33 in 7b76e09e14 outdated 
    28+
29+        self.log.info("Simultaneously send a large message on both sides")
30+        rand_msg = random_bytes(4000000).hex()
31+
32+        thread0 = threading.Thread(target=send_message, args=(node0, 0, "unknown", rand_msg))
33+        thread1 = threading.Thread(target=send_message, args=(node1, 0, "unknown", rand_msg))
    maflcko commented at 5:43 am on August 18, 2023: 
    0        thread1 = threading.Thread(target=node1.send_message, args=(0, "unknown", rand_msg))

    Not sure if it works, but maybe you can drop the wrapper function?

    brunoerg commented at 1:00 pm on August 18, 2023:
    It works. But in the case it would be target=node1.sendmsgtopeer instead of target=node1.send_message.
    mzumsande commented at 8:04 pm on August 21, 2023:
    thanks, used target=node1.sendmsgtopeer and removed the wrapper
  9. in test/functional/p2p_net_deadlock.py:2 in 7b76e09e14 outdated 
    0@@ -0,0 +1,46 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2023-The Bitcoin Core developers
    maflcko commented at 5:47 am on August 18, 2023: 
    0# Copyright (c) 2023-present The Bitcoin Core developers

    nit: Either remove the typo, or remove the year, or append present to avoid having to “bump” this newly added file ever again.

    mzumsande commented at 8:03 pm on August 21, 2023:
    used 2023-present as suggested
  10. in src/rpc/net.cpp:994 in 7b76e09e14 outdated 
    989+    const NodeId peer_id{request.params[0].getInt<int64_t>()};
990+    const std::string& msg_type{request.params[1].get_str()};
991+    if( msg_type.size() > CMessageHeader::COMMAND_SIZE) {
992+        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Error: msg_type too long, max is (%i)", CMessageHeader::COMMAND_SIZE));
993+    }
994+    const std::vector<unsigned char> msg{ParseHex(request.params[2].get_str())};
    maflcko commented at 5:50 am on August 18, 2023:
    nit: Use TryParseHex?
    mzumsande commented at 8:03 pm on August 21, 2023:
    done, and added a JSONRPCError if parsing fails.
  11. in src/rpc/net.cpp:976 in 7b76e09e14 outdated 
    967@@ -968,6 +968,53 @@ static RPCHelpMan addpeeraddress()
968     };
969 }
970 
971+static RPCHelpMan sendmsgtopeer()
972+{
973+    return RPCHelpMan{"sendmsgtopeer",
974+                "\nSend a p2p message to a peer specified by id.\n"
975+                "The message type and body must be provided, the message header will be generated"
976+                "This RPC is for testing only\n",
    maflcko commented at 5:51 am on August 18, 2023: 
    0                "Send a p2p message to a peer specified by id.\n"
1                "The message type and body must be provided, the message header will be generated.\n"
2                "This RPC is for testing only.",

    nit

    mzumsande commented at 8:02 pm on August 21, 2023:
    done
  12. in src/rpc/net.cpp:1017 in 7b76e09e14 outdated 
    1011+
1012+    UniValue ret{UniValue::VOBJ};
1013+    return ret;
1014+},
1015+    };
1016+}
    maflcko commented at 5:53 am on August 18, 2023:
    nit: Clang-format-diff new code?
    mzumsande commented at 8:02 pm on August 21, 2023:
    For some reason I always thought that clang-format doesn’t apply to rpc indentation (because most of the existing ones don’t comply with it), but that doesn’t seem to be the case, so I’ve applied it.
  13. maflcko approved
  14. maflcko commented at 5:53 am on August 18, 2023: member
    lgtm. Feel free to ignore the nits.
  15. maflcko commented at 5:54 am on August 18, 2023: member
    Error: RPC command "sendmsgtopeer" not found in RPC_COMMANDS_SAFE_FOR_FUZZING or RPC_COMMANDS_NOT_SAFE_FOR_FUZZING. Please update test/fuzz/rpc.cpp.
  16. in src/rpc/net.cpp:991 in 7b76e09e14 outdated 
    986+                },
987+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
988+{
989+    const NodeId peer_id{request.params[0].getInt<int64_t>()};
990+    const std::string& msg_type{request.params[1].get_str()};
991+    if( msg_type.size() > CMessageHeader::COMMAND_SIZE) {
    brunoerg commented at 6:56 pm on August 18, 2023:

    nit

    0    if (msg_type.size() > CMessageHeader::COMMAND_SIZE) {
    mzumsande commented at 7:58 pm on August 21, 2023:
    fixed
  17. russeree commented at 9:44 pm on August 20, 2023: contributor
    Concept ACK this would be useful.
  18. in test/functional/p2p_net_deadlock.py:26 in 7b76e09e14 outdated 
    21+        self.setup_nodes()
22+
23+    def run_test(self):
24+        node0 = self.nodes[0]
25+        node1 = self.nodes[1]
26+        self.connect_nodes(0, 1)
    brunoerg commented at 12:19 pm on August 21, 2023:

    I think you don’t need to override setup_network:

     0diff --git a/test/functional/p2p_net_deadlock.py b/test/functional/p
 12p_net_deadlock.py
 2index 1b2139cc8..4ed9170e4 100755
 3--- a/test/functional/p2p_net_deadlock.py
 4+++ b/test/functional/p2p_net_deadlock.py
 5@@ -17,14 +17,9 @@ class NetDeadlockTest(BitcoinTestFramework):
 6         self.setup_clean_chain = True
 7         self.num_nodes = 2
 8 
 9-    def setup_network(self):
10-        self.setup_nodes()
11-
12     def run_test(self):
13         node0 = self.nodes[0]
14         node1 = self.nodes[1]
15-        self.connect_nodes(0, 1)
16-        self.sync_all()
17 
18         self.log.info("Simultaneously send a large message on both sides")
19         rand_msg = random_bytes(4000000).hex()
    mzumsande commented at 7:58 pm on August 21, 2023:
    done
  19. mzumsande force-pushed on Aug 21, 2023
  20. mzumsande force-pushed on Aug 21, 2023
  21. mzumsande commented at 8:19 pm on August 21, 2023: contributor

    Thanks for the reviews! I pushed an update with changes due to feedback, haven’t addressed @ajtowns suggestion yet:

    Concept ACK. Probably should add some basic functional tests to rpc_net.py with a P2PInterface checking that it works – what happens if msg_type is 0 bytes, 13 bytes (> protocol.h:COMMAND_SIZE); what if msg is empty or more than 4MB (net.h:MAX_PROTOCOL_MESSAGE_LENGTH) or 32MB (serialize.h:MAX_SIZE)?

    Will add this to rpc_net.py soon. I think the best approach might be to allow these kinds of things things (e.g. empty msg_type, message body > 4MB) even if they don’t make much sense - as long as they can be dealt with locally and don’t trigger any asserts, like the oversized msg_type does.

    Error: RPC command “sendmsgtopeer” not found in RPC_COMMANDS_SAFE_FOR_FUZZING or RPC_COMMANDS_NOT_SAFE_FOR_FUZZING. Please update test/fuzz/rpc.cpp.

    updated with the same comment getblockfrompeer has, indicating that no msg will be sent when there are no peers.

  22. DrahtBot removed the label CI failed on Aug 21, 2023
  23. brunoerg approved
  24. brunoerg commented at 11:53 pm on August 21, 2023: contributor
    utACK d1404b344d5907d159502a2cb05aa0ad557ebb8f
  25. rpc: add test-only sendmsgtopeer rpc 
    This rpc can be used when we want a node to send a message, but
cannot use a python P2P object, for example for testing of low-level
net transport behavior.
    a9a1d69391
  26. test: add basic tests for sendmsgtopeer to rpc_net.py 3557aa4d0a
  27. mzumsande force-pushed on Aug 22, 2023
  28. mzumsande commented at 5:36 pm on August 22, 2023: contributor

    I added a commit for basic coverage in rpc_net.py and made some other minor changes.

    what if msg is empty or more than 4MB (net.h:MAX_PROTOCOL_MESSAGE_LENGTH) or 32MB (serialize.h:MAX_SIZE)?

    I added coverage for >4MB. I couldn’t test the case >32MB, because already at ~16MB I get a failure passing a string that long via RPC (independent on the actual RPC, so this can’t be handled within sendmsgtopeer).

  29. test: add functional test for deadlock situation b3a93b409e
  30. mzumsande force-pushed on Aug 22, 2023
  31. ajtowns commented at 12:17 pm on August 23, 2023: contributor

    ACK b3a93b409e7fb33af77bd34a269a3eae71d3ba83

    LGTM. bitcoin-cli sendmsgtopeer 0 mempool "" to a peer that has you whitelisted works as a poor man’s importmempool (#27460) which is neat. Confirm that p2p_net_deadlock test catches the bug if #27981 is reverted (and running netstat -n confirms the tcp buffers are just sitting there being ignored while the test case is running).

  32. DrahtBot requested review from brunoerg on Aug 23, 2023
  33. glozow added the label Tests on Aug 24, 2023
  34. glozow added the label RPC/REST/ZMQ on Aug 24, 2023
  35. sipa commented at 5:47 pm on August 24, 2023: member

    ACK b3a93b409e7fb33af77bd34a269a3eae71d3ba83

    Two non-blocking comments:

    • Maybe add a comment to the p2p_net_deadlock.py that explains what issue it is supposed to address (perhaps by linking to the PR that fixed it).
    • The RPC added does not check whether the msg_type consists of valid characters (see CMessageHeader::IsCommandValid()). Currently, such messages are ignored by the receiver. Does it make sense to add a check the RPC, or a test that sending such messages works?
  36. achow101 commented at 9:28 pm on August 24, 2023: member
    ACK b3a93b409e7fb33af77bd34a269a3eae71d3ba83
  37. achow101 merged this on Aug 24, 2023
  38. achow101 closed this on Aug 24, 2023
  39. Frank-GER referenced this in commit 74aeda39e2 on Sep 8, 2023
  40. mzumsande deleted the branch on Nov 7, 2023


mzumsande DrahtBot brunoerg ajtowns maflcko russeree sipa achow101


brunoerg

Labels
Tests RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-06-29 07:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me