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

feat: use channels for chainsync client requests for results and allo… #639

Merged
merged 1 commit into from
May 28, 2024

Conversation

rakshasa
Copy link
Contributor

…w simultaneous GetCurrentTip requests

@agaffney
Copy link
Contributor

agaffney commented May 17, 2024

@rakshasa if you could provide a high-level description/overview of the changes, that would be helpful in reviewing this

@rakshasa
Copy link
Contributor Author

Issues with the old code:

  • want* variables did not have memory barriers when being accessed from the messageHandler.
  • firstBlockChan did not report errors.
  • GetCurrentTip always resulted in find intersect requests, and was difficult to improve with the old code.
  • Channels make it easier to implement chaining of multiple requests, e.g. for GetAvailableBlockRange.

By using channels here we reduce the number of struct member variables and make it follow golang idioms. While using atomic bool would fix the main issue, it doesn't help future improvements in making the chainsync client asynchronous and able to chain requests.

waitingForCurrentTipChan will process all the requests for the current tip until the channel is empty.

want* only processes one request per message reply received from the server. If the message request fails, it is the responsibility of the caller to clear the channel.

GetCurrentTip splits the types of requests into one (waiting*) that just piggybacks on any current incoming messages, while the other (want*) is a one-to-one request/reply.

@rakshasa rakshasa force-pushed the feat/chainsync-client-wants branch 3 times, most recently from 80b2d59 to 97e0f57 Compare May 22, 2024 15:17
@rakshasa rakshasa force-pushed the feat/chainsync-client-wants branch from 97e0f57 to 8014084 Compare May 22, 2024 15:27
Copy link
Contributor

@agaffney agaffney left a comment

Choose a reason for hiding this comment

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

Sorry that it took so long to review this PR. These changes look great, and we appreciate these contributions!

@agaffney agaffney merged commit cb5ccad into blinklabs-io:main May 28, 2024
8 checks passed
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