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 |
0687436 to
d536e5a
Compare
|
Concept ACK |
|
utACK d536e5a6325d1885224f36debdcc5245b94efe8a |
jonatack
left a comment
There was a problem hiding this comment.
ACK d536e5a6325d1885224f36debdcc5245b94efe8a
src/addrman.h
Outdated
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.
src/test/addrman_tests.cpp
Outdated
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.
src/addrman.h
Outdated
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).
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.