ZcashFoundation / zebra

Zcash - Financial Privacy in Rust 🦓
https://zfnd.org/zebra/
Apache License 2.0
401 stars 92 forks source link

Add proptest for `AddBlock` <-> `GetBlock` roundtrip #418

Closed yaahc closed 3 years ago

yaahc commented 4 years ago

Similar to this test, it may be worthwhile to think about a proptest for Request::AddBlock/GetBlock pair, the property that we are testing being, if we successfully add a block, we should successfully get a block, for any valid AddBlock/GetBlock pair. This is a variant of testing "there and back again", pairing an operation with its inverse, though not required to merge this, we can open an issue for it. @hdevalence

Originally posted by @dconnolly in https://github.com/ZcashFoundation/zebra/pull/414

hdevalence commented 4 years ago

I wonder whether it might be better to hold off on doing property testing until we're a) further along in figuring out what the API should be and b) working on the "real" backend using whatever persistent storage system we choose. At that point, we could do a much more sophisticated testing strategy:

  1. Use the existing in-memory backend as a simulator for the persistent backend;
  2. Write a data type that describes changes to the state or different requests and responses;
  3. Generate random streams of state changes;
  4. Check invariants against the real backend and the in-memory backend.

This has kind of a different code shape than individual round-trip proptests, so I'm not sure how much common code there is. On the other hand, because the in-memory backend we're using as a stub right now is eventually only going to be used for testing, if we spend a bunch of effort now on testing it super rigorously, it's not super clear to me how well that will translate into assurance about the real backend.

teor2345 commented 4 years ago

because the in-memory backend we're using as a stub right now is eventually only going to be used for testing

Here are two potential use cases for an in-memory backend:

  1. I make Zcash transactions using a web wallet app, without access to persistent storage.
  2. I make shielded Zcash transactions using an anonymous wallet app, without any disk traces.

In both cases, we'd probably need to do something like:

I don't know if these use cases are worth the complexity of maintaining two first-class backends. (If our database backend can live in memory, then we don't need to keep the in-memory backend as a first class backend.)

hdevalence commented 4 years ago

I think both of those cases aren't quite the target of zebra-state, since they're cases where an application wants to interact with the chain without having to keep a copy of the chain state protocol, so rather than using zebra-state it might be better for them to use a light client protocol instead, which is aimed more at the wallet use case.

teor2345 commented 4 years ago

rather than using zebra-state it might be better for them to use a light client protocol instead, which is aimed more at the wallet use case.

Thanks, I opened issue #425 and PR #426 to document that design choice. (It wasn't clear to me in the docs, but that could just be because I'm new!)

hdevalence commented 4 years ago

@teor2345 Yeah, I think part of the problem with the explanation in the docs is that we have on the one hand zebrad which is supposed to do a bunch of fullnode stuff, and components (like zebra-network or zebra-chain) that don't actually require full node state and are conceivably useful outside of that context. But in the latter case, because we're not finished with them and don't have clear applications other than zebrad and the pegzone yet, we're not really highlighting the component reusability as much.

hdevalence commented 3 years ago

This test might not make sense in light of the evolution of the state design.