Move block::Header validation checks into zebra-consensus.

[hdevalence]

Same rationale as #907, but per #917 (comment) there might be an obstacle.

There are currently two checks:

• is_equihash_solution_valid

  • This function should be split into two parts:
    • the work of checking the solution should move to a new method on Solution in work/equihash.rs:

      #[non_exhaustive]
      #[derive(Debug, thiserror::Error)]
      #[error("invalid equihash solution for BlockHeader")]
      pub struct Error(#[from] equihash::Error);
      
      impl Solution {
          pub fn check(header: &block::Header) -> Result<(), Error> {
              // do the check
          }
      }
      

    • the block::check module should have

      pub fn is_equihash_solution_valid(header: &block::Header) -> Result<(), work::equihash::Error> {
          header.solution.check(&header)
      }
      

      This might seem pointless, but it means that all of the consensus checks have the same interface: block::check::name_of_check.
    • the EquihashError in block/header.rs should be removed (it moves to work/equihash.rs above).

• is_time_valid_at

  • move the tests that don't rely on generate to zebra-consensus
  • move is_time_valid_at to zebra-consensus
  • keep the tests that rely on generate in zebra-chain, and open a ticket to fix them (Combine proptests and transcripts to perform top-level integration tests #1021)
  • detailed explanation:
    • is_time_valid_at is also used in test code, specifically chain/src/block/tests/vectors.rs.
    • These tests should be moved into the consensus crate.
    • Some of the tests can't be moved, because they rely on the generate code, which isn't exposed to the rest of the crate API
    • The generate code should be expressed as proptest strategies (Rewrite block::tests::generate as proptest strategies. #919) to fix this.
    • Then consensus tests can be written using the proptest strategies.
    • In fact, rather than writing unit tests, we could combine proptests with the transcript functionality to perform top-level integration tests.
    • Doing this correctly requires some amount of work but will save a lot of work compared to writing individual unit tests.
    • Rather than blocking correct code organization on the test refactoring, a better move would be to split the tests, leaving the dependent tests in zebra-chain

[vramana]

Can I work on this?

[yaahc]

Can I work on this?

absolutely!

[teor2345]

@hdevalence, I'd like to suggest a different change for is_time_valid_at:

is_time_valid_at is used as a consensus rule, and in one particular test, but that particular test doesn't rely on generate:

zebra/zebra-consensus/src/block.rs

Line 110 in 8539302

block.header.is_time_valid_at(now)?;

zebra/zebra-chain/src/block/tests/vectors.rs

Line 163 in 8e15c16

.is_time_valid_at(now)

So instead of commenting out the tests, we could:

• move the tests that don't rely on generate to zebra-consensus
• move is_time_valid_at to zebra-consensus
• keep the tests that rely on generate in zebra-chain, and open a ticket to fix them

(I don't like to rely on code comments for future work, because they aren't actually tracked anywhere.)

[hdevalence]

Cool, that sounds like a better plan!

[teor2345]

I have updated the issue description.