Async in Zebra RFC #1965

teor2345 · 2021-03-30T08:43:20Z

TODO

Atomics - use an abstraction, always choose the strongest ordering, or provide a correctness justification
Document that we should use futures-aware mutexes in async code: https://docs.rs/futures/0.3.13/futures/lock/struct.Mutex.html
- always qualify Mutex imports, e.g. std::sync::Mutex or futures::lock::Mutex
- prefer futures::lock::Mutex in async functions or blocks
- if you have to use std::sync::Mutex in async code, hold the mutex for as short a time as possible
- see Switch to an async mutex for handshake nonces #2031, Refactor and document correctness for std::sync::Mutex in ErrorSlot #2032 and Refactor and document correctness for std::sync::Mutex<AddressBook> #2033
Be specific about the conditions under which a mutex can never deadlock
- the code only acquires one mutex/readiness/etc., or mutexes are always acquired in the same order, and there are no circular dependencies
- the critical section (define) is short, and it doesn't depend on any other tasks (for example, it doesn't use await)
  - Rust will catch some instances of this, use a block to drop the mutex lock (example)
    - Add the CandidateSet mutex critical section as an example of Rust stopping us from holding the lock over an await
Add TurboWish to the testing / introspection sections as a possible future tool: https://blog.pnkfx.org/blog/2021/04/26/road-to-turbowish-part-1-goals/

Summary

Zebra executes concurrent tasks using async Rust, with the tokio executor.

At a higher level, Zebra also uses tower::Services, tower::Buffers, and our own tower-batch implementation.

Zebra programmers need to carefully write async code so it doesn't deadlock or hang. This is particularly important for poll, select, Buffer, and Batch.

More information

Feature Name: async_rust_constraints
Start Date: 2021-03-30
Zebra Issue: ZcashFoundation/zebra#1593

Document

Rendered

Zebra Team Approval

Everyone on the Zebra team should review design RFCs:

If no-one has time to review by 2021-04-09, let's merge this RFC as a draft.

Related Tickets

Closes #1593.

oxarbitrage

Looks good, i made a few suggestions as i will like to see the RFC as a reference guide where the developer/reviewer can refer to specific sections where the rules are set.

book/src/dev/rfcs/drafts/xxxx-async-rust.md

teor2345 · 2021-04-07T09:37:05Z

I think we're ready for a final review now.

book/src/dev/rfcs/drafts/xxxx-async-rust.md

teor2345 · 2021-04-30T02:13:38Z

This RFC is now ready for another round of review.

Here's what I changed since the last reviews:

added atomics
added futures-aware types
rewrote the acquiring locks section
did a rebase so the build jobs will succeed

Here are the changes since @oxarbitrage and @dconnolly last reviewed:
https://github.com/ZcashFoundation/zebra/pull/1965/files/2ce751c9dbd7f3dcc3742e6b812fb22b4924e6d5..154966e5071ad98c10481b69df6c532f14adbdac

dconnolly · 2021-05-01T23:22:19Z

book/src/dev/rfcs/drafts/xxxx-async-rust.md

+Zebra's [`Inbound service`](https://github.com/ZcashFoundation/zebra/blob/0203d1475a95e90eb6fd7c4101caa26aeddece5b/zebrad/src/components/inbound.rs#L238)
+can't use an async-aware mutex for its `AddressBook`, because the mutex is shared
+with non-async code. It only holds the mutex to clone the address book, reducing
+the amount of time that other tasks on its thread are blocked:


oxarbitrage

Please rename xxxx-async-rust.md to 0011-async-rust.md and move out from drafts folder.

Rest looks good.

teor2345 · 2021-05-03T19:35:26Z

@conradoplg you might also want to read the examples in this RFC. It should help you write or troubleshoot async Rust.

teor2345 · 2021-05-03T19:36:03Z

Feel free to ask questions, and let me know if anything is unclear.

teor2345 · 2021-05-03T23:08:03Z

@oxarbitrage I moved and renamed the file, this PR will auto-squash and merge once you approve it.

dconnolly · 2021-05-03T23:36:56Z

book/src/dev/rfcs/drafts/xxxx-async-rust.md

+If the lock isn't dropped, compilation fails, because the mutex lock can't be
+sent between threads.


dconnolly · 2021-05-03T23:50:41Z

book/src/dev/rfcs/drafts/xxxx-async-rust.md

+If you are unable to use futures-aware types:
+- block the thread for as short a time as possible
+- document the correctness of each blocking call
+- consider re-designing the code to use `tower::Services`, or other futures-aware types


dconnolly · 2021-05-03T23:51:14Z

book/src/dev/rfcs/drafts/xxxx-async-rust.md

+- make critical sections as short as possible, and
+- do not depend on other tasks or locks inside the critical section.


dconnolly · 2021-05-03T23:52:02Z

book/src/dev/rfcs/drafts/xxxx-async-rust.md

+In Zebra, we try to use safe abstractions, and write obviously correct code. It takes
+a lot of effort to write, test, and maintain low-level code. Almost all of our
+performance-critical code is in cryptographic libraries. And our biggest performance
+gains from those libraries come from async batch cryptography.


dconnolly · 2021-05-03T23:52:08Z

book/src/dev/rfcs/drafts/xxxx-async-rust.md

@@ -505,3 +679,6 @@ those bugs can take a lot of developer effort.
 Can we catch these bugs using automated tests?

 How can we diagnose these kinds of issues faster and more reliably?
+  - [TurboWish](https://blog.pnkfx.org/blog/2021/04/26/road-to-turbowish-part-1-goals/)


dconnolly

Looks great!

teor2345 added A-docs Area: Documentation C-design Category: Software design work S-needs-triage Status: A bug report needs triage NU-5 Network Upgrade: NU5 specific tasks P-Low labels Mar 30, 2021

teor2345 added this to the 2021 Sprint 6 milestone Mar 30, 2021

teor2345 requested a review from a team March 30, 2021 08:43

teor2345 self-assigned this Mar 30, 2021

teor2345 modified the milestones: 2021 Sprint 6, 2021 Sprint 7 Mar 31, 2021

oxarbitrage reviewed Apr 5, 2021

View reviewed changes

teor2345 commented Apr 5, 2021

View reviewed changes