addrman: reset I2P ports in all "new" buckets #22471

1. 
   vasild commented at 4:29 PM on July 16, 2021: member

   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 #22470

   <!-- *** Please remove the following help text before submitting: *** Pull requests without a rationale and clear improvement may be closed immediately. GUI-related pull requests should be opened against https://github.com/bitcoin-core/gui first. See CONTRIBUTING.md -->

   <!-- Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly: * Any test improvements or new tests that improve coverage are always welcome. * All other changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). Contributors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change. * Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed. * Features are welcome, but might be rejected due to design or scope issues. If a feature is based on a lot of dependencies, contributors should first consider building the system outside of Bitcoin Core, if possible. * Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. -->

   <!-- Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time. -->

2. addrman: reset I2P ports in all "new" buckets

   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 https://github.com/bitcoin/bitcoin/issues/22470

   24cad63539
3. DrahtBot added the label Tests on Jul 16, 2021
4. MarcoFalke added this to the milestone 22.0 on Jul 17, 2021
5. MarcoFalke removed the label Tests on Jul 17, 2021
6. MarcoFalke added the label P2P on Jul 17, 2021
7. in src/test/addrman_tests.cpp:1098 in 24cad63539

   1094 | +    const CAddress i2p_new5_changed_port{
   1095 | +        ResolveService("zsxwyo6qcn3chqzwxnseusqgsnuw3maqnztkiypyfxtya4snkoka.b32.i2p",
   1096 | +                       I2P_SAM31_PORT),
   1097 | +        NODE_NONE,
   1098 | +        good_time};
   1099 | +    // Crashes due to https://github.com/bitcoin/bitcoin/issues/22470
   

   ---

   ---

   

   jonatack commented at 9:02 AM on July 17, 2021:

       // Crashes without the change in this commit due to [#22470](/bitcoin-bitcoin/22470/)
   

   ---

   

   jonatack commented at 9:06 AM on July 17, 2021:

   (and maybe add the explanation in the PR description here)

8. 
   jonatack commented at 9:05 AM on July 17, 2021: member

   Very helpful test!

   ACK 24cad6353921e81f3701b527745e46b80d441e8a 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.

9. 
   DrahtBot commented at 7:25 PM on July 19, 2021: member

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #22497 by vasild
   • #22496 by jnewbery
   • #20233 by jnewbery

   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.

10. 
    fanquake commented at 1:03 AM on July 20, 2021: member

    Closing now that #22497 has been merged.

11. fanquake closed this on Jul 20, 2021

    ---

12. vasild deleted the branch on Jul 20, 2021
13. DrahtBot locked this on Aug 16, 2022

Contributors

vasildjonatackDrahtBotfanquake

Milestone
22.0