addrman: reset I2P ports in all "new" buckets#22471
addrman: reset I2P ports in all "new" buckets#22471vasild wants to merge 1 commit intobitcoin:masterfrom
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
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
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.
| // 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.
(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. |
5 participants
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
CAddrInfoobject isshared between all occurrences of an address in all "new" buckets. So
subsequent encounters of that address will see the
CAddrInfoalreadywith 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