Improves block commit error typing (WIP) #5682
Conversation
There was a problem hiding this comment.
This looks like a good refactor. Just wondering if it is needed for the getblocktemplate work?
We are currently in an audit freeze for all other Rust changes, so we can make any audit changes quickly and easily.
We could add the branch to the ticket, and open a PR when we are out of the freeze?
| @@ -271,10 +272,10 @@ where | |||
| match state_service | |||
| .ready() | |||
| .await | |||
| .map_err(VerifyBlockError::Commit)? | |||
| .expect("poll_ready method of state service and Ready::<StateService>::poll will never return Err") | |||
There was a problem hiding this comment.
For your information - no changes needed:
This change locks us into always having the state service ready.
That might be ok, because our design requires the service is either ready: Poll::Ready(Ok(_)), or permanently failed: Poll::Ready(Err(_)). The state service can never be temporarily paused: Poll::Pending.
The state needs to commit blocks in order. So we want to pause higher blocks, but still let lower blocks commit. But Tower services can only pause all requests, so we can't use poll_ready() to do that.
So as long as we're sure we'll never error here, even on shutdown, it would be ok to do this.
There was a problem hiding this comment.
Got it! I was very uncertain about this part, that's good to know.
There was a problem hiding this comment.
I am also unsure about this!
It's not needed (unless we find out #5487 is required), I was just curious. Thank you for taking a look.
This sounds good :) |
Motivation
This is a rough draft and there are a lot of fixes to be made if it's to be pursued.
I had started on this when there wasn't much else in need of urgent attention.
Briefly opening this up for comments on whether this issue is something that could be addressed in the near-term and if this PR is in the right direction.
This is likely very low priority (though I'd be happy to be convinced otherwise) and unlikely to be merged.
Related to #2908.
Review
This PR is a rough draft.
Reviewer Checklist