refactor, test: refactor addrman_tried_collisions test to directly check for collisions #23713

1. 
   josibake commented at 4:53 PM on December 8, 2021: member

   Previously, the addrman_tried_collisions test behaved in the following way:

   1. add an address to addrman
   2. attempt to move the new address to the tried table (using AddrMan.Good())
   3. verify that num_addrs matched size() to check for collisions in the new table

   AddrMan.size(), however, returns the number of unique address in addrman, regardless of whether they are in new or tried. This means the test would still pass for addresses where a collision did occur in the tried table. After 3 collisions in the tried table, there would eventually be a collision in the new table when trying to add a new address, which was then detected by checking num_addrs - collisions == size().

   While the collision in the new table was caused by a collision in the tried table, the test is misleading as it's not directly testing for collisions in the tried table and misses 3 collisions before identifying a collision in the new table.

   solution

   To more directly test the tried table, I refactored AddrMan::Good() to return a boolean after successfully adding an address to the tried table. This makes the test much cleaner by first adding an address to new, calling Good to move it to the tried table, and checking if it was successful or not. It is worth noting there are other reasons, aside from collisions, which will cause Good to return false. That being said, this is an improvement over the previous testing methodology.

   Additionally, having Good() return a boolean is useful outside of testing as it allows the caller to handle the case where Good is unable to move the entry to the tried table (e.g https://github.com/bitcoin/bitcoin/blob/a06364741358feae04813050e4225eb43fc386e3/src/rpc/net.cpp#L945).

   followup

   As a follow up to this PR, I plan to look at the following places Good() is called and see if it makes sense to handle the case where it is unable to add an entry to tried:

   • https://github.com/bitcoin/bitcoin/blob/a06364741358feae04813050e4225eb43fc386e3/src/rpc/net.cpp#L945
   • https://github.com/bitcoin/bitcoin/blob/a06364741358feae04813050e4225eb43fc386e3/src/net.cpp#L2067
   • https://github.com/bitcoin/bitcoin/blob/a06364741358feae04813050e4225eb43fc386e3/src/net_processing.cpp#L2708
2. josibake force-pushed on Dec 8, 2021
3. laanwj added the label Tests on Dec 8, 2021
4. laanwj added the label Refactoring on Dec 8, 2021
5. fanquake requested review from jnewbery on Dec 9, 2021
6. fanquake requested review from mzumsande on Dec 9, 2021
7. 
   DrahtBot commented at 3:50 AM on December 9, 2021: contributor

   <!--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:

   • #23545 (scripted-diff: Use clang-tidy syntax for C++ named arguments by MarcoFalke)
   • #23373 (Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change by vasild)

   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.

8. josibake force-pushed on Dec 10, 2021
9. josibake force-pushed on Dec 10, 2021
10. josibake marked this as ready for review on Dec 10, 2021
11. 
    jonatack commented at 12:25 PM on December 11, 2021: contributor

    Interesting. Will have a look soon.

12. 
    mzumsande commented at 12:40 AM on December 13, 2021: contributor

    Concept ACK I think it makes sense to have Good() return a bool indicating whether it moved an address to new, even if the main call site in net_processing doesn't currently have a need for it.

    There are also several other unit tests where making use the return value of Good() would makes sense, like addrman_selecttriedcollision, addrman_noevict, addrman_evictionworks, plus the addpeeraddress RPC.

13. 
    josibake commented at 1:09 PM on December 14, 2021: member

    There are also several other unit tests where making use the return value of Good() would makes sense

    I've refactored the remaining addrman tests here: https://github.com/josibake/bitcoin/tree/josibake-addrman-test-refactors , I'll open as a PR once/if this one merges.

14. in src/addrman.cpp:629 in 52dd3df9e6 outdated

     625 | @@ -626,7 +626,7 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT
     626 |  
     627 |      // if not found, bail out
     628 |      if (!pinfo)
     629 | -        return;
     630 | +        return false;
    

    ---

    ---

    

    jnewbery commented at 1:28 PM on December 14, 2021:

    Feel free to update to current project code style while touching this line:

        if (!pinfo) return false;
    

    or

        if (!pinfo) {
            return false;
        }
    

15. in src/addrman.cpp:642 in 52dd3df9e6 outdated

     638 | @@ -639,11 +639,11 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT
     639 |  
     640 |      // if it is already in the tried set, don't do anything else
     641 |      if (info.fInTried)
     642 | -        return;
     643 | +        return false;
    

    ---

    ---

    

    jnewbery commented at 1:28 PM on December 14, 2021:

    as above

16. in src/test/addrman_tests.cpp:271 in 52dd3df9e6 outdated

     267 | @@ -268,32 +268,32 @@ BOOST_AUTO_TEST_CASE(addrman_new_collisions)
     268 |  
     269 |  BOOST_AUTO_TEST_CASE(addrman_tried_collisions)
     270 |  {
     271 | -    AddrManTest addrman;
     272 | +    auto addrman = std::make_unique<AddrMan>(std::vector<bool>(), /* makeDeterministic */ true, /* consistency_check_ratio */ 100);
    

    ---

    ---

    

    jnewbery commented at 1:43 PM on December 14, 2021:

    clang-tidy expects the comments to be formatted as:

        auto addrman = std::make_unique<AddrMan>(std::vector<bool>(), /*deterministic=*/true, /*consistency_check_ratio=*/100);
    

17. 
    jnewbery commented at 1:44 PM on December 14, 2021: contributor

    Concept ACK. I've left a few minor comments inline.

    There's also a small typo in the commit log for commit 74e89661b9f9172f1b1b6f12afe7692c9b2db9d9 "refactor: check Good() in tried_collisions test":

    - check whether or not moving to the tried table was successul by
    + check whether or not moving to the tried table was successful by
    

18. refactor: make AddrMan::Good return bool

    If AddrMan::Good is unable to add an entry
    to tried (for a number of reasons), return false.
    
    This makes it much easier and cleaner to directly
    test for tried collisions. It also allows anyone
    calling Good() to handle the case where adding an
    address to tried is unsuccessful.
    
    Update docs to doxygen style.

    207f1c825c
19. refactor: check Good() in tried_collisions test

    Rather than try to infer a collision by checking `AddrMan::size`,
    check whether or not moving to the tried table was successful by
    checking the output from `AddrMan::Good`

    f961c477b5
20. refactor: remove dependence on AddrManTest caac999ff0
21. josibake force-pushed on Dec 14, 2021
22. 
    josibake commented at 5:00 PM on December 14, 2021: member

    force pushed to take the style and commit message fixes from @jnewbery (thanks for catching those!)

    changes can be verified with git range-diff 52dd3df...caac999

23. 
    jnewbery commented at 5:04 PM on December 14, 2021: contributor

    utACK caac999ff0

24. fanquake requested review from vasild on Dec 14, 2021
25. 
    mzumsande commented at 12:34 AM on December 15, 2021: contributor

    Code review ACK caac999ff0fc5c98fa438b7e96fe1232f6573fd5

26. maflcko merged this on Dec 15, 2021
27. maflcko closed this on Dec 15, 2021

    ---

28. in src/addrman.h:89 in caac999ff0

      86 | +    /**
      87 | +     * Mark an address record as accessible and attempt to move it to addrman's tried table.
      88 | +     *
      89 | +     * @param[in] addr            Address record to attempt to move to tried table.
      90 | +     * @param[in] nTime           The time that we were last connected to this peer.
      91 | +     * @return    true if the address is successfully moved from the new table to the tried table.
    

    ---

    ---

    

    vasild commented at 9:47 AM on December 15, 2021:

    nit: maybe add something like "if the same address is already in the tried table, then false is returned" to avoid wrong assumptions that if false is returned then the address is not in the tried table after the call - i.e. "it was not moved to tried, so it is not in tried after the call" (wrong).

    Or return true in that case? :eyes:

    ---

    

    josibake commented at 11:51 AM on December 15, 2021:

    thanks for the review! good point about the docs, I've made a note for myself to update them in a future PR.

    Regarding returning true for duplicates, I'm not sure about that. My gut feeling is that I would want to handle attempting to add duplicates to tried differently than adding new entries, but I'm not comfortable enough with AddrMan yet to clearly articulate why I think this :sweat_smile:

29. 
    vasild commented at 9:49 AM on December 15, 2021: contributor

    ACK caac999ff0fc5c98fa438b7e96fe1232f6573fd5

30. sidhujag referenced this in commit 6c62f0d091 on Dec 15, 2021
31. maflcko referenced this in commit d1dc6b895f on Dec 20, 2021
32. bitcoin locked this on Dec 15, 2022
33. josibake deleted the branch on Jan 26, 2024

Contributors

josibakeDrahtBotjonatackmzumsandejnewberyvasild

Review Requested
mzumsande