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. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Since |
0xB10C
force-pushed
the
2024-03-29007-follow-ups
branch
from
2951239 to
777fb3d
Compare
src/addrdb.cpp
Outdated
There was a problem hiding this comment.
nit (feel free to ignore): If you want, you can also replace LogPrintf with LogInfo for clarity, while touching this file.
There was a problem hiding this comment.
I'm not doing this for now as I think this is slightly out-of-scope of this PR even if I touch the file.
The addpeeraddress calls can fail due to collisions. As we are using a deteministic addrman, they won't fail with the current bucket/position calculation. However, if the calculation is changed, they might collide and fail silently causing tests using `seed_addrman()` to fail. Assert that the addpeeraddress calls are successful.
Just having deterministic is enough. See bitcoin#29007 (comment)
0xB10C
force-pushed
the
2024-03-29007-follow-ups
branch
from
777fb3d to
9a44a20
Compare
|
With #28998 merged, this PR is now ready-for-review. I've also rebased it. |
stratospher
left a comment
stratospher
left a comment
There was a problem hiding this comment.
tested ACK 9a44a20.
you can also include the check in test/functional/feature_asmap.py since addpeeraddress + deterministic addrman is present there too.
|
lgtm ACK 9a44a20 |
7 participants
A few, small follow-ups to #29007. See commit messages for details.