Turn lite::types::Requester into an async trait
#171
Conversation
Requester not being an async trait forced implementations to block on RPC queries, eg. by spawning a new executor, which is undesirable considering that we eventually want the whole codebase to play nice with async/.await, and not unnecessarily block the enclosing task. Because trait cannot currently define async functions, this commit introduces a dependency on the `async-trait` crate, which turns async fns within traits and their implementations into plain functions returning `Pin<Box<dyn Future<Output = T + ...>>>`. The obvious drawback of this approach is that these async fns now allocate memory on the heap. Hopefully, the language will soon stabilize the feature, which will help us get rid of these undesirable heap allocations. On top of that, the `lite::verifier::verify_bisection_inner` fn had to be manually adapted to return a boxed future because recursive async fn are not currently supported in Rust. An possible alternative would be to rewrite `verify_bisection_inner` to not be recursive anymore, but that is outside of the scope of this commit.
| ) -> Result<TrustedState<C, H>, Error> | ||
| req: &'a R, | ||
| mut cache: &'a mut Vec<TrustedState<C, H>>, | ||
| ) -> LocalBoxFuture<'a, Result<TrustedState<C, H>, Error>> |
There was a problem hiding this comment.
It is a bit unfortunate that the we had to change the method signature to return a boxed future. But this change is good in the sense that it surfaces that there is still a too tight coupling between verification logic and the requester (or networking / IO).
@brapse also suggested yesterday that we should probably implement the iterative version here too. It seems like this is a good idea for several reasons now.
|
|
||
| fn main() { | ||
| #[tokio::main] | ||
| async fn main() { |
| tokio = "0.2" | ||
|
|
||
| async-trait = "0.1" | ||
| tokio = { version = "0.2", features = ["full"] } |
There was a problem hiding this comment.
Just to double check: do we need all Tokio public API features? Is there a good reason to enable them all anyways?
There was a problem hiding this comment.
Yeah sorry, macros should be sufficient (as in #173).
| #[test] | ||
| fn test_verify_single_skip_1_val_verify() { | ||
| #[tokio::test] | ||
| async fn test_verify_single_skip_1_val_verify() { |
There was a problem hiding this comment.
Do the tests which don't use async functions need to be async?
There was a problem hiding this comment.
No, they don't. Maybe this wasn't intentional (only the bisection test use async functions).
Closes: #170
On top of the changes mentioned in the linked issue,
lite::verifier::verify_bisection_innerhad to be manually adapted to return a boxed future because recursive async fn are not currently supported in Rust.An possible alternative would be to rewrite
verify_bisection_innerto not be recursive anymore, but that is outside of the scope of this PR.