New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
addrman: select addresses by network follow-up #27745
Conversation
retain the values needed to prevent redundant node lookups
Capture potential performance slow down for `Select` by network & clarify return values.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
0cd26f2
to
486257b
Compare
`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.
Add a counter to ensure that the error case is bounded rather than leading to a CI timeout
486257b
to
cd8ef5b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review ACK cd8ef5b
assert(it != mapInfo.end()); | ||
const auto info{it->second}; | ||
if (info.GetNetwork() == *network) break; | ||
if (Assume(it != mapInfo.end()) && it->second.GetNetwork() == *network) break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crACK cd8ef5b
cd8ef5b test: ensure addrman test is finite (Amiti Uttarwar) b9f1e86 addrman: change asserts to Assumes (Amiti Uttarwar) 7687707 doc: update `Select` function description (Amiti Uttarwar) 2b6bd12 refactor: de-duplicate lookups (Amiti Uttarwar) Pull request description: this PR addresses outstanding review comments from bitcoin#27214 ACKs for top commit: achow101: ACK cd8ef5b mzumsande: Code Review ACK cd8ef5b brunoerg: crACK cd8ef5b Tree-SHA512: 669f67904263e3f51c39b175eabf5fa1b1e7b6841e889656afec33d0bd93fb446de9403f0a91b186ddeaf29498c8938484a0547b1188256c4e7c90db6f30bb55
this PR addresses outstanding review comments from #27214