Skip to content

Conversation

@tomt1664
Copy link
Member

@tomt1664 tomt1664 commented May 1, 2025

…int64_t

51f7668 addrman: change nid_type from int to int64_t (Martin Zumsande)
051ba32 addrman, refactor: introduce user-defined type for internal nId (Martin Zumsande)

Pull request description:

  With `nIdCount` being incremented for each addr received, an attacker could cause an overflow in the past, see https://bitcoincore.org/en/2024/07/31/disclose-addrman-int-overflow/
  Even though that attack was made infeasible indirectly by addr rate-limiting (PR #22387), to be on the safe side and prevent any regressions change the `nId`s used internally to `int64_t`.
  This is being done by first introducing a user-defined type for `nId`s in the first commit, and then updating it to `int64_t` (thanks sipa for help with this!).

  Note that `nId` is only used internally, it is not part of the serialization, so `peers.dat` should not be affected by this.

  I assume that the only reason this was not done in the past is to not draw attention to this previously undisclosed issue.

ACKs for top commit:
  naumenkogs:
    ACK 51f7668
  stratospher:
    ACK 51f7668. I think it's a good change to make the nId space large(64 bits) so that the nId values are distinct.
  achow101:
    ACK 51f7668

Tree-SHA512: 68d4b8b0269a01a9544bedfa7c1348ffde00a288537e4c8bf2b88372ac7d96c4566a44dd6b06285f2fcf31b4f9336761e3bca7253fbc20db5e0d04e887156224
Copy link
Member

@delta1 delta1 left a comment

Choose a reason for hiding this comment

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

utACK 285f81b

@delta1 delta1 merged commit 7ed597f into ElementsProject:master May 1, 2025
13 checks passed
delta1 added a commit to delta1/elements that referenced this pull request Jul 9, 2025
delta1 added a commit to delta1/elements that referenced this pull request Jul 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants