Refactor and slightly stricter p2p message processing #15197

jonasschnelli · 2019-01-18T07:04:37Z

There are currently redundant checks of the network magic and the MAX_SIZE which makes the code more difficult to read.

This moves and refactors network magic and message header verification from ProcessMessages() to the deserialisation of a network message.

Slightly stricter because...
... directly disconnect when network magic is invalid even before reading the rest of the message
... disconnect directly rather then skipping a message when a command string in invalid (not all zeros after the first zero, invalid chars)

Simplifies possible p2p protocol upgrades like BIP151

src/net.cpp

src/protocol.cpp

laanwj · 2019-01-18T07:34:57Z

Concept ACK, it is good to reject an invalid message as soon as possible.

I think this is correct

CNetMessage::readHeader for every message called before ::ProcessMessages
CMessageHeader::IsValid does the following checks, which includes checking the message size
- Check message start
- Check command string for errors
- Check message size

laanwj · 2019-01-21T16:35:55Z

utACK c749b13

naumenkogs · 2019-01-21T19:46:51Z

utACK c749b13

Perhaps also replace 24 with HEADER_SIZE here? (twice)

bitcoin/src/net.cpp

Line 820 in c749b13

unsigned int nRemaining = 24 - nHdrPos;

jonasschnelli · 2019-01-24T19:32:16Z

Perhaps also replace 24 with HEADER_SIZE here? (twice)

It should be replaced but its irrelevant for this PR and not within the scope of it (will do later).

laanwj · 2019-01-31T13:50:15Z

p2p_invalid_messages is failing locally on this, probably due to something else merged in the meantime?

$ test/functional/test_runner.py p2p_invalid_messages.py
Temporary test directory at /tmp/test_runner_₿_🏃_20190131_144734
1/1 - p2p_invalid_messages.py failed, Duration: 1 s

stdout:
2019-01-31T13:47:34.927000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20190131_144734/p2p_invalid_messages_0
2019-01-31T13:47:35.309000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/.../bitcoin/test/functional/test_framework/test_framework.py", line 173, in main
    self.run_test()
  File "/home/orion/projects/bitcoin/bitcoin/test/functional/p2p_invalid_messages.py", line 45, in run_test
    self.test_magic_bytes()
  File "/home/orion/projects/bitcoin/bitcoin/test/functional/p2p_invalid_messages.py", line 150, in test_magic_bytes
    self.nodes[0].disconnect_p2ps()
  File "/usr/lib/python3.6/contextlib.py", line 88, in __exit__
    next(self.gen)
  File "/.../bitcoin/test/functional/test_framework/test_node.py", line 291, in assert_debug_log
    self._raise_assertion_error('Expected message "{}" does not partially match log:\n\n{}\n\n'.format(expected_msg, print_log))
  File "/.../bitcoin/test/functional/test_framework/test_node.py", line 143, in _raise_assertion_error
    raise AssertionError(self._node_msg(msg))
AssertionError: [node 0] Expected message "PROCESSMESSAGE: INVALID MESSAGESTART ping" does not partially match log:

 - 2019-01-31T13:47:35.257438Z INVALID MESSAGESTART ping
 - 2019-01-31T13:47:35.257494Z disconnecting peer=0
 - 2019-01-31T13:47:35.257613Z Cleared nodestate for peer=0


2019-01-31T13:47:35.363000Z TestFramework (INFO): Stopping nodes
2019-01-31T13:47:35.567000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_₿_🏃_20190131_144734/p2p_invalid_messages_0
2019-01-31T13:47:35.567000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_₿_🏃_20190131_144734/p2p_invalid_messages_0/test_framework.log
2019-01-31T13:47:35.569000Z TestFramework (ERROR): Hint: Call /.../bitcoin/test/functional/combine_logs.py '/tmp/test_runner_₿_🏃_20190131_144734/p2p_invalid_messages_0' to consolidate all logs


stderr:



TEST                    | STATUS    | DURATION

p2p_invalid_messages.py | ✖ Failed  | 1 s

ALL                     | ✖ Failed  | 1 s (accumulated)
Runtime: 1 s

src/net.cpp

maflcko · 2019-01-31T15:42:44Z

Needs rebase

jonasschnelli · 2019-01-31T19:35:00Z

Re-added the invalid header log (slightly changed log message) and fixed the new tests.

maflcko · 2019-01-31T19:42:55Z

src/net.cpp

