doc, test: test and explain service flag handling #29213
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
Concept ACK |
mzumsande
force-pushed
the
202401_addrman_serviceflags
branch
from
0687436
to
d536e5a
Compare
|
Concept ACK |
|
utACK d536e5a |
| @@ -111,6 +111,9 @@ class AddrMan | |||
|
|
|||
| /** | |||
| * Attempt to add one or more addresses to addrman's new table. | |||
| * If an address already exists in addrman, the existing entry may be updated | |||
| * (e.g. adding additional service flags). If the existing entry is in the new table, | |||
| * it may be added to more buckets, improving the probability of selection. | |||
There was a problem hiding this comment.
Perhaps add a "see AddSingle()" mention somewhere in here, as the logic referred to here is in that method called from this one (Add/Add_).
There was a problem hiding this comment.
I think it's better if the header doesn't refer to implementation details. If I remember correctly, this logic used to be in Add(), then Add_(), now AddSingle(), and I don't think it's nice if we have to update addrman.h for refactors like that.
| BOOST_CHECK_EQUAL(vAddr5.size(), 1U); | ||
| BOOST_CHECK_EQUAL(vAddr5.at(0).nServices, NODE_NETWORK); | ||
|
|
||
| // Adding service flags even works when the addr is in Tried |
There was a problem hiding this comment.
| // Adding service flags even works when the addr is in Tried | |
| // Adding service flags even works when the addr is in Tried; see `AddSingle()` |
There was a problem hiding this comment.
similar to above, I don't like referring to implementation details in comments unless really necessary.
| * Attempt to add one or more addresses to addrman's new table. | ||
| * If an address already exists in addrman, the existing entry may be updated | ||
| * (e.g. adding additional service flags). If the existing entry is in the new table, |
There was a problem hiding this comment.
It is not immediately clear that additional services flags may be added, but the return value could still be false. And the same seems to happen for the AddrInfo timestamp. It can also be updated while the return value remains false.
There was a problem hiding this comment.
I think that followed from the @return doc, but it wasn't so obvious that adding an existing addr to another new bucket also affects the return value; Changed the @return description to true if at least one address is successfully added, or added to an additional bucket. Unaffected by updates.
Service flags are handled differently, depending on whether validated (if received from the peer) or unvalidated (received via gossip relay).
mzumsande
force-pushed
the
202401_addrman_serviceflags
branch
from
d536e5a
to
74ebd4d
Compare
|
ACK 74ebd4d |
|
Post-merge ACK 74ebd4d |
Service flags received from the peer-to-peer network are handled differently, depending on how we receive them.
If received directly from an outbound peer the flags belong to, they replace existing flags.
If received via gossip relay (so that anyone could send them), new flags are added, but existing ones but cannot be overwritten.
Document that and add test coverage for it.