protocol.h: Move MESSAGE_START_SIZE into CMessageHeader #8880

laanwj · 2016-10-04T11:17:50Z

This moves all the header sizes together. Also move the enum to the top, and remove a deceptive TODO comment (this is just a dumb data structure describing the protocol and there's nothing to be gained from encapsulation at that level).

Other commit fixes a silly typo in version.h

Also move the enum to the top, and remove a deceptive TODO comment.

dcousens

utACK

sipa · 2016-10-04T12:33:56Z

utACK

paveljanik · 2016-10-04T13:23:04Z

src/protocol.h

@@ -48,17 +56,6 @@ class CMessageHeader
        READWRITE(FLATDATA(pchChecksum));
    }

-    // TODO: make private (improves encapsulation)


Comment removed. Is it OK?

See OP

and remove a deceptive TODO comment (this is just a dumb data structure describing the protocol and there's nothing to be gained from encapsulation at that level).

paveljanik · 2016-10-04T13:23:18Z

utACK

btcdrak · 2016-10-04T14:29:04Z

utACK

theuni · 2016-10-04T15:22:32Z

src/protocol.h

@@ -29,6 +27,16 @@
 class CMessageHeader
 {
 public:
+    enum {
+        MESSAGE_START_SIZE = 4,


nit: could make these constexprs instead while we're at it. No real reason other than cosmetic, though.

theuni · 2016-10-04T15:24:43Z

utACK 2c09a52

laanwj · 2016-10-04T15:47:40Z

src/protocol.h

@@ -29,6 +27,16 @@
 class CMessageHeader
 {
 public:
+    enum {
+        MESSAGE_START_SIZE = 4,


Huh yes now I look at it again using an enum here is a somewhat of a strange use of the C++ language.

Though it looks like we use this kind of constant declaration in many places, and so does leveldb:

arith_uint256.h: enum { WIDTH=BITS/32 }; chain.h: enum { nMedianTimeSpan=11 }; leveldb/db/skiplist.h: enum { kMaxHeight = 12 }; leveldb/helpers/memenv/memenv.cc: enum { kBlockSize = 8 * 1024 }; ...

So I suppose it's somewhat acceptable in some cases?

@laanwj Looks like several of those are hold-overs from c++03. In the case where there's no actual grouping and we're just using them as constants, I think constexpr's would be fine, but the enum is ofc acceptable as well.

Nevermind for here, we can discuss in a separate style guide PR.

laanwj · 2016-10-04T16:05:57Z

Added another commit:

protocol.h: Make enums in GetDataMsg concrete values: This concretizes the numbers and adds a comment to make it clear that these numbers are fixed by the protocol, and may avoid people forgetting to claim numbers in the future (e.g. issue MSG_CMPCT_BLOCK enum value is already in use #8500).

paveljanik · 2016-10-05T10:45:42Z

src/protocol.h

-    MSG_TX,
-    MSG_BLOCK,
+    MSG_TX = 1,
+    MSG_BLOCK = 2,
    MSG_TYPE_MAX = MSG_BLOCK,


MSG_TYPE_MAX is not used at all in the sources and looks strange in the middle...

HUH. That's a good catch. How could I have missed that one.

@paveljanik

This concretizes the numbers and adds a comment to make it clear that these numbers are fixed by the protocol, and may avoid people forgetting to claim numbers in the future (e.g. issue bitcoin#8500). Also gets rid of a weird unused `MSG_TYPE_MAX` in the middle of the enumeration (thanks @paveljanik for noticing).

laanwj · 2016-10-05T11:33:59Z

Got rid of the awkward MSG_TYPE_MAX leftover in between the enum values (and squashed). Thanks @paveljanik for noticing.

sipa · 2016-10-05T11:38:13Z

The MSG_TYPE_MAX is a leftover from earlier code that got refactored in the segwit PR. It delimited the maximum msg type that could be seen in an inv (as opposed to the next ones, which can only be seen in getdata).

laanwj · 2016-10-05T12:07:37Z

The MSG_TYPE_MAX is a leftover from earlier code that got refactored in thesegwit PR.

OK, in historical context it makes sense, it just looks weird now.

jonasschnelli

utACK 1df3111

paveljanik

ACK 1df3111

1df3111 protocol.h: Make enums in GetDataMsg concrete values (Wladimir J. van der Laan) 2c09a52 protocol.h: Move MESSAGE_START_SIZE into CMessageHeader (Wladimir J. van der Laan) f9bd92d version.h: s/shord/short/ in comment (Wladimir J. van der Laan)

…Header 1df3111 protocol.h: Make enums in GetDataMsg concrete values (Wladimir J. van der Laan) 2c09a52 protocol.h: Move MESSAGE_START_SIZE into CMessageHeader (Wladimir J. van der Laan) f9bd92d version.h: s/shord/short/ in comment (Wladimir J. van der Laan)

…TNESS_BLOCK Found these missing while working on bitcoin/bitcoin#8880 .

…Header 1df3111 protocol.h: Make enums in GetDataMsg concrete values (Wladimir J. van der Laan) 2c09a52 protocol.h: Move MESSAGE_START_SIZE into CMessageHeader (Wladimir J. van der Laan) f9bd92d version.h: s/shord/short/ in comment (Wladimir J. van der Laan)

ZIP 239 preparations 3 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#8080 - bitcoin/bitcoin#8082 - bitcoin/bitcoin#8126 - bitcoin/bitcoin#7910 - This is the unsquashed version of bitcoin/bitcoin#8149 - We take three cleanup commits to the protocol / `CInv` code. - bitcoin/bitcoin#8822 - bitcoin/bitcoin#8880 - Excluding the first commit (we don't have the comment it fixes yet). - bitcoin/bitcoin#19322

laanwj added 2 commits October 4, 2016 11:11

version.h: s/shord/short/ in comment

f9bd92d

protocol.h: Move MESSAGE_START_SIZE into CMessageHeader

2c09a52

Also move the enum to the top, and remove a deceptive TODO comment.

laanwj added the Refactoring label Oct 4, 2016

dcousens approved these changes Oct 4, 2016

View reviewed changes

paveljanik reviewed Oct 4, 2016

View reviewed changes

theuni reviewed Oct 4, 2016

View reviewed changes

laanwj commented Oct 4, 2016

View reviewed changes

laanwj force-pushed the 2016_10_cosmetic branch from 463786a to f5d3f0f Compare October 5, 2016 02:09

paveljanik reviewed Oct 5, 2016

View reviewed changes

laanwj force-pushed the 2016_10_cosmetic branch from f5d3f0f to 1df3111 Compare October 5, 2016 11:33

laanwj mentioned this pull request Oct 5, 2016

bip-0144: Add enum values for MSG_WITNESS_{BLOCK,TX}, MSG_FILTERED_WITNESS_BLOCK bitcoin/bips#457

Merged

jonasschnelli approved these changes Oct 8, 2016

View reviewed changes

paveljanik approved these changes Oct 8, 2016

View reviewed changes

laanwj merged commit 1df3111 into bitcoin:master Oct 15, 2016

hypo-test pushed a commit to hypo-test/BitcoinBips-713993353 that referenced this pull request Nov 25, 2018

bip-0144: Add enum values for MSG_WITNESS_{BLOCK,TX}, MSG_FILTERED_WI…

eccda40

…TNESS_BLOCK Found these missing while working on bitcoin/bitcoin#8880 .

str4d mentioned this pull request Aug 13, 2021

ZIP 239 preparations 3 zcash/zcash#5274

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

protocol.h: Move MESSAGE_START_SIZE into CMessageHeader #8880

protocol.h: Move MESSAGE_START_SIZE into CMessageHeader #8880

laanwj commented Oct 4, 2016 •

edited

Loading

dcousens left a comment

sipa commented Oct 4, 2016

paveljanik Oct 4, 2016

laanwj Oct 5, 2016

paveljanik commented Oct 4, 2016

btcdrak commented Oct 4, 2016

theuni Oct 4, 2016

theuni commented Oct 4, 2016

laanwj Oct 4, 2016

laanwj Oct 4, 2016

theuni Oct 4, 2016

laanwj commented Oct 4, 2016

paveljanik Oct 5, 2016

laanwj Oct 5, 2016

laanwj commented Oct 5, 2016 •

edited

Loading

sipa commented Oct 5, 2016 via email

laanwj commented Oct 5, 2016

jonasschnelli left a comment

paveljanik left a comment

protocol.h: Move MESSAGE_START_SIZE into CMessageHeader #8880

protocol.h: Move MESSAGE_START_SIZE into CMessageHeader #8880

Conversation

laanwj commented Oct 4, 2016 • edited Loading

dcousens left a comment

Choose a reason for hiding this comment

sipa commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paveljanik commented Oct 4, 2016

btcdrak commented Oct 4, 2016

Choose a reason for hiding this comment

theuni commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj commented Oct 5, 2016 • edited Loading

sipa commented Oct 5, 2016 via email

laanwj commented Oct 5, 2016

jonasschnelli left a comment

Choose a reason for hiding this comment

paveljanik left a comment

Choose a reason for hiding this comment

laanwj commented Oct 4, 2016 •

edited

Loading

laanwj commented Oct 5, 2016 •

edited

Loading