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

Off To The Races #3956

Open
zebambam opened this issue Apr 16, 2019 · 0 comments
Open

Off To The Races #3956

zebambam opened this issue Apr 16, 2019 · 0 comments
Labels
epic Represents a multi-sprint effort (auto-generated label) I-race Problems and improvements related to race conditions.

Comments

@zebambam
Copy link

Zcash inherits C++ (unsafe language) code from bitcoin, with a threading model (threads, locks, global state) that nobody at Zcash has been able to explain to me.

To be successful in developing secure multithreaded code in C++, you need an absolutely solid threading model to avoid obscure concurrency bugs that result in either memory corruption or memory disclosure to remote attackers.

Meantime, making changes to code when we're not fully aware of the consequences of those changes is insanely dangerous from a security point of view. Concurrency generally go undetected until they are triggered accidentally, many aren't practically possible to trigger in the "usual run of things" unless you maliciously attempt to cause them, but are still very dangerous.

Benefits of this change:

  1. Every change we make to the C++ layer incurs a significantly higher cost than an equivalent change would take if the code were correctly modularized with a well designed, documented and therefore simple-to-reason-about threading and global state model.
  2. The lost velocity translates to slower market fit, slower feature addition and slower turnaround of fixes.
  3. Overall product quality suffers significantly because of the lack of testability of subsystems that rely on a diffuse global state.
  4. Corrupted state and wallets could be eradicated much faster by moving to a concise threading, locking and state model for Zcashd (because we could implement a crash-only shutdown model where consistency is always maintained FAR more simply when we know what global state we are in).

Suggested course of action:

a) We start a branch that:

  • defines global state from the C++ layer into a serializable/deserializable context object
  • defines valid state transitions through a concise state machine with simple-to-understand locking
    b) We run existing unit tests on that branch
    c) Develop subsystem-level tests that demonstrate, for instance, checking ConnectBlock for counterfeiting issues
    d) We deploy that branch to testnet
    e) We merge the branch to master and include it in the next release after that
@zebambam zebambam added epic Represents a multi-sprint effort (auto-generated label) I-race Problems and improvements related to race conditions. labels Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
epic Represents a multi-sprint effort (auto-generated label) I-race Problems and improvements related to race conditions.
Projects
None yet
Development

No branches or pull requests

1 participant