Skip to content
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

test: fix intermittent failure in p2p_getaddr_caching.py #28144

Merged
merged 2 commits into from Aug 1, 2023

Conversation

mzumsande
Copy link
Contributor

@mzumsande mzumsande commented Jul 25, 2023

Fixes #28133

In the consistency check, it's not enough to check that our address/port is unique, only the combination of source and target must be unique. Otherwise, the OS may reuse ports for connections to different -addrbind, which was happening in the failed runs.

While at it, the second commit cleans up duplicate getaddr messages in p2p_getaddr_caching.py that do nothing but generate Ignoring repeated "getaddr" log messages (and cleans up some whitespace the python linter complains about).

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 25, 2023

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 vasild
Concept ACK jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Tests label Jul 25, 2023
@mzumsande mzumsande changed the title test: fix intermittent timeout in p2p_getaddr_caching.py test: fix intermittent failure in p2p_getaddr_caching.py Jul 25, 2023
@jonatack
Copy link
Contributor

Concept ACK, thank you for looking into it.

@maflcko maflcko requested a review from vasild July 25, 2023 05:48
@maflcko maflcko added this to the 26.0 milestone Jul 25, 2023
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e4d3787

Today I learned something new: it is possible to have duplicate (srcaddr, srcport) for TCP connections if the destination is different. Thank you!

test/functional/p2p_getaddr_caching.py Show resolved Hide resolved
test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
Only the combined addr:port of source and destination
must be unique. If the destination is different, the same addr:port
for the source may be used by the OS.
python p2p instances will automatically send a getaddr msg after
connecting, the explicit message was a duplicate that was being ignored.
@mzumsande
Copy link
Contributor Author

e4d3787 to 8a20f76: Addressed feedback, thanks!

Today I learned something new: it is possible to have duplicate (srcaddr, srcport) for TCP connections if the destination is different. Thank you!

I was also surprised by this!

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 8a20f76

@fanquake fanquake merged commit 1b5cbf7 into bitcoin:master Aug 1, 2023
15 checks passed
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 9, 2023
…ching.py

8a20f76 test: drop duplicate getaddrs from p2p_getaddr_caching (Martin Zumsande)
feb0096 test: fix intermittent failure in p2p_getaddr_caching (Martin Zumsande)

Pull request description:

  Fixes bitcoin#28133

  In the consistency check, it's not enough to check that our address/port is unique, only the combination of source and target must be unique. Otherwise, the OS may reuse ports for connections to different `-addrbind`, which was happening in the failed runs.

  While at it, the second commit cleans up duplicate `getaddr` messages in `p2p_getaddr_caching.py` that do nothing but generate `Ignoring repeated "getaddr"` log messages (and cleans up some whitespace the python linter complains about).

ACKs for top commit:
  vasild:
    ACK 8a20f76

Tree-SHA512: eabe4727d7887f729074076f6333a918bba8cb34b8e3baaa83f167b441b0daa24f7c4824abcf03a9538a2ef14b2d826ff19aeffcb93a6c20735253a9678aac9c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

p2p_getaddr_caching.py failure in TSan CI
6 participants