Deterministic Pact SPV test

[sirlensalot]

No description provided.

[larskuhtz]

Could you also enable the Pact.SPV tests in .github/workflows/applications.yaml?

[larskuhtz]

Have you tried if the test works with calling createNewCut?

I think, the same can be achieved by using a chainweb version that has trivial target and doesn't do DA. I think that's the case for most test versions.

[sirlensalot]

It's possible, but that's not the intention. The point here is to use mocking techniques to avoid violating separation-of-concerns in unit tests.

Using ChainwebVersion is less desirable, as it is overloaded with three separate concerns:

1. A protocol version. This in itself (protocol version compliance) is not of interest to many subsystems.
2. A graph. This is relevant to multi-chain-aware subsystems.
3. A hashing algorithm/DA/difficulty regime. This is relevant to mining- and DA-concerned subsystems.

Notice how all three of these properties are irrelevant to some single-chain subsystem that is not involved in mining or DA or protocols.

Here, by mocking out the nonce validation, a test is able to precisely specify whether 3 is of concern. (const True) (maybe with a suitably literate type synonym) is the most accurate way to say that mining difficulty is irrelevant, in which case it is strictly better as it makes a test less brittle if later on it needs to change something regarding 2 or 1.

Finally, leaving it to ChainwebVersion means that the only true safe way to use these functions is via a mine function, in case the protocol requires mining. This necessarily involves a clock, randomness etc, and is explicitly what I wanted to avoid.

[larskuhtz]

Short reply:

the test already depends on being used with a chainweb version that uses a trivial POW target, because insert does POW validation. Passing const True seems thus redundant.

Long reply:

The consistency rules for a chainweb are part of the chainweb version, which are implemented by the validation rules. Validation includes:

1. block header validation (intrinsic and indicutive)
2. cut validation (braiding structure of chainweb)
3. payload validation (construction of payload Merkle tree, tx uniqueness, tx timing validation, gas, etc.)
4. pact validation (computation of outputs via pact validation)

In the original design we used parameterized test chainweb versions to tweak consistency properties for testing. In most cases properties are statically bound to a particular version. Currently only the graph can be changed freely on some test versions. But @fosskers and I have been discussing to add free more parameters, like for instance the block rate.

In the current code we don't follow this original design everywhere, but we use chainweb versions to disable certain validation properties. For instance most test versions already use a trivial POW target, which is equivalent to passing const True to the POW validator.

[larskuhtz]

The idea behind using the chainweb version to relax validation constraints is to gather all configurations in a single place (Chainweb.Version) and avoid accidentally disabling or changing consistency properties. We try to make it impossible (or at least hard) to run code with chainweb version Mainnet01 (or any other production version) with some consistency rules altered or disabled.

[sirlensalot]

Hmm, I got confused by the check in the code here, I didn't realize insert did it as well.

In that case the indirection is indeed unnecessary. On the larger topic above, I might want to allow tests to set the target to 0 (irrespective of what comes out of newHeader and it's merkle hash), but that's for a later debate. I'll switch to using createNewCut and drop this function.

[larskuhtz]

👍

[larskuhtz]

Just as a note: It might make sense to base the actual CutDB on a data type like this and add all reactive functionality as an extra layer.

That's out of scope for this PR. Just an idea to remember for future cut db refactoring.

[sirlensalot]

That's very interesting. This was the "natural" result of finding the common dependencies of the SPV test, combined with the MVar Cut to simply use "current cut" as a shortcut for having a "latest block" concept.

But now that you mention it, it does resemble CutDB (swap a TVar in anyway)!

In general I was very pleased with how easy this refactoring was, which speaks extremely highly of the underlying design.

[sirlensalot]

Also I would be happy to do your suggested refactoring. In general I am very glad to be getting to know these parts of the code.

[larskuhtz]

Just be aware that it might lead you to the less pleasant territories of the underlying design :-) Namely the code for validating remote cuts and fetching dependencies. But separating that from a local-only cut db might be useful.

[larskuhtz]

Suggested change

-- print (_blockHeight ph, cid)

[sirlensalot]

This is a useful debug output so I wanted to keep it. Should I instead make it a Debug log?

[larskuhtz]

Just ignore the suggestion. I was just not sure if the comment was there intentionally or just forgotten.

[sirlensalot]

on second thought I'll just remove it

[larskuhtz]

This function seems very useful. Could you add a comment that describes what the functions does?

I think this code is useful beyond the scope of the tests in this module. Maybe move it to a a new test utils module (e.g. Chainweb.Test.Utils.Cuts) to make it easier to reuse?

[sirlensalot]

I'll comment and move it to Chainweb.Test.Pact.Utils alongside testWebPactExecutionService.

I'm not so sure about CutOutputs, do you think that's worth promoting? If not this function can simply be IO ().

[larskuhtz]

Not sure how useful the outputs are. However, throwing in a void on the use side isn't a big deal.

[sirlensalot]

Hmm ... the problem with this function is it hardcodes the requirements of addPayloadBlock above ...

[sirlensalot]

I'd need to add a "payload adder" indirection basically with the signature of addPayloadBlock. Do you think that isn't too wonky for promotion?

[sirlensalot]

I'll try it out and see.

[larskuhtz]

Does testWebPactExecutionService synchronize the pact with the current cut? It calls evalPactServiceM_ ctx (initialPayloadState dummyLogger v cid), but I am not sure if that's enough to fully initialize the checkpointer.

[larskuhtz]

The following code is used in a chainweb node to sync the checkpointer. I don't know if this is needed, too, in case bh is the genesis block.

        syncOne :: (BlockHeader, ChainResources logger) -> IO ()
        syncOne (bh, cr) = do
            let pact = _chainResPact cr
            let logCr = logFunctionText $ _chainResLogger cr
            let hsh = _blockHash bh
            let h = _blockHeight bh
            logCr Info $ "pact db synchronizing to block "
                      <> T.pack (show (h, hsh))
            payload <- payloadWithOutputsToPayloadData
                       <$> casLookupM payloadDb (_blockPayloadHash bh)
            void $ _pactValidateBlock pact bh payload
            logCr Info "pact db synchronized"

[sirlensalot]

Not sure, I'll investigate.

[larskuhtz]

it may also be just a corner case related to starting with the genesis blocks. In production networks there probably never will be normal user txs at single digit block heights.