BIP155: clarify variable integer format and change time to fixed 32 bit #967

pull vasild wants to merge 2 commits into bitcoin:master from vasild:bip155_time_and_varint changing 1 files +6 −8

vasild commented at 9:49 AM on August 13, 2020: contributor
- Clarify that by VARINT (variable integer) we mean CompactSize, not VARINT (as in the source code) (huh!?). Relevant IRC discussion: http://www.erisian.com.au/bitcoin-core-dev/log-2020-07-27.html#l-94
- Change time to fixed 32 bits as that will work until year 2106, not just 2038 and takes 1 byte less than a variable integer format.

BIP155: change "time" to fixed 32 bit unsigned

32 bit unsigned can represent time up to year 2106
(32 bit signed is limited to just 2038).

So, we don't need to have "time" encoded as variable integer which would
take 5 bytes instead of 4.

2c7630ec61

vasild force-pushed on Aug 13, 2020
vasild commented at 9:54 AM on August 13, 2020: contributor

cc @jonasschnelli @luke-jr - this touches your suggestions.
vasild commented at 9:55 AM on August 13, 2020: contributor

Render preview
vasild cross-referenced this on Aug 13, 2020 from issue BIP155: include changes from followup discussions by vasild
vasild commented at 10:00 AM on August 13, 2020: contributor

cc also @laanwj as the author of BIP155 and @sipa who was involved in the VARINT vs CompactSize discussion.
luke-jr added the label Proposed BIP modification on Aug 20, 2020
vasild cross-referenced this on Aug 25, 2020 from issue Tor v3 support by vasild
sipa commented at 7:37 PM on September 16, 2020: member

LGTM

in bip-0155.mediawiki:64 in 44ff4bfe03 outdated

  63 |  |-
  64 | -| <code>VARINT</code> (unsigned)
  65 | +| <code>CompactSize</code>
  66 |  | <code>services</code>
  67 | -| Service bits. A 64-wide bit field.
  68 | +| Service bits. A bit field that is 64-bit wide, encoded in [https://en.bitcoin.it/wiki/Protocol_documentation#Variable_length_integer CompactSize].

jonatack commented at 6:19 PM on September 17, 2020:

nit, s/64-bit wide/64 bits wide/

vasild commented at 9:13 AM on September 29, 2020:

Done.

jonatack commented at 6:20 PM on September 17, 2020: contributor

Seems good
laanwj commented at 8:04 PM on September 17, 2020: member

ACK but agree with @jonatack nit
troygiorshev commented at 2:32 AM on September 23, 2020: none

ACK
vasild cross-referenced this on Sep 29, 2020 from issue Complete the BIP155 implementation and upgrade to TORv3 by vasild

BIP155: clarify that "services" uses CompactSize

The Bitcoin Core source code has `VARINT` type which is different than
the "variable integer" format used all over the P2P protocol and also
for the "services" field in this BIP. The latter is called `CompactSize`
in some BIPs and also in the Bitcoin Core source code, thus use the word
`CompactSize` to refer to it and link to its documentation.

87ef5aaa7c

vasild force-pushed on Sep 29, 2020
vasild commented at 9:14 AM on September 29, 2020: contributor

Addressed nit: s/64-bit wide/64 bits wide/
laanwj commented at 2:59 PM on September 29, 2020: member

re-ACK
troygiorshev commented at 12:45 PM on September 30, 2020: none

reACK
jonatack commented at 1:42 PM on October 1, 2020: contributor

ACK
luke-jr merged this on Oct 1, 2020
luke-jr closed this on Oct 1, 2020
vasild deleted the branch on Oct 1, 2020
MarcoFalke cross-referenced this on Jun 1, 2021 from issue p2p: fix ubsan addrman errors, make nTime truncation conversion explicit by jonatack

Contributors

Labels

Proposed BIP modification

Linked (view graph)

#907 BIP155: include changes from followup discussions