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: prewitnessing uses unfinalised sc stream #4220

Merged
merged 1 commit into from Nov 8, 2023

Conversation

msgmaxim
Copy link
Contributor

@msgmaxim msgmaxim commented Nov 8, 2023

Pull Request

Closes: PRO-929

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

Pretty straightforward change. Tested on localnet and seems to behave as expected. The only thing that's still unclear to me is whether we also want to specialise is_header_ready somehow so that we don't pessimistically wait for the next block (which could be a long time for BTC). Figured it is worth discussing this before making more changes.

@msgmaxim msgmaxim requested a review from kylezs November 8, 2023 07:28
Copy link

linear bot commented Nov 8, 2023

PRO-929 Use unfinalised stream for prewitnessing

See: #4146

We want to remove the delay that we have as described in PRO-923 by using the unfinalised stream for the pre-witnessers.

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #4220 (33b58e3) into main (22c713b) will decrease coverage by 0%.
The diff coverage is 0%.

@@          Coverage Diff          @@
##            main   #4220   +/-   ##
=====================================
- Coverage     72%     72%   -0%     
=====================================
  Files        383     383           
  Lines      62349   62347    -2     
  Branches   62349   62347    -2     
=====================================
- Hits       44637   44632    -5     
+ Misses     15377   15376    -1     
- Partials    2335    2339    +4     
Files Coverage Δ
engine/src/witness/btc.rs 27% <0%> (-<1%) ⬇️
engine/src/witness/eth.rs 0% <0%> (ø)
engine/src/witness/dot.rs 29% <0%> (+1%) ⬆️
engine/src/witness/start.rs 0% <0%> (ø)

... and 3 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@kylezs
Copy link
Contributor

kylezs commented Nov 8, 2023

The only thing that's still unclear to me is whether we also want to specialise is_header_ready

I think I'd rather leave this for now until/if there's a need for it. This gets us most of the way there, and introducing that might increase the complexity a bit, when we could be deleting pre-witnessing in favour of something more sophisticated for instant-ingress.

@kylezs kylezs enabled auto-merge (squash) November 8, 2023 09:45
@kylezs kylezs merged commit 7594ede into main Nov 8, 2023
41 checks passed
@kylezs kylezs deleted the feat/unfinalised-prewitnessing branch November 8, 2023 09:47
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

2 participants