Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: raise V1_PREFIX_LEN from 12 to 16 #28577

Merged
merged 1 commit into from Oct 4, 2023

Conversation

sipa
Copy link
Member

@sipa sipa commented Oct 3, 2023

A "version" message in the V1 protocol starts with a fixed 16 bytes:

  • The 4-byte network magic
  • The 12-byte command string: "version" plus 5 0x00 bytes

The current code detects incoming V1 connections by just looking at the first 12 bytes (matching an earlier version of BIP324), but 16 bytes is more precise. This isn't an observable difference right now, as a 12 byte prefix ought to be negligible already, but it may become observable with future extensions to the protocol, so make the code match the specification.

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 3, 2023

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 theStack, mzumsande, achow101
Stale ACK vasild

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the P2P label Oct 3, 2023
@maflcko maflcko added this to the 26.0 milestone Oct 3, 2023
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 78fc36d

src/net.cpp Outdated Show resolved Hide resolved
A "version" message in the V1 protocol starts with a fixed 16 bytes:
* The 4-byte network magic
* The 12-byte zero-padded command "version" plus 5 0x00 bytes

The current code detects incoming V1 connections by just looking at the
first 12 bytes (matching an earlier version of BIP324), but 16 bytes is
more precise. This isn't an observable difference right now, as a 12 byte
prefix ought to be negligible already, but it may become observable with
future extensions to the protocol, so make the code match the
specification.
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 78fc36d

FWIW, I've extended the functional test to add coverage for incoming v1 connection and wrong network magic detections: #28588 (it could be included here, though I think the change in this PR is simple enough to to merge it without the test)

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK ba2e5bf

Copy link
Contributor

@mzumsande mzumsande left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK ba2e5bf

@achow101
Copy link
Member

achow101 commented Oct 4, 2023

ACK ba2e5bf

@achow101 achow101 merged commit 5b44784 into bitcoin:master Oct 4, 2023
15 of 16 checks passed
fanquake added a commit that referenced this pull request Oct 5, 2023
… network magic detection

e130896 test: BIP324: add checks for v1 prefix matching / wrong network magic detection (Sebastian Falbesoner)

Pull request description:

  This PR adds missing test coverage for the detection of incoming v1 connections and wrong network magic on BIP324-enabled (i.e. running with `-v2transport=1`) nodes. Both checks are using prefix sizes of 16 bytes (previously only 12 bytes were used for the v1 prefix matching, which was fixed by PR #28577).

ACKs for top commit:
  Sjors:
    utACK e130896
  MarcoFalke:
    lgtm ACK e130896

Tree-SHA512: d4d1567277297f42c543b9638a6c64d14b17ff0ddbf85a7efff22f45c619478139dbedcb9dc4f449b4814b00856ee43644f15df1aa20c8931d5496a607ca2fd4
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 13, 2023
ba2e5bf net: raise V1_PREFIX_LEN from 12 to 16 (Pieter Wuille)

Pull request description:

  A "version" message in the V1 protocol starts with a fixed 16 bytes:
  * The 4-byte network magic
  * The 12-byte command string: "version" plus 5 0x00 bytes

  The current code detects incoming V1 connections by just looking at the first 12 bytes (matching an [earlier version](bitcoin/bips#1496) of BIP324), but 16 bytes is more precise. This isn't an observable difference right now, as a 12 byte prefix ought to be negligible already, but it may become observable with future extensions to the protocol, so make the code match the specification.

ACKs for top commit:
  achow101:
    ACK ba2e5bf
  theStack:
    re-ACK ba2e5bf
  mzumsande:
    Code review ACK ba2e5bf

Tree-SHA512: 64876b03613bd1c5dda82f4ca1b367014365f9ae4cfa30f45c5758a563c68cbea81a98d02ba616c264674c204517aac8b7de94da10f32e77b56267a43710c651
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants