Skip to content

Conversation

@mzumsande
Copy link
Contributor

Block announcements via headers may have up to MAX_BLOCKS_TO_ANNOUNCE = 8 entries according to the definition of this constant.
However, there are a few spots saying they should have a size less than MAX_BLOCKS_TO_ANNOUNCE. Fix these.
I don't think that this is critical (this only changes behavior when we get a headers announcement with exactly MAX_BLOCKS_TO_ANNOUNCE blocks which we can't connect), but it would be nice to handle this limit consistently.

It is an inclusive upper bound according to its definition.
@fanquake fanquake added the P2P label Jun 17, 2022
@ghost
Copy link

ghost commented Jun 18, 2022

LGTM

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 18, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25454 (p2p, refactor: Avoid multiple getheaders messages in flight to the same peer by sdaftuar)
  • #24571 (p2p: Prevent block index fingerprinting by sending additional getheaders messages by dergoegge)

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.

Copy link
Member

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

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

utACK e357c89 - This PR makes the usage and docs of MAX_BLOCKS_TO_ANNOUNCE consistent with its description.

@sdaftuar
Copy link
Member

utACK - this was an oversight in #8305

@fanquake fanquake merged commit 2fe2702 into bitcoin:master Jun 27, 2022
@mzumsande mzumsande deleted the 202206_max_block_announce branch June 27, 2022 14:23
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 27, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Jun 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants