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

Fix: routing table size use MaxPeers config #200

Merged
merged 2 commits into from
Nov 23, 2022

Conversation

0xcb9ff9
Copy link

@0xcb9ff9 0xcb9ff9 commented Sep 28, 2022

Description

Error logs:

dogechain.network.discovery: Failed to add new peer to routing
table: peer=XXXXXXXXXXXXXXXXXXXXXXXXXXXXXX err="peer rejected; insufficient capacity"

network/server_discovery.go#L199 use fixed value

network/server_discovery.go#L211 add peer to dial queue from callback

In extreme cases, each peer will allocate a bucket, and cannot actively initiate a connection to a new peer when the capacity is insufficient

Changes include

  • Bugfix (non-breaking change that solves an issue)

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels

Testing

  • I have tested this code with the official test suite

@0xcb9ff9 0xcb9ff9 added the bug fix Functionality that fixes a bug label Sep 28, 2022
@0xcb9ff9 0xcb9ff9 self-assigned this Sep 28, 2022
@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2022

Codecov Report

Merging #200 (df3b9d4) into dev (a840ee8) will decrease coverage by 0.93%.
The diff coverage is 38.94%.

@@            Coverage Diff             @@
##              dev     #200      +/-   ##
==========================================
- Coverage   51.21%   50.28%   -0.94%     
==========================================
  Files         117      117              
  Lines       17525    18341     +816     
==========================================
+ Hits         8975     9222     +247     
- Misses       7789     8321     +532     
- Partials      761      798      +37     
Impacted Files Coverage Δ
blockchain/storage/kvstorage/kvstorage.go 54.16% <ø> (+2.46%) ⬆️
consensus/ibft/hooks.go 55.26% <ø> (ø)
consensus/ibft/pos.go 0.00% <0.00%> (ø)
helper/keccak/keccak.go 0.00% <ø> (ø)
helper/keccak/pool.go 0.00% <ø> (ø)
jsonrpc/debug_endpoint.go 36.84% <0.00%> (ø)
network/bootnodes.go 100.00% <ø> (ø)
network/connections.go 98.07% <ø> (ø)
secrets/secrets.go 100.00% <ø> (ø)
state/executor.go 3.07% <0.00%> (-0.12%) ⬇️
... and 58 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@DarianShawn DarianShawn left a comment

Choose a reason for hiding this comment

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

Good job, man.
Some comments need to be resolved.

Copy link
Collaborator

@DarianShawn DarianShawn left a comment

Choose a reason for hiding this comment

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

LGTM

@DarianShawn DarianShawn self-requested a review September 29, 2022 06:52
Copy link
Collaborator

@DarianShawn DarianShawn left a comment

Choose a reason for hiding this comment

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

We need to figure out more information of p2p params.

@0xcb9ff9 0xcb9ff9 force-pushed the fix/route_table_size branch from a24523f to df3b9d4 Compare November 21, 2022 03:31
Copy link
Collaborator

@DarianShawn DarianShawn left a comment

Choose a reason for hiding this comment

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

Good job.

Well, it seems like I made some mistakes with the bucket size concept.
The TryAddPeer would fail when bucket full.
It is horrible when it was connected by overwhelming peers, and node didn't even notice it.

@abrahamcruise321 Some more suggestions?

@YuXiaoCoder
Copy link

My node is currently out of sync. When is the new version expected to be released?

Copy link
Collaborator

@abrahamcruise321 abrahamcruise321 left a comment

Choose a reason for hiding this comment

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

lgtm.

@abrahamcruise321 abrahamcruise321 merged commit 82f943b into dogechain-lab:dev Nov 23, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Nov 23, 2022
@0xcb9ff9 0xcb9ff9 deleted the fix/route_table_size branch November 23, 2022 08:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug fix Functionality that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants