test, refactor: Magic bytes array followup #28857

pull TheCharlatan wants to merge 2 commits into bitcoin:master from TheCharlatan:netSerializationFollowup changing 4 files +15 −6

TheCharlatan commented at 10:07 AM on November 13, 2023: contributor
This is a followup-PR for #28423
- Initialize magic bytes in constructor
- Add a small unit test for serializing arrays.
DrahtBot commented at 10:07 AM on November 13, 2023: contributor


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 sipa, maflcko

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:
- #28686 (refactor: Split per-peer parts of net module into new node/connection module by ajtowns)
- #28451 (refactor: Remove unused SER_DISK, SER_NETWORK, SER_GETHASH by maflcko)
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.

Type	Reviewers
ACK	sipa, maflcko

in src/protocol.cpp:94 in 151ad2911d outdated

  89 | @@ -90,10 +90,9 @@ const static std::vector<std::string> g_all_net_message_types{
  90 |      NetMsgType::SENDTXRCNCL,
  91 |  };
  92 |  
  93 | -CMessageHeader::CMessageHeader(const MessageStartChars& pchMessageStartIn, const char* pszCommand, unsigned int nMessageSizeIn)
  94 | +CMessageHeader::CMessageHeader(const MessageStartChars& pchMessageStartIn, const char* pszCommand, unsigned int nMessageSizeIn) :
  95 | +    pchMessageStart{pchMessageStartIn}

maflcko commented at 10:16 AM on November 13, 2023:

CMessageHeader::CMessageHeader(const MessageStartChars& pchMessageStartIn, const char* pszCommand, unsigned int nMessageSizeIn) 
    : pchMessageStart{pchMessageStartIn}

nit: on the next line, according to clang-format?

TheCharlatan commented at 1:22 PM on November 13, 2023:

Thanks, pushed.

refactor: Initialize magic bytes in constructor initializer

Also remove an assert that is already enforced by the compiler checking
that the length of the std::array matches.

d49d198840

test: Add test for array serialization 1e5b86171e
TheCharlatan force-pushed on Nov 13, 2023
TheCharlatan marked this as ready for review on Nov 13, 2023
sipa commented at 2:59 PM on November 13, 2023: member

utACK 1e5b86171e81ab4b022b9746bb06e1968ecf4086

in src/test/serialize_tests.cpp:89 in 1e5b86171e

  84 | @@ -85,6 +85,8 @@ BOOST_AUTO_TEST_CASE(sizes)
  85 |      BOOST_CHECK_EQUAL(GetSerializeSize(int64_t(0), 0), 8U);
  86 |      BOOST_CHECK_EQUAL(GetSerializeSize(uint64_t(0), 0), 8U);
  87 |      BOOST_CHECK_EQUAL(GetSerializeSize(bool(0), 0), 1U);
  88 | +    BOOST_CHECK_EQUAL(GetSerializeSize(std::array<uint8_t, 1>{0}, 0), 1U);
  89 | +    BOOST_CHECK_EQUAL(GetSerializeSize(std::array<uint8_t, 2>{0, 0}, 0), 2U);

maflcko commented at 3:31 PM on November 13, 2023:

    BOOST_CHECK_EQUAL(GetSerializeSize(std::array<uint8_t, 2>{}, 0), 2U);

nit: I think array will fill itself with zeros if they are omitted

maflcko approved
maflcko commented at 3:32 PM on November 13, 2023: member

lgtm ACK 1e5b86171e81ab4b022b9746bb06e1968ecf4086
fanquake merged this on Nov 14, 2023
fanquake closed this on Nov 14, 2023
bitcoin locked this on Nov 13, 2024

Contributors

Linked (view graph)

#28423 kernel: Remove protocol.h/netaddress.h/compat.h from kernel headers

test, refactor: Magic bytes array followup #28857

Code Coverage

Reviews

Conflicts