-    if (hdr.nMessageSize > MAX_SIZE)
+    // reject if message has an invalid header
+    if (!hdr.IsValid(Params().MessageStart())) {
+        LogPrint(BCLog::NET, "INVALID HEADER DETECTED\n");


There is already logging in

LogPrintf("CMessageHeader::IsValid(): (%s, %u bytes) nMessageSize > MAX_SIZE

I have no opinion on where to do it, but it should suffice to log only one line instead of two for a single error.

Without the log entry in L670, we would only log the invalid message size (not invalid magic check and invalid command strings).
We could drop the invalid message size but I think this is one we should keep since it smells after DoS/dishonest peers.

After this PR, you will get two log entries in case of an message size larger then MAX_SIZE.

LogPrintf("CMessageHeader::IsValid(): (%s, %u bytes) nMessageSize > MAX_SIZE\n", GetCommand(), nMessageSize);

LogPrint(BCLog::NET, "INVALID HEADER DETECTED\n");

Which I think is acceptable.

Of course we could remove the first entry about the message size.

DrahtBot · 2019-03-11T18:59:50Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: test: replace inv type magic numbers by constants #18764 (refactor: test: replace inv type magic numbers by constants by theStack)
p2p: Unify Send and Receive protocol versions #17785 (p2p: Unify Send and Receive protocol versions by hebasto)

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.

jonasschnelli · 2019-04-24T12:44:22Z

Rebased.

practicalswift · 2019-04-24T13:43:39Z

Concept ACK

dongcarl · 2020-01-22T19:47:33Z

Rebased this PR for you: master...dongcarl:2020-01-netmsg_1-rebased

# Conflicts: # src/net_processing.cpp

jonasschnelli · 2020-05-07T07:35:26Z

Rebased (used @dongcarl's rebase).

jonatack

Almost-ACK 2fccdaf code review, tests green, but like with the CI, the process_messages fuzzer crashes immediately.

fuzzer crash details

$ src/test/fuzz/process_messages

INFO: Seed: 3691449907
INFO: Loaded 1 modules   (492032 inline 8-bit counters): 492032 [0x56069c1c61e8, 0x56069c23e3e8), 
INFO: Loaded 1 PC tables (492032 PCs): 492032 [0x56069c23e3e8,0x56069c9c03e8), 
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: A corpus is not provided, starting from an empty corpus
#2	INITED cov: 5995 ft: 5996 corp: 1/1b exec/s: 0 rss: 180Mb
	NEW_FUNC[0/1]: 0x560699684c00 in std::_Rb_tree_iterator<std::pair<long const, (anonymous namespace)::CNodeState> >::operator--() /usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/stl_tree.h:301
#4	NEW    cov: 6006 ft: 8664 corp: 2/2b exec/s: 0 rss: 185Mb L: 1/1 MS: 1 ChangeBit-
#5	NEW    cov: 6017 ft: 13028 corp: 3/4098b exec/s: 0 rss: 186Mb L: 4096/4096 MS: 1 CrossOver-
#6	NEW    cov: 6017 ft: 13130 corp: 4/4106b exec/s: 0 rss: 187Mb L: 8/4096 MS: 1 CMP- DE: "cmpctbl"-
	NEW_FUNC[0/125]: 0x56069947f910 in FuzzedDataProvider::ConsumeBytesAsString[abi:cxx11](unsigned long) /home/jon/projects/bitcoin/bitcoin/src/./test/fuzz/FuzzedDataProvider.h:63
	NEW_FUNC[1/125]: 0x56069947ffd0 in ConsumeRandomLengthByteVector(FuzzedDataProvider&, unsigned long) /home/jon/projects/bitcoin/bitcoin/src/./test/fuzz/util.h:28
#12	NEW    cov: 7458 ft: 15384 corp: 5/4189b exec/s: 0 rss: 191Mb L: 83/4096 MS: 1 InsertRepeatedBytes-
#13	REDUCE cov: 7458 ft: 15384 corp: 5/2206b exec/s: 0 rss: 192Mb L: 2113/2113 MS: 1 EraseBytes-
#19	NEW    cov: 7459 ft: 15385 corp: 6/2289b exec/s: 0 rss: 195Mb L: 83/2113 MS: 1 ChangeBinInt-
#27	NEW    cov: 7459 ft: 15409 corp: 7/2461b exec/s: 0 rss: 195Mb L: 172/2113 MS: 3 ChangeBit-ShuffleBytes-InsertRepeatedBytes-
#28	NEW    cov: 7459 ft: 15416 corp: 8/2478b exec/s: 0 rss: 197Mb L: 17/2113 MS: 1 CMP- DE: "getblockt"-
#31	NEW    cov: 7459 ft: 15422 corp: 9/2490b exec/s: 0 rss: 202Mb L: 12/2113 MS: 3 PersAutoDict-ChangeBinInt-CopyPart- DE: "cmpctbl"-
#42	NEW    cov: 7459 ft: 15450 corp: 10/2582b exec/s: 0 rss: 203Mb L: 92/2113 MS: 1 CMP- DE: "getblocks"-
process_messages: test/util/net.cpp:12: void ConnmanTestMsg::NodeReceiveMsgBytes(CNode &, const char *, unsigned int, bool &) const: Assertion `node.ReceiveMsgBytes(pch, nBytes, complete)' failed.
==49545== ERROR: libFuzzer: deadly signal
    #0 0x560699453cc7 in __sanitizer_print_stack_trace /tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_stack.cc:38:3
    #1 0x560699391b46 in fuzzer::Fuzzer::CrashCallback() /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:233:5
    #2 0x560699391b0f in fuzzer::Fuzzer::StaticCrashSignalCallback() /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:206:6
    #3 0x7f45a3cc972f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1272f)
    #4 0x7f45a35267ba in gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x377ba)
    #5 0x7f45a3511534 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x22534)
    #6 0x7f45a351140e in __tls_get_addr (/lib/x86_64-linux-gnu/libc.so.6+0x2240e)
    #7 0x7f45a351f101 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x30101)
    #8 0x56069a017513 in ConnmanTestMsg::NodeReceiveMsgBytes(CNode&, char const*, unsigned int, bool&) const /home/jon/projects/bitcoin/bitcoin/src/test/util/net.cpp:12:5
    #9 0x56069a0179b7 in ConnmanTestMsg::ReceiveMsgFrom(CNode&, CSerializedNetMsg&) const /home/jon/projects/bitcoin/bitcoin/src/test/util/net.cpp:36:5
    #10 0x56069947cb33 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) /home/jon/projects/bitcoin/bitcoin/src/test/fuzz/process_messages.cpp:70:23
    #11 0x56069a6cd4bf in LLVMFuzzerTestOneInput /home/jon/projects/bitcoin/bitcoin/src/test/fuzz/fuzz.cpp:38:5
    #12 0x560699392d9c in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:515:13
    #13 0x5606993925fb in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:440:3
    #14 0x56069939402d in fuzzer::Fuzzer::MutateAndTestOne() /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:648:19
    #15 0x5606993948e5 in fuzzer::Fuzzer::Loop(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:775:5
    #16 0x5606993895f0 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:754:6
    #17 0x5606993ab1f2 in main /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #18 0x7f45a351309a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #19 0x560699382689 in _start (/home/jon/projects/bitcoin/bitcoin/src/test/fuzz/process_messages+0x1d4b689)

NOTE: libFuzzer has rudimentary signal handlers.
      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal
MS: 4 InsertByte-ChangeBinInt-ChangeBit-InsertByte-; base unit: 238a1471e0fe80591ae349e15e2e05d2f27d2752
0x53,0x6d,0x70,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x97,0x51,0x51,0x51,0x51,0x51,0x27,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x4a,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x67,0x65,0x74,0x62,0x6c,0x6f,0x63,0x6b,0x33,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x51,0x63,0x74,0x62,0x6c,0x2,
SmpQQQQQQQQ\x97QQQQQ'QQQQQQQQQQQQQQQQQQQQQQQQQQQJQQQQQQQQQQQQQQQQQQQQQQQQQQgetblock3QQQQQQQQctbl\x02
artifact_prefix='./'; Test unit written to ./crash-1d54d0ac152cf2bedf83aeae267ba747a6faf9e4
Base64: U21wUVFRUVFRUVGXUVFRUVEnUVFRUVFRUVFRUVFRUVFRUVFRUVFRUVFRUVFRSlFRUVFRUVFRUVFRUVFRUVFRUVFRUVFRUVFRZ2V0YmxvY2szUVFRUVFRUVFjdGJsAg==


$ src/test/fuzz/process_messages ../qa-assets/fuzz_seed_corpus/

INFO: Seed: 3660452216
INFO: Loaded 1 modules   (492032 inline 8-bit counters): 492032 [0x559f8ede51e8, 0x559f8ee5d3e8), 
INFO: Loaded 1 PC tables (492032 PCs): 492032 [0x559f8ee5d3e8,0x559f8f5df3e8), 
INFO:    52251 files found in ../qa-assets/fuzz_seed_corpus/
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1048576 bytes
INFO: seed corpus: files: 52251 min: 1b max: 3984182b total: 462001668b rss: 224Mb
#512	pulse  cov: 6015 ft: 13211 corp: 7/9b exec/s: 256 rss: 515Mb
#1024	pulse  cov: 6015 ft: 13362 corp: 9/14b exec/s: 256 rss: 515Mb
#2048	pulse  cov: 6015 ft: 13372 corp: 11/23b exec/s: 227 rss: 515Mb
#4096	pulse  cov: 6015 ft: 13374 corp: 12/29b exec/s: 240 rss: 515Mb
process_messages: test/util/net.cpp:12: void ConnmanTestMsg::NodeReceiveMsgBytes(CNode &, const char *, unsigned int, bool &) const: Assertion `node.ReceiveMsgBytes(pch, nBytes, complete)' failed.
==49620== ERROR: libFuzzer: deadly signal
    #0 0x559f8c072cc7 in __sanitizer_print_stack_trace /tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_stack.cc:38:3
    #1 0x559f8bfb0b46 in fuzzer::Fuzzer::CrashCallback() /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:233:5
    #2 0x559f8bfb0b0f in fuzzer::Fuzzer::StaticCrashSignalCallback() /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:206:6
    #3 0x7fcd3259272f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1272f)
    #4 0x7fcd31def7ba in gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x377ba)
    #5 0x7fcd31dda534 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x22534)
    #6 0x7fcd31dda40e in __tls_get_addr (/lib/x86_64-linux-gnu/libc.so.6+0x2240e)
    #7 0x7fcd31de8101 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x30101)
    #8 0x559f8cc36513 in ConnmanTestMsg::NodeReceiveMsgBytes(CNode&, char const*, unsigned int, bool&) const /home/jon/projects/bitcoin/bitcoin/src/test/util/net.cpp:12:5
    #9 0x559f8cc369b7 in ConnmanTestMsg::ReceiveMsgFrom(CNode&, CSerializedNetMsg&) const /home/jon/projects/bitcoin/bitcoin/src/test/util/net.cpp:36:5
    #10 0x559f8c09bb33 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) /home/jon/projects/bitcoin/bitcoin/src/test/fuzz/process_messages.cpp:70:23
    #11 0x559f8d2ec4bf in LLVMFuzzerTestOneInput /home/jon/projects/bitcoin/bitcoin/src/test/fuzz/fuzz.cpp:38:5
    #12 0x559f8bfb1d9c in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:515:13
    #13 0x559f8bfb15fb in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:440:3
    #14 0x559f8bfb34ee in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:723:7
    #15 0x559f8bfb3765 in fuzzer::Fuzzer::Loop(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:739:3
    #16 0x559f8bfa85f0 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:754:6
    #17 0x559f8bfca1f2 in main /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #18 0x7fcd31ddc09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #19 0x559f8bfa1689 in _start (/home/jon/projects/bitcoin/bitcoin/src/test/fuzz/process_messages+0x1d4b689)

NOTE: libFuzzer has rudimentary signal handlers.
      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x28,0xff,0xff,0xff,0xff,
\xff\xff\xff\xff\xff\xff\xff\xff(\xff\xff\xff\xff
artifact_prefix='./'; Test unit written to ./crash-f85246b48c886455a89bedd4a1a6b76478478e98
Base64: //////////8o/////w==

jonatack · 2020-05-10T10:44:28Z

src/net_processing.cpp

-    // Check header
-    if (!msg.m_valid_header)
-    {
-        LogPrint(BCLog::NET, "PROCESSMESSAGE: ERRORS IN HEADER %s peer=%d\n", SanitizeString(msg.m_command), pfrom->GetId());


Is the msg command and peer id information in the above two NET log error messages useful, worth keeping?

83da576 net: use CMessageHeader::HEADER_SIZE, add missing include (Jon Atack) Pull request description: as suggested 16 months ago by Gleb Naumenko in #15197 (comment). `static constexpr CMessageHeader::HEADER_SIZE` is already used in this file, `src/net.cpp`, in 2 instances. This commit replaces the remaining 2 integer values in the file with it and adds the explicit include header. Co-authored by: Gleb Naumenko <naumenko.gs@gmail.com> ACKs for top commit: naumenkogs: utACK 83da576 practicalswift: ACK 83da576 -- patch looks correct theStack: ACK 83da576 -- verified that its just magic number elimination refactoring and additionally checked that all tests pass 👍 Tree-SHA512: 5b915483bca4ea162c259865a1b615d73b88a1b1db3f82db05f770d10b8a42494d948f5b21badbcce2d9efa5915b8cbb6af83073867c23d2f152c0d35ac37b96

…g include 83da576 net: use CMessageHeader::HEADER_SIZE, add missing include (Jon Atack) Pull request description: as suggested 16 months ago by Gleb Naumenko in bitcoin#15197 (comment). `static constexpr CMessageHeader::HEADER_SIZE` is already used in this file, `src/net.cpp`, in 2 instances. This commit replaces the remaining 2 integer values in the file with it and adds the explicit include header. Co-authored by: Gleb Naumenko <naumenko.gs@gmail.com> ACKs for top commit: naumenkogs: utACK 83da576 practicalswift: ACK 83da576 -- patch looks correct theStack: ACK 83da576 -- verified that its just magic number elimination refactoring and additionally checked that all tests pass 👍 Tree-SHA512: 5b915483bca4ea162c259865a1b615d73b88a1b1db3f82db05f770d10b8a42494d948f5b21badbcce2d9efa5915b8cbb6af83073867c23d2f152c0d35ac37b96

DrahtBot · 2020-05-20T19:33:09Z

🐙 This pull request conflicts with the target branch and needs rebase.

…r, extend logging deb5271 Remove header checks out of net_processing (Troy Giorshev) 52d4ae4 Give V1TransportDeserializer CChainParams& member (Troy Giorshev) 5bceef6 Change CMessageHeader Constructor (Troy Giorshev) 1ca20c1 Add doxygen comment for ReceiveMsgBytes (Troy Giorshev) 890b1d7 Move checksum check from net_processing to net (Troy Giorshev) 2716647 Give V1TransportDeserializer an m_node_id member (Troy Giorshev) Pull request description: Inspired by #15206 and #15197, this PR moves all message header verification from the message processing layer and into the network/transport layer. In the previous PRs there is a change in behavior, where we would disconnect from peers upon a single failed checksum check. In various discussions there was concern over whether this was the right choice, and some expressed a desire to see how this would look if it was made to be a pure refactor. For more context, see https://bitcoincore.reviews/15206.html#l-81. This PR improves the separation between the p2p layers, helping improvements like [BIP324](#18242) and #18989. ACKs for top commit: ryanofsky: Code review ACK deb5271 just rebase due to conflict on adjacent line jnewbery: Code review ACK deb5271. Tree-SHA512: 1a3b7ae883b020cfee1bef968813e04df651ffdad9dd961a826bd80654f2c98676ce7f4721038a1b78d8790e4cebe8060419e3d8affc97ce2b9b4e4b72e6fa9f

jnewbery · 2020-09-29T10:39:54Z

This is obsoleted by #19107

@jonasschnelli - if you think that there's anything in here that isn't addressed by 19107, feel free to reopen or open another PR.

…rk layer, extend logging deb5271 Remove header checks out of net_processing (Troy Giorshev) 52d4ae4 Give V1TransportDeserializer CChainParams& member (Troy Giorshev) 5bceef6 Change CMessageHeader Constructor (Troy Giorshev) 1ca20c1 Add doxygen comment for ReceiveMsgBytes (Troy Giorshev) 890b1d7 Move checksum check from net_processing to net (Troy Giorshev) 2716647 Give V1TransportDeserializer an m_node_id member (Troy Giorshev) Pull request description: Inspired by bitcoin#15206 and bitcoin#15197, this PR moves all message header verification from the message processing layer and into the network/transport layer. In the previous PRs there is a change in behavior, where we would disconnect from peers upon a single failed checksum check. In various discussions there was concern over whether this was the right choice, and some expressed a desire to see how this would look if it was made to be a pure refactor. For more context, see https://bitcoincore.reviews/15206.html#l-81. This PR improves the separation between the p2p layers, helping improvements like [BIP324](bitcoin#18242) and bitcoin#18989. ACKs for top commit: ryanofsky: Code review ACK deb5271 just rebase due to conflict on adjacent line jnewbery: Code review ACK deb5271. Tree-SHA512: 1a3b7ae883b020cfee1bef968813e04df651ffdad9dd961a826bd80654f2c98676ce7f4721038a1b78d8790e4cebe8060419e3d8affc97ce2b9b4e4b72e6fa9f

jonasschnelli added the P2P label Jan 18, 2019

laanwj reviewed Jan 18, 2019

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

laanwj reviewed Jan 18, 2019

View reviewed changes

src/protocol.cpp Outdated Show resolved Hide resolved

jonasschnelli force-pushed the 2019/01/netmsg_1 branch from 99c83df to c749b13 Compare January 18, 2019 07:33

maflcko reviewed Jan 31, 2019

View reviewed changes

src/net.cpp Show resolved Hide resolved

jonasschnelli force-pushed the 2019/01/netmsg_1 branch from c749b13 to 8b961df Compare January 31, 2019 19:34

jonasschnelli force-pushed the 2019/01/netmsg_1 branch from 8b961df to 0350100 Compare January 31, 2019 19:36

maflcko reviewed Jan 31, 2019

View reviewed changes

DrahtBot added the Needs rebase label Apr 23, 2019

jonasschnelli force-pushed the 2019/01/netmsg_1 branch from 0350100 to 865e760 Compare April 24, 2019 11:41

DrahtBot removed the Needs rebase label Apr 24, 2019

jonasschnelli mentioned this pull request Jul 24, 2019

p2p: Refactor network message deserialization #16202

Merged

DrahtBot added the Needs rebase label Oct 28, 2019

jonasschnelli mentioned this pull request May 6, 2020

Add BIP324 encrypted p2p transport de-/serializer (only used in tests) #18242

Closed

jonasschnelli and others added 3 commits May 7, 2020 09:21

Remove redundant message network magic check

183c54f

Reject invalid headers (disconnect) during deserialization

3a6579b

# Conflicts: # src/net_processing.cpp

net: Move MAX_PROTOCOL_MESSAGE_LENGTH to protocol.h

2fccdaf

jonasschnelli force-pushed the 2019/01/netmsg_1 branch from 865e760 to 2fccdaf Compare May 7, 2020 07:35

DrahtBot removed the Needs rebase label May 7, 2020

This was referenced May 7, 2020

p2p: Unify Send and Receive protocol versions #17785

Merged

refactor: test: replace inv type magic numbers by constants #18764

Merged

jonatack reviewed May 10, 2020

View reviewed changes

jonatack mentioned this pull request May 10, 2020

net: use CMessageHeader::HEADER_SIZE, add missing include #18931

Merged

maflcko mentioned this pull request May 18, 2020

tests: Add fuzzing harnesses for functions in script/ #18994

Merged

troygiorshev mentioned this pull request May 20, 2020

Immediately disconnect on invalid net message checksum #15206

Closed

DrahtBot added the Needs rebase label May 20, 2020

This was referenced May 26, 2020

p2p: Refactor, move all header verification into net, without changing behavior troygiorshev/bitcoin#3

Closed

p2p: Move all header verification into the network layer, extend logging #19107

Merged

jnewbery closed this Sep 29, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Refactor and slightly stricter p2p message processing #15197

Refactor and slightly stricter p2p message processing #15197

Uh oh!

Conversation

jonasschnelli commented Jan 18, 2019

Uh oh!

Uh oh!

Uh oh!

laanwj commented Jan 18, 2019

Uh oh!

laanwj commented Jan 21, 2019

Uh oh!

naumenkogs commented Jan 21, 2019

Uh oh!

jonasschnelli commented Jan 24, 2019

Uh oh!

laanwj commented Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jan 31, 2019

Uh oh!

jonasschnelli commented Jan 31, 2019

Uh oh!

maflcko Jan 31, 2019

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Jan 31, 2019

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonasschnelli commented Apr 24, 2019

Uh oh!

practicalswift commented Apr 24, 2019

Uh oh!

dongcarl commented Jan 22, 2020

Uh oh!

jonasschnelli commented May 7, 2020

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack May 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented May 20, 2020

Uh oh!

jnewbery commented Sep 29, 2020

Uh oh!

Uh oh!

laanwj commented Jan 31, 2019 •

edited

Loading

DrahtBot commented Mar 11, 2019 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack May 10, 2020 •

edited

Loading