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

Add the ability to listen for discovered chains #808

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

masih
Copy link
Member

@masih masih commented Dec 20, 2024

Expand chain exchange to accept a listener which is notified whenever a new chain is discovered. This mechanism is intended to be integrated into F3 host pubsub, whereupon receiving a partial message the host looks up its chain. When known, the chain is returned immediately. Otherwise, the host would buffer the partial message and await notification of its discovering from chain exchange.

Part of #792

Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 10 lines in your changes missing coverage. Please review.

Project coverage is 68.54%. Comparing base (135bfe6) to head (7c4840e).

Files with missing lines Patch % Lines
chainexchange/pubsub.go 75.00% 7 Missing ⚠️
chainexchange/options.go 57.14% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #808      +/-   ##
==========================================
- Coverage   68.97%   68.54%   -0.44%     
==========================================
  Files          81       81              
  Lines        8255     8281      +26     
==========================================
- Hits         5694     5676      -18     
- Misses       2079     2116      +37     
- Partials      482      489       +7     
Files with missing lines Coverage Δ
chainexchange/chainexchange.go 100.00% <ø> (ø)
chainexchange/options.go 35.95% <57.14%> (+1.80%) ⬆️
chainexchange/pubsub.go 55.90% <75.00%> (-0.31%) ⬇️

... and 5 files with indirect coverage changes

chainexchange/pubsub.go Outdated Show resolved Hide resolved
Base automatically changed from masih/pubsub-chainexchange to main December 20, 2024 15:41
@masih masih force-pushed the masih/pubsub-chainexchange-listener branch 2 times, most recently from 0bca51c to b200822 Compare December 20, 2024 16:29
@masih masih marked this pull request as ready for review December 20, 2024 16:29
Expand chain exchange to accept a listener which is notified whenever a
new chain is discovered. This mechanism is intended to be integrated
into F3 host pubsub, whereupon receiving a partial message the host
looks up its chain. When known, the chain is returned immediately.
Otherwise, the host would buffer the partial message and await
notification of its discovering from chain exchange.

Part of #792
@masih masih force-pushed the masih/pubsub-chainexchange-listener branch from b200822 to 7c4840e Compare December 20, 2024 16:43
@masih masih requested a review from Kubuxu December 20, 2024 16:49
@Kubuxu
Copy link
Contributor

Kubuxu commented Dec 20, 2024

If you are saying we are fine with doing it under lock, we should keep it under lock. Manual interrupted locking like this is a foot gun.

@masih
Copy link
Member Author

masih commented Dec 20, 2024

Manual interrupted locking like this is a foot gun.

I take this as a blocker? It's either this or notify listener under lock. I have no preference.

Taking a closer look at the lru library we are using, I am sort of underwhelmed that it doesn't expose atomic combined removal operations. if we had those, we could remove the need for lock in most places. I also understand why they haven't , i.e. the auto eviction of lru used for caching.

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