Skip to content

Conversation

@pinheadmz
Copy link
Contributor

The current addrman patch results in an addrman that buckets ipv4 address by 32 bits instead of 16. The issue is that in NetGroupManager::GetGroup() we are dealing with IPv4 mapped to IPv6 addresses and so the 32 bits the function actually processes are the null prefix in the embedding scheme. This means all ipv4 addresses in warnet always end up in the same bucket :-P

The solution in this PR ensures that we process the last 32 bits of the embedded ipv6 structure which is where the actual ipv4 address really lives.

The issue is illustrated really well in the debugger while stopped in GetGroup():

(lldb) print address
(const CNetAddr &) 0x0000000170028e50: {
  m_addr = {
    _union = {
      direct = "d\U00000014\U0000001e("
      indirect_contents = (indirect = "", capacity = 0)
    }
    _size = 4
  }
  m_net = NET_IPV4
  m_scope_id = 0
}

(lldb) print address.GetLinkedIPv4()
(uint32_t) 1679040040

(lldb) print address.GetAddrBytes()
(std::vector<unsigned char>) size=16 {
  [0] = '\0'
  [1] = '\0'
  [2] = '\0'
  [3] = '\0'
  [4] = '\0'
  [5] = '\0'
  [6] = '\0'
  [7] = '\0'
  [8] = '\0'
  [9] = '\0'
  [10] = '\xff'
  [11] = '\xff'
  [12] = 'd'
  [13] = '\x14'
  [14] = '\x1e'
  [15] = '('
}

@pinheadmz
Copy link
Contributor Author

h/t to @0xB10C and #338 which brought this issue to light!

@willcl-ark
Copy link
Contributor

Dang, that's a nice bugfix fren!

Patch LGTM.

We'll want to rebuild the images after this one. I'll kick them all off in a bit on my machine, and then merge if they all complete?

@pinheadmz
Copy link
Contributor Author

Just realized I made a mistake as well will update....

@pinheadmz
Copy link
Contributor Author

Fixed my mistake and added an addrman unit test for reviewers

@pinheadmz
Copy link
Contributor Author

added a test that pulls the new v27.0 image and checks addrman buckets. also had to bump lnd version: https://x.com/roasbeef/status/1781086945986404559

Copy link
Contributor

@willcl-ark willcl-ark left a comment

Choose a reason for hiding this comment

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

ACK fa0e383

tested locally, and it works nicely

@willcl-ark willcl-ark merged commit 36638b1 into bitcoin-dev-project:main Jun 6, 2024
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.

2 participants