Introduction of serialization instances in support of ledger peer snapshot #4851
Conversation
ouroboros-network/src/Ouroboros/Network/PeerSelection/LedgerPeers.hs
Outdated
Show resolved
Hide resolved
ouroboros-network-api/src/Ouroboros/Network/PeerSelection/RelayAccessPoint.hs
Show resolved
Hide resolved
ouroboros-network-api/src/Ouroboros/Network/PeerSelection/RelayAccessPoint.hs
Outdated
Show resolved
Hide resolved
ouroboros-network-api/src/Ouroboros/Network/PeerSelection/RelayAccessPoint.hs
Show resolved
Hide resolved
ouroboros-network-api/src/Ouroboros/Network/PeerSelection/RelayAccessPoint.hs
Outdated
Show resolved
Hide resolved
ouroboros-network-api/src/Ouroboros/Network/PeerSelection/RelayAccessPoint.hs
Outdated
Show resolved
Hide resolved
crocodile-dentist
force-pushed
the
mwojtowicz/cbor-instances-peer-snapshot
branch
2 times, most recently
from
0afa950
to
2a2fa6c
Compare
crocodile-dentist
force-pushed
the
mwojtowicz/cbor-instances-peer-snapshot
branch
2 times, most recently
from
5f20a4e
to
442b306
Compare
This change tidies up a function signature to make use of records that were created previously for use by other functions. They bundle many common related values together to facilitate passing them around.
This change binds useLedgerPeers after unblocking due to a demand for ledger peers by its clients. If this value has changed in the configuration while the thread was dormant, ie. a race, it will be picked up when the function unblocks.
Moved utility functions to ouroboros-network-api to support calculating big ledger peer snapshots by upstream libraries, for eg. Genesis consensus mode and bootstrapping a node with a recent snapshot of these peers.
This change adds ToCBOR and FromCBOR instances that are necessary to serialize a snapshot of ledger peers.
crocodile-dentist
force-pushed
the
mwojtowicz/cbor-instances-peer-snapshot
branch
from
442b306
to
5e98e10
Compare
| -- | These instances are used to serialize 'LedgerPeerSnapshot' | ||
| -- consensus LocalStateQuery server which uses these instances | ||
| -- for all its query responses. It appears they provide some improved | ||
| -- debugging diagnostics over Serialize instances. | ||
| instance ToCBOR RelayAccessPoint where | ||
| toCBOR = \case | ||
| RelayAccessDomain domain port -> | ||
| encodeListLen 3 | ||
| <> encodeWord8 0 | ||
| <> serialise' port | ||
| <> toCBOR domain | ||
| RelayAccessAddress (IP.IPv4 ipv4) port -> | ||
| encodeListLen 3 | ||
| <> encodeWord8 1 | ||
| <> serialise' port | ||
| <> toCBOR (IP.fromIPv4 ipv4) | ||
| RelayAccessAddress (IP.IPv6 ip6) port -> | ||
| encodeListLen 3 | ||
| <> encodeWord8 2 | ||
| <> serialise' port | ||
| <> toCBOR (IP.fromIPv6 ip6) | ||
| where | ||
| serialise' = toCBOR . toInteger | ||
|
|
||
| instance FromCBOR RelayAccessPoint where | ||
| fromCBOR = do | ||
| listLen <- decodeListLen | ||
| when (listLen /= 3) . fail $ "Unrecognized RelayAccessPoint list length " | ||
| <> show listLen | ||
| constructorTag <- decodeWord8 | ||
| port <- fromInteger <$> fromCBOR @Integer | ||
| case constructorTag of | ||
| 0 -> do | ||
| domain <- fromCBOR | ||
| return $ RelayAccessDomain domain port | ||
| 1 -> do | ||
| ip4 <- IP.IPv4 . IP.toIPv4 <$> fromCBOR | ||
| return $ RelayAccessAddress ip4 port | ||
| 2 -> do | ||
| ip6 <- IP.IPv6 . IP.toIPv6 <$> fromCBOR | ||
| return $ RelayAccessAddress ip6 port | ||
| _ -> fail $ "Unrecognized RelayAccessPoint tag: " <> show constructorTag |
There was a problem hiding this comment.
Is this according to the current CDDL spec?
There was a problem hiding this comment.
Also CBOR codecs in ouroboros-network-protocols where the cddl spec is defined and the codecs are defined use:
import Codec.CBOR.Decoding qualified as CBOR
import Codec.CBOR.Encoding qualified as CBOR
and the Codec type. Take a look at how things are done there. I am not sure about defining the CDDL, though because this project doesn't deal with it... hmm
There was a problem hiding this comment.
I haven't seen the CDDL spec for this, but I could create one if this is a requirement. I guess we should establish who should own this spec in the first place, us, node or consensus?
| @@ -51,7 +52,7 @@ isLedgerPeersEnabled UseLedgerPeers {} = True | |||
| -- | |||
| newtype PoolStake = PoolStake { unPoolStake :: Rational } | |||
| deriving (Eq, Ord, Show) | |||
| deriving newtype (Fractional, Num, NFData) | |||
There was a problem hiding this comment.
I think it would be better to write the instance by hand and document it on the CDDL spec.
There was a problem hiding this comment.
One wrinkle here is that is you look at Slot in cardano-slotting, the SlotNo type, which is a component of the snapshot, has a newtype automatically derived instance for Serialise, which is leveraged by the ToCBOR instance. Same for WithOrigin. And so I might as well bite the bullet and keep my auto derived instances as well I think.
There was a problem hiding this comment.
So I've issued a PR to cardano-base so that hand written instances can be made for the WithOrigin type for recording the slot number of the snapshot.
Description
This change adds support for serialization of ledger peers by defining instances for its subcomponent types. The snapshot type will be introduced in a future commit.
Checklist