Skip to content

fix(pex): seed doesn't respond to pex requests#574

Merged
lklimek merged 12 commits intov0.10-devfrom
fix-seed-pex
Feb 10, 2023
Merged

fix(pex): seed doesn't respond to pex requests#574
lklimek merged 12 commits intov0.10-devfrom
fix-seed-pex

Conversation

@lklimek
Copy link
Collaborator

@lklimek lklimek commented Feb 7, 2023

Issue being fixed or feature implemented

On a devnet with > 10 masternodes, seed fails to advertise addresses in response to PexRequest.

This is caused by a combination of several factors:

  1. When there is only 1 address in addressbook, seed limits number of returned addresses to 1. This causes PexResponse to contain only 1 address, belonging to the seed.
  2. Timer which determines when the seed sends PexRequest is reset after processing each request. Multiple concurrent messages received on the Pex channel reset this timer before it is fired.

What was done?

  1. Adjusted advertising logic to include self address when determining number of addresses to return.
  2. Timer in pex channel processing is reset only after sending PexRequest
  3. Refactored code to never access peerManage.store directly
  4. Improved logging and error handling
  5. Improved tests of peer manager advertising
  6. Extraced some code from sendRequestForPeers to make critical section shorter
  7. Refactored node/seed.go and node/node.go to decrease code redundancy.

How Has This Been Tested?

Tested on devnet, by providing a Docker image and ensuring the issue cannot be reproduced anymore.

Breaking Changes

None

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@lklimek lklimek force-pushed the fix-seed-pex branch 2 times, most recently from 44d9bc5 to ead9533 Compare February 10, 2023 08:34
@lklimek lklimek marked this pull request as ready for review February 10, 2023 08:38
shotonoff
shotonoff previously approved these changes Feb 10, 2023
Copy link
Collaborator

@shotonoff shotonoff left a comment

Choose a reason for hiding this comment

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

LGTM
left one question

Copy link
Collaborator

@shotonoff shotonoff left a comment

Choose a reason for hiding this comment

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

approve

@lklimek lklimek merged commit a7a885c into v0.10-dev Feb 10, 2023
@lklimek lklimek deleted the fix-seed-pex branch February 10, 2023 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants