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(p2p): adaptive chain exchange timeout #4470

Merged
merged 9 commits into from
Jul 18, 2024

Conversation

hanabi1224
Copy link
Contributor

@hanabi1224 hanabi1224 commented Jul 2, 2024

Summary of changes

This PR tries to address the below comment on CHAIN_EXCHANGE_TIMEOUT

/// Timeout for response from an RPC request
// This value could be tweaked, this is just set pretty low to avoid peers
// timing out requests from slowing the node down. If increase, should create a
// countermeasure for this.

Changes introduced in this pull request:

  • introduce AdaptiveValueProvider to automatically adjust CHAIN_EXCHANGE_TIMEOUT on successes and failures

https://github.com/ChainSafe/forest/actions/runs/9754275564/job/26921065275?pr=4470#step:8:445

2024-07-02T03:11:08.618161Z  INFO forest_filecoin::chain_sync::network_context: Decreased chain exchange timeout to 2500ms. Current average: 96ms
2024-07-02T03:11:08.659245Z  INFO forest_filecoin::chain_sync::network_context: Decreased chain exchange timeout to 2000ms. Current average: 39ms

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

@hanabi1224 hanabi1224 changed the title fix: adaptive chain exchange timeout fix(p2p): adaptive chain exchange timeout Jul 2, 2024
@hanabi1224 hanabi1224 marked this pull request as ready for review July 2, 2024 06:25
@hanabi1224 hanabi1224 requested a review from a team as a code owner July 2, 2024 06:25
@hanabi1224 hanabi1224 requested review from LesnyRumcajs and sudo-shashank and removed request for a team July 2, 2024 06:25
Comment on lines 297 to 298
let success_time_cost_millis_total = Arc::new(AtomicU64::new(0));
let success_count = Arc::new(AtomicU64::new(0));
Copy link
Member

Choose a reason for hiding this comment

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

Could we abstract it to a higher level class to avoid having this lower-level logic in such places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this crate look like a good fit? https://crates.io/crates/incr_stats

Copy link
Member

Choose a reason for hiding this comment

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

I'd avoid relying on niche crates, since it should be simple to implement a basic abstraction ourselves. Niche crates bloat dependency tree and are more susceptible to malicious code (since there's hardly anyone reviewing the changes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@hanabi1224 hanabi1224 added this pull request to the merge queue Jul 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Jul 11, 2024
@hanabi1224
Copy link
Contributor Author

@lemmih @LesnyRumcajs can you please re-approve? Thanks!

@hanabi1224 hanabi1224 added this pull request to the merge queue Jul 18, 2024
Merged via the queue into main with commit 2cf2a25 Jul 18, 2024
28 checks passed
@hanabi1224 hanabi1224 deleted the hm/adaptive-chain-exchange-timeout branch July 18, 2024 08:35
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.

None yet

3 participants