New GetChainBlockNo and GetChainPoint queries

[newhoggy]

No description provided.

[dcoutts]

So if we're adding GetChainBlockNo and GetChainPoint queries then we do not need one for the tip. So we can get rid of the patch that adds GetHeaderStateTip and we don't need the corresponding tip type that's added there.

I think we can also get rid of the dependency on the serialise package.

I think we ought to be able to shrink this PR somewhat. Check we really need all the new code we're adding.

I agree that the right way to structure this PR is as two patches, the first that only adds support for another protocol version, and then the second that adds the two new queries.

[jasagredo]

It looks good to me, though I coincide with Duncan's comments. I just made some minor comments on style and alignment.

[dnadales]

This will change the distribution of what QC generates, won't it? So we'll go from a 15 to 1 to a 15 to 3 block query to "with version" generated data. Is this desired?

[newhoggy]

What principle should be followed for this kind of decision making?

[dnadales]

Ideally, we'd have coverage testing which would safeguard that we still cover a sufficient percentage of the cases we consider important. I'll ping you on slack to discuss this further.

Addressed concerns.

[dnadales]

One thing that I'd like to discuss in a call is why not just hardcoding the frequencies as it used to be

[dnadales]

So, in particular, I was wondering if it'd be possible to write something like:

    case queryVersion of
      Query.TopLevelQueryDisabled -> arbitraryBlockQuery queryVersion
      Query.QueryVersion1         -> arbitraryBlockVersion1Query
      Query.QueryVersion2         -> arbitraryBlockVersion1Query

  where
    query item = do blockV <- arbitrary
                     return (WithVersion (queryVersion, blockV)
                                         (SomeSecond item)))
    arbitraryBlockVersion1Query =
      frequency
        [ (15, arbitraryBlockQuery queryVersion)
        , (1,  query GetSystemStart)))
        ]
    arbitraryBlockVersion2Query =
      oneof
        [ arbitraryBlockVersion1Query
        , frequency [ (1, query GetChainBlockNo)
                    , (1, query GetChainPoint)
                    ]
        ]

[dnadales]

Or repeating code, but removing one level of indirection for the reader so that it is easier for them to understand what the frequency distributions are:

    arbitraryBlockVersion2Query =
      frequency
        [ (15, arbitraryBlockQuery queryVersion)
        , (1,  query GetSystemStart)))
        , (1, query GetChainBlockNo)
        , (1, query GetChainPoint)
        ]

[newhoggy]

How does it look now?

[nfrisby]

I shared some minor-to-medium observations, mostly around avoiding code duplication.

[nfrisby]

I'm thinking out loud here, in case someone sees something objectionable.

It's fine to bump this, since it's inconsequential whether the next release/release candidate tries to negotiate this new version. Esp: if they talk to a node that doesn't support it, they'll simply negotiate the eg previous version. Thus, as long as the cardano-node dependence on these new queries is well-guarded wrt to the negotiated version, then it's fine to "enable" them here.

[nfrisby]

I pointed out enough small-to-medium things that I'm Requesting Changes.

Also: apologies for not grouping all my comments into this review; I made a few before realizing there'd be more than a couple.

[nfrisby]

We have a general preference for ToCBOR and FromCBOR instead of Serialise. But using Serialise here is not without precedent (eg BlockQuery ShelleyBlock uses Serialise for a Point's HeaderHash).

Ultimately, the Serialise vs ToCBOR/FromCBOR vs SerialiseNodeToClient/SerialiseNodeToNode vs SerialiseResult is in a confused and confusing state, in my opinion. The only stark contrast is that many "low-level" Serialise instances come from 3rd party libraries, and so we shouldn't be using them: changes in our codecs should never be accidental, and so shouldn't run the risk of accidentally depending on 3rd party serializations.

So: if it's not too much trouble, I would prefer at least ToCBOR and FromCBOR instead of Serialise. But really, this is already in the scope of existing tech debt :(

[newhoggy]

If it's okay I'd like to do this in a separate PR.

Review comments addressed.

[nfrisby]

@dnadales pointed out on Slack that this PR is enriching our interface, and so deserves an entry in https://github.com/input-output-hk/ouroboros-network/blob/master/ouroboros-consensus/docs/interface-CHANGELOG.md 🙌

I'm sorry I didn't realize that sooner.

@newhoggy Would you add a section at the top there. I'm guess it'll be similarly formatted to the latest one (ie Circa 2021-08-31). If the top of the interface-CHANGELOG.md file doesn't give you enough guidance, please let me know 👍

[dnadales]

bors r+

[iohk-bors]

Build succeeded:

• ci/hydra:Cardano:ouroboros-network:required
• buildkite/ouroboros-network