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

josibake · 2021-12-08T16:53:26Z

Previously, the addrman_tried_collisions test behaved in the following way:

add an address to addrman
attempt to move the new address to the tried table (using AddrMan.Good())
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

bitcoin/src/rpc/net.cpp

Line 945 in a063647

node.addrman->Good(address);

).

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:

bitcoin/src/rpc/net.cpp

Line 945 in a063647

node.addrman->Good(address);
bitcoin/src/net.cpp

Line 2067 in a063647

addrman.Good(addr);
bitcoin/src/net_processing.cpp

Line 2708 in a063647

m_addrman.Good(pfrom.addr);

DrahtBot · 2021-12-09T03:50:01Z

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

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.

jonatack · 2021-12-11T12:25:33Z

Interesting. Will have a look soon.

mzumsande · 2021-12-13T00:40:28Z

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.

josibake · 2021-12-14T13:09:15Z

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.

jnewbery

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

src/addrman.cpp

jnewbery · 2021-12-14T13:28:48Z

src/addrman.cpp

src/test/addrman_tests.cpp

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.

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`

josibake · 2021-12-14T17:00:22Z

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

jnewbery · 2021-12-14T17:04:48Z

utACK caac999

mzumsande · 2021-12-15T00:34:17Z

Code review ACK caac999

vasild

ACK caac999

vasild · 2021-12-15T09:47:14Z

src/addrman.h

+     *
+     * @param[in] addr            Address record to attempt to move to tried table.
+     * @param[in] nTime           The time that we were last connected to this peer.
+     * @return    true if the address is successfully moved from the new table to the tried table.


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? 👀

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 😅

…s test to directly check for collisions caac999 refactor: remove dependence on AddrManTest (josibake) f961c47 refactor: check Good() in tried_collisions test (josibake) 207f1c8 refactor: make AddrMan::Good return bool (josibake) Pull request description: 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 ACKs for top commit: jnewbery: utACK caac999 mzumsande: Code review ACK caac999 Tree-SHA512: f328896b1f095e8d2581fcdbddce46fc0491731a0440c6fff01081fa5696cfb896dbbe1d183eda2c100f19aa111e1f8b096ef93582197edc6b791de563a58f99

…e output from `AddrMan::Good()` bf4f817 refactor: addrman_select test (josibake) 5a64dc0 refactor: addrman_evictionworks test (josibake) e281fcc refactor: addrman_noevict test (josibake) 8bdd924 refactor: addrman_selecttriedcollisions test (josibake) Pull request description: As a follow-up to bitcoin#23713 , this PR refactors the remaining tests in `src/tests/addrman_tests.cpp` to use the output from `AddrMan::Good()` where appropriate. ACKs for top commit: naumenkogs: ACK bf4f817 mzumsande: Code Review ACK bf4f817 Tree-SHA512: 93cc127aecff42c1c174daa04911af7e3460a5c40ddf96952fe4a6ab86fa1ff22d66724326abb709008d7f9f79c26c55c6d62753c40059c9ac60f869507ec913

josibake force-pushed the josibake-addrman-collisions-refactor branch from 06fd827 to 7d15135 Compare December 8, 2021 17:12

laanwj added Tests Refactoring labels Dec 8, 2021

fanquake requested review from jnewbery and mzumsande December 9, 2021 02:34

DrahtBot mentioned this pull request Dec 9, 2021

test: Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change #23373

Merged

josibake force-pushed the josibake-addrman-collisions-refactor branch 2 times, most recently from fae9468 to 52dd3df Compare December 10, 2021 19:40

josibake marked this pull request as ready for review December 10, 2021 20:37

DrahtBot mentioned this pull request Dec 11, 2021

scripted-diff: Use clang-tidy syntax for C++ named arguments #23545

Closed

jnewbery reviewed Dec 14, 2021

View reviewed changes

josibake added 3 commits December 14, 2021 17:50

refactor: check Good() in tried_collisions test

f961c47

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`

refactor: remove dependence on AddrManTest

caac999

josibake force-pushed the josibake-addrman-collisions-refactor branch from 52dd3df to caac999 Compare December 14, 2021 16:55

fanquake requested a review from vasild December 14, 2021 23:44

maflcko merged commit 40fdb9e into bitcoin:master Dec 15, 2021

vasild reviewed Dec 15, 2021

View reviewed changes

josibake mentioned this pull request Dec 15, 2021

refactor, test: update addrman_tests.cpp to use output from AddrMan::Good() #23780

Merged

bitcoin locked and limited conversation to collaborators Dec 15, 2022

josibake deleted the josibake-addrman-collisions-refactor branch January 26, 2024 10:51

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

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

Uh oh!

Conversation

josibake commented Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

solution

followup

Uh oh!

DrahtBot commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonatack commented Dec 11, 2021

Uh oh!

mzumsande commented Dec 13, 2021

Uh oh!

josibake commented Dec 14, 2021

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

josibake commented Dec 14, 2021

Uh oh!

jnewbery commented Dec 14, 2021

Uh oh!

mzumsande commented Dec 15, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

vasild Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

josibake Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

josibake commented Dec 8, 2021 •

edited

Loading

DrahtBot commented Dec 9, 2021 •

edited

Loading