Skip to content

Issue #131 - Node Sync Optimization#149

Merged
rem1niscence merged 3 commits into
mainfrom
issue-#131
Apr 30, 2025
Merged

Issue #131 - Node Sync Optimization#149
rem1niscence merged 3 commits into
mainfrom
issue-#131

Conversation

@ezeike
Copy link
Copy Markdown
Contributor

@ezeike ezeike commented Apr 17, 2025

Description

This PR updates the node syncing functionality in controller/consensus.go to optimize synchronization and handle unexpected blocks.

Changes Made

  • Implements a queue to track both block requests sent and block request responses awaiting processing by the FSM.
  • A new type blockSyncRequest was created to track these requests in the queue.
  • Block requests are distributed randomly to all known peers in the PeerSet.
  • Uses the SimpleLimiter to limit requests per node to avoid rate limiting and reputation slashing.
  • A rateScaleFactor setting to allow node runners to increase or decrease their block request rates
  • Gracefully handles (ignores and prints warning) when the latest gossiped block is received.

Closes

Issue: #131

@ezeike ezeike marked this pull request as ready for review April 23, 2025 15:49
@andrewnguyen22 andrewnguyen22 added BFT Consensus module Medium Priority Medium priority issue labels Apr 24, 2025
Comment thread controller/consensus.go Outdated
Comment thread controller/consensus.go Outdated
Comment thread controller/consensus.go
Comment thread controller/consensus.go Outdated
Comment thread controller/consensus.go
}

// applyTimeouts removes reuqests from the queue that have timed out
func (c *Controller) applyTimeouts(queue map[uint64]blockSyncRequest) []blockSyncRequest {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this behavior is not strictly related to the controller itself and I see you perform more operations over the queue, why don't convert the queue map into its own type like type queue map[uint64]blockSyncRequest{} and more this method and queuing/dequeueing to methods of that type, that way you can encapsulate all the behavior of this queue there

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could do this and generally I prefer that approach but in this case there's very little to the queue.

One spot to add, two spots to remove, and it's not going to be used from anywhere else.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@andrewnguyen22 thoughts on this? I honestly ultimately don't mind keeping it as-is but might be a little cleaner to have a custom type

@rem1niscence rem1niscence merged commit dec840d into main Apr 30, 2025
2 checks passed
@rem1niscence rem1niscence deleted the issue-#131 branch April 30, 2025 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BFT Consensus module Medium Priority Medium priority issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants