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

Refactor Network and NetworkUpgrade enums into structs and a trait #1135

Closed
2 of 3 tasks
Tracked by #745
yaahc opened this issue Oct 8, 2020 · 4 comments
Closed
2 of 3 tasks
Tracked by #745

Refactor Network and NetworkUpgrade enums into structs and a trait #1135

yaahc opened this issue Oct 8, 2020 · 4 comments
Labels
C-enhancement Category: This is an improvement S-needs-design Status: Needs a design decision

Comments

@yaahc
Copy link
Contributor

yaahc commented Oct 8, 2020

Is your feature request related to a problem? Please describe.

Right now the follow up PR for the implementation of the NonFinalizedState component in zebra-state is blocked on some questions about how to effectively test the CommitBlock request on the state service. The main issue is that we cannot create an appropriately brief test that would also be able to commit new non-finalized blocks. The state implementation only supports v4 transactions, which means we can only accept blocks from after the sapling network upgrade. For the two extant networks, Mainnet and Testnet, these network upgrades happened hundreds of thousands of blocks into the block chain.

Describe the solution you'd like

To resolve this I'd like to generalize our handling for Networks and how we represent them. Right now we represent networks as a closed set via an enum. We then match on the value of that enum to access the appropriate parameters of that network or to trigger the appropriate behaviour. If we could instead represent an open set via trait objects and or generic parameters we could then create arbitrary networks with parameters much more suited for small scale tests. Then we could create arbitrary blocks via proptest that are valid in the context of these new ephemeral test networks and use those to validate the behaviour of the state service.

As part of this change, we should also:

What are the constraints on this solution?

Any alternate consensus parameters or regtest mode would have to respect the constraints set by the Contextual Difficulty RFC's design.

In particular:

  • the PoWLimit must be less than or equal to (2^256 - 1) / PoWAveragingWindow (approximately 2^251) to avoid overflow,
  • the PoWAveragingWindow and PoWMedianBlockSpan are fixed by function argument types (at least until Rust gets stable const generics), and
  • the design eliminates a significant number of edge cases by assuming that difficulty adjustments aren't validated for the first PoWAveragingWindow + PoWMedianBlockSpan (28) blocks in the chain.

See the alternate consensus parameters section of the RFC for more details.

Describe alternatives you've considered

The alternatives that I've considered are either to introduce a new internal 3rd network variant to our enum or to rely on larger scale tests such as network sync tests to validate this behaviour.

The former alternative would touch all the same parts of the code that a trait based refactor would, so it does not save us much time or effort. The latter alternative gives us less control over what behaviour we're testing and seems like a less reliable option, though it may be advantageous in the short term.

Additional context

Relevant PR - #1119
Difficulty Adjustment constraints - https://github.com/ZcashFoundation/zebra/pull/1246/files#diff-3e636ad0596bc279407be7ac69096f4bea1da79cd5aa0ac76b7c4ef3f8b64ad9R708

@yaahc yaahc added C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage labels Oct 8, 2020
@teor2345
Copy link
Contributor

teor2345 commented Oct 8, 2020

Sounds like a good idea!

Let's make sure we have a reasonable amount of test coverage before doing this refactor. And if we add new tests before this refactor, let's try to separate the test cases (in a vector) from the test harness - in case we need to rewrite the test harness.

@teor2345 teor2345 changed the title Refactor Network enum into individual structs and a trait Refactor Network and NetworkUpgrade enums into structs and a trait Dec 4, 2020
@mpguerra mpguerra added this to the v1.0.0-alpha.1 milestone Dec 15, 2020
@teor2345 teor2345 added the S-needs-design Status: Needs a design decision label Dec 17, 2020
@teor2345 teor2345 removed this from the v1.0.0-alpha.1 milestone Dec 17, 2020
@mpguerra mpguerra added this to the Block Validation milestone Dec 22, 2020
@mpguerra mpguerra removed this from the Block Validation milestone Jan 5, 2021
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Feb 2, 2021
@teor2345
Copy link
Contributor

We don't need to do this, the current code works fine.

@teor2345 teor2345 reopened this Jul 2, 2023
@teor2345 teor2345 closed this as not planned Won't fix, can't repro, duplicate, stale Jul 2, 2023
@teor2345 teor2345 reopened this Jul 2, 2023
@teor2345
Copy link
Contributor

teor2345 commented Jul 2, 2023

This might be relevant to #7119

@teor2345
Copy link
Contributor

Closing this in favour of #7119 or its replacements. I have copied useful information from this ticket into #7119.

@teor2345 teor2345 closed this as not planned Won't fix, can't repro, duplicate, stale Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: This is an improvement S-needs-design Status: Needs a design decision
Projects
None yet
Development

No branches or pull requests

3 participants