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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See 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
Concept ACK
utACK d536e5a6325d1885224f36debdcc5245b94efe8a
110 | @@ -111,6 +111,9 @@ class AddrMan 111 | 112 | /** 113 | * Attempt to add one or more addresses to addrman's new table. 114 | + * If an address already exists in addrman, the existing entry may be updated 115 | + * (e.g. adding additional service flags). If the existing entry is in the new table, 116 | + * it may be added to more buckets, improving the probability of selection.
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_).
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.
3375 | @@ -3376,6 +3376,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, 3376 | vRecv >> CNetAddr::V1(addrMe); 3377 | if (!pfrom.IsInboundConn()) 3378 | { 3379 | + // Overwrites potentially existing services. In contrast to this, 3380 | + // unvalidated services received via gossip relay are only 3381 | + // ever added but cannot replace existing ones.
It seems to me this might be helpful and the message names are unlikely to change.
// Overwrites potentially existing services. In contrast to this,
- // unvalidated services received via gossip relay are only
- // ever added but cannot replace existing ones.
+ // unvalidated services received via gossip relay in ADDR/ADDRV2
+ // messages are only ever added but do not replace existing ones.
done
1114 | + BOOST_CHECK(addrman->Good(addr)); // addr has NODE_NONE 1115 | + std::vector<CAddress> vAddr5{addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt)}; 1116 | + BOOST_CHECK_EQUAL(vAddr5.size(), 1U); 1117 | + BOOST_CHECK_EQUAL(vAddr5.at(0).nServices, NODE_NETWORK); 1118 | + 1119 | + // Adding service flags even works when the addr is in Tried
// Adding service flags even works when the addr is in Tried; see `AddSingle()`
similar to above, I don't like referring to implementation details in comments unless really necessary.
ACK d536e5a6325d1885224f36debdcc5245b94efe8a
110 | @@ -111,6 +111,9 @@ class AddrMan 111 | 112 | /** 113 | * Attempt to add one or more addresses to addrman's new table. 114 | + * If an address already exists in addrman, the existing entry may be updated 115 | + * (e.g. adding additional service flags). If the existing entry is in the new table,
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.
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).
ACK 74ebd4d1359edce82a134dfcd3da9840f8d206e2
utACK 74ebd4d1359edce82a134dfcd3da9840f8d206e2
ACK 74ebd4d1359edce82a134dfcd3da9840f8d206e2
Post-merge ACK 74ebd4d1359edce82a134dfcd3da9840f8d206e2