addrman: select addresses by network follow-up #27745

pull amitiuttarwar wants to merge 4 commits into bitcoin:master from amitiuttarwar:2023-04-sbn-followups changing 4 files +23 −19

amitiuttarwar commented at 7:22 PM on May 24, 2023: contributor

this PR addresses outstanding review comments from #27214

refactor: de-duplicate lookups

retain the values needed to prevent redundant node lookups

2b6bd12eea

doc: update `Select` function description

Capture potential performance slow down for `Select` by network & clarify
return values.

768770771f

DrahtBot commented at 6:05 AM on May 25, 2023: contributor



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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK mzumsande, brunoerg, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot renamed this:
~~addrman: select addresses by network follow-up~~
addrman: select addresses by network follow-up
on May 25, 2023
DrahtBot added the label P2P on May 25, 2023
in src/addrman.cpp:767 in 7a137fafff outdated
```
 763 | @@ -764,7 +764,7 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::option
 764 |              if (node_id != -1) {
 765 |                  if (network.has_value()) {
 766 |                      const auto it{mapInfo.find(node_id)};
 767 | -                    assert(it != mapInfo.end());
 768 | +                    Assume(it != mapInfo.end());
```
maflcko commented at 6:11 AM on May 25, 2023:

I don't think this is correct. Dereferencing an end-iterator is UB, so there is just no way to safely proceed in production, unless you add an early return (no idea if that is possible or even safe)

amitiuttarwar commented at 8:02 PM on May 26, 2023:

good catch! I intended to bundle this assume with the callers, think I accidentally reverted to an in-between state. will fix

amitiuttarwar commented at 11:15 PM on May 26, 2023:

should be ok now (both here and in GetEntry)
amitiuttarwar force-pushed on May 26, 2023

Type	Reviewers
ACK	mzumsande, brunoerg, achow101

addrman: change asserts to Assumes

`Assume` is safer since the checks are non-fatal- errors in these functions
should provide feedback in debug builds, but do not need to deter further node
operations in production.

b9f1e86f12

test: ensure addrman test is finite

Add a counter to ensure that the error case is bounded rather than leading to a
CI timeout

cd8ef5b3e6

amitiuttarwar force-pushed on May 26, 2023
in src/addrman.cpp:767 in b9f1e86f12 outdated
```
 763 | @@ -764,9 +764,7 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::option
 764 |              if (node_id != -1) {
 765 |                  if (network.has_value()) {
 766 |                      const auto it{mapInfo.find(node_id)};
 767 | -                    assert(it != mapInfo.end());
 768 | -                    const auto info{it->second};
 769 | -                    if (info.GetNetwork() == *network) break;
 770 | +                    if (Assume(it != mapInfo.end()) && it->second.GetNetwork() == *network) break;
```
mzumsande commented at 7:43 PM on May 31, 2023:

I guess if the Assume assumption breaks the worst thing that could happen is that the while loop may run indefinitely - but it might also pick another address if there is one. So this seems better than crashing with an assert.

amitiuttarwar commented at 5:05 PM on June 2, 2023:

yeah, not the best but I think the chances of repeatedly not hitting the Assume condition to be extremely low. alternatively, I could add a counter to prevent that worst case if it seems worth the additional complexity.
mzumsande commented at 7:45 PM on May 31, 2023: contributor

Code Review ACK cd8ef5b3e66b3f766c9c883259b5feb44540d7df
brunoerg approved
brunoerg commented at 7:55 PM on June 22, 2023: contributor

crACK cd8ef5b3e66b3f766c9c883259b5feb44540d7df
achow101 commented at 5:20 PM on June 30, 2023: member

ACK cd8ef5b3e66b3f766c9c883259b5feb44540d7df

Checked that the changes here resolve the remaining review comments on #27214
achow101 merged this on Jun 30, 2023
achow101 closed this on Jun 30, 2023
sidhujag referenced this in commit f70e00d132 on Jul 1, 2023
bitcoin locked this on Jun 29, 2024

Contributors

Labels

Linked (view graph)

#27214 addrman: Enable selecting addresses by network

addrman: select addresses by network follow-up #27745

Reviews