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
addrman: reset I2P ports in all "new" buckets #22471
Conversation
In `CAddrMan::ResetI2PPorts()`, if an I2P address is found in two or more "new" buckets, our first encounter of it would change the port to 0 and re-position it within that "new" bucket. The `CAddrInfo` object is shared between all occurrences of an address in all "new" buckets. So subsequent encounters of that address will see the `CAddrInfo` already with port 0 and will skip re-positioning. To fix that, check and re-position if necessary even for I2P entries with port 0. Fixes bitcoin#22470
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very helpful test!
ACK 24cad63 code review, verified the new test fails without the change
One tradeoff with this change is that it potentially increases the action surface of ResetI2PPorts()
. This function was expected to be temporary; it may be worth looking at if it can be removed before v22.0 rather than later.
I2P_SAM31_PORT), | ||
NODE_NONE, | ||
good_time}; | ||
// Crashes due to https://github.com/bitcoin/bitcoin/issues/22470 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Crashes due to https://github.com/bitcoin/bitcoin/issues/22470 | |
// Crashes without the change in this commit due to https://github.com/bitcoin/bitcoin/issues/22470 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and maybe add the explanation in the PR description here)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Closing now that #22497 has been merged. |
In
CAddrMan::ResetI2PPorts()
, if an I2P address is found in two ormore "new" buckets, our first encounter of it would change the port to
0 and re-position it within that "new" bucket. The
CAddrInfo
object isshared between all occurrences of an address in all "new" buckets. So
subsequent encounters of that address will see the
CAddrInfo
alreadywith port 0 and will skip re-positioning.
To fix that, check and re-position if necessary even for I2P entries
with port 0.
Fixes #22470