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

refactor: shared source inside chunked_by adapters #4232

Merged
merged 2 commits into from
Nov 16, 2023

Conversation

msgmaxim
Copy link
Contributor

Pull Request

Closes: PRO-925

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have updated documentation where appropriate.

Summary

One downside of this approach is that shared is sometimes called more times than strictly necessary (when shared source was used by more than just the chunked_by adapter, e.g. on strictly_monotonic_source in btc witnesser). The alternative I considered is to make chunked_by adapters methods on SharedSource directly, but I think current solution is OK considering that shared adapter seems relatively lightweight, and moving calls to shared cleaned up the high level code a little.

@msgmaxim msgmaxim requested a review from kylezs November 13, 2023 06:25
Copy link

linear bot commented Nov 13, 2023

PRO-925 Use shared adapter inside the chunking adapters

In order to not duplicate the work of the upstream adapters after chunking, the chain source that's passed into the chunking adapter needs to be shared. Rather than have to remember this needs to be done, we should instead do the sharing inside the chunking adapters themselves so it can't be forgotten.

The reason for this is that the chunked adapters create a separate client and stream for each epoch, the shared adapter ensures they are all using the same underlying stream and client.

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (1de7247) 72% compared to head (3fbb185) 71%.
Report is 6 commits behind head on main.

Files Patch % Lines
...ngine/src/witness/common/chain_source/extension.rs 0% 18 Missing ⚠️
engine/src/witness/eth.rs 0% 4 Missing ⚠️
engine/src/witness/btc.rs 0% 3 Missing ⚠️
engine/src/witness/dot.rs 0% 3 Missing ⚠️
engine/src/witness/eth/key_manager.rs 0% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #4232    +/-   ##
======================================
- Coverage     72%     71%    -0%     
======================================
  Files        384     384            
  Lines      63015   63185   +170     
  Branches   63015   63185   +170     
======================================
+ Hits       45074   45123    +49     
- Misses     15609   15728   +119     
- Partials    2332    2334     +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kylezs
Copy link
Contributor

kylezs commented Nov 14, 2023

current solution is OK considering that shared adapter seems relatively lightweight

I agree, think it's fine if it's used somewhere it's not necessary for the benefit of never forgetting to include it (which is a worse consequence)

engine/src/witness/eth.rs Outdated Show resolved Hide resolved
engine/src/witness/eth.rs Outdated Show resolved Hide resolved
@kylezs kylezs merged commit aa50279 into main Nov 16, 2023
@kylezs kylezs deleted the refactor/shared-in-chunked branch November 16, 2023 14:39
dandanlen pushed a commit that referenced this pull request Nov 17, 2023
* refactor: shared source inside chunked_by adapters

* chore: address review
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.

3 participants