Skip to content

Conversation

erikd
Copy link
Contributor

@erikd erikd commented May 19, 2020

No description provided.

@erikd erikd requested review from edsko and mrBliss May 19, 2020 05:10
@erikd erikd force-pushed the erikd/query-ledger-state branch from 846dab3 to 6df124c Compare May 19, 2020 05:14
Copy link
Contributor

@mrBliss mrBliss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, @edsko and I think this is wrong:

  • The LedgerState is huge
  • The whole point of the LocalStateQuery protocol is to avoid sending the whole ledger across the wire and to send small queries and small results across the wire instead.
  • Whenever we change the LedgerState type, we would break network compatibility.

Why is the whole LedgerState needed? Wouldn't another query (or multiple) solve the real issue?

@erikd erikd force-pushed the erikd/query-ledger-state branch 2 times, most recently from 41ecb02 to 894a856 Compare May 20, 2020 03:58
GetStakeDistribution -> toCBOR
GetFilteredUTxO {} -> toCBOR
GetUTxO -> toCBOR
GetCurrentLedgerState -> wrapCBORinCBOR encode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you just use toCBOR instead of encode (and fromCBOR instead of decode)? Then you won't need all these orphans below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let also add a comment saying why we use CBOR-in-CBOR for this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not wrapCBORinCBOR toCBOR? (Or does that not work?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not work.

@erikd erikd force-pushed the erikd/query-ledger-state branch 5 times, most recently from 45e7139 to f4f6f6b Compare May 20, 2020 09:09
@edsko
Copy link
Contributor

edsko commented May 20, 2020

I don't understand the need and usefulness of CBOR-in-CBOR here. If it's CBOR-in-CBOR, the client becomes responsible for deserialization, rather than us providing the serializer; this means that if the client writes their own deserializer, we are even more restricted in how we change stuff, it doens't really seem to help with compatiblity. And if not for compatibility, then why would clients want the raw CBOR?

, cborg >=0.2.2 && <0.3
, containers >=0.5 && <0.7
, cryptonite >=0.25 && <0.26
, iproute
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not be needed anymore

Comment on lines 559 to 570
fromCborInCbor :: FromCBOR a => Decoder s a
fromCborInCbor = do
tag <- CBOR.decodeWord8
when (tag /= 24) $ fail "expected tag 24 (CBOR-in-CBOR)"
fromCBOR

toCborInCbor :: ToCBOR a => a -> Encoding
toCborInCbor x =
mconcat
[ toCBOR (24 :: Word8)
, toCBOR x
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As commented elsewhere: I prefer we use wrapCBORinCBOR toCBOR instead of copy-pasting its implementation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The types of wrapCBORinCBOR and toCborInCbor are different as are their implementations!

Copy link
Contributor Author

@erikd erikd May 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrapCBORinCBOR :: (a -> Encoding) -> a -> Encoding

and

toCborInCbor :: ToCBOR a => a -> Encoding

The former does not have the ToCBOR a constraint.

@edsko
Copy link
Contributor

edsko commented May 20, 2020

So @mrBliss explained to me that the idea here is that if we don't use CBOR-in-CBOR, the client running the local state query protocol will disconnect if the serialization format has changed and it gets a CBOR error, whereas the CBOR-in-CBOR forces the client to do the decoding and therefore take alternative action if that decoder fails. Ok, seems reasonable, but then I do wonder, if the client is asking for a UTxO, but it can't decode it, what kind of alternative action will it take? Is there something useful it can do? Because if not, then just getting the disconnected error doesn't seem a whole lot worse.

It also feels a bit ad-hoc by the way to have CBOR-in-CBOR for this one specific query. If this really is seful, I'd prefer something like

GetSerialised :: Query a -> Query (Serialised a)

@erikd
Copy link
Contributor Author

erikd commented May 20, 2020

There may be other queries that require CBOR-in-CBOR.

The `GetCurrentLedgerState` query is mainly for debugging. The UTxO is removed
from the ledger state to shrink the size of the result and because the UTxO is
already available via another command.

If the client communicating with the node is not using the same version of
this code, decoding the ledger state may fail. For that reason, we add the
`GetCBOR` query, which takes another query as argument and will use
CBOR-in-CBOR to encode the result so that the client can dump it as decoded
CBOR if the full decode fails.

Co-authored-by: Thomas Winant <thomas@well-typed.com>
@mrBliss mrBliss force-pushed the erikd/query-ledger-state branch from f4f6f6b to 61eeee1 Compare May 20, 2020 10:25
Copy link
Contributor

@mrBliss mrBliss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work! 😏

@mrBliss
Copy link
Contributor

mrBliss commented May 20, 2020

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 20, 2020

@iohk-bors iohk-bors bot merged commit 7da0540 into master May 20, 2020
@iohk-bors iohk-bors bot deleted the erikd/query-ledger-state branch May 20, 2020 10:40
coot pushed a commit that referenced this pull request May 16, 2022
2113: Shelley: Implement GetCurrentLedgerState query r=mrBliss a=erikd



Co-authored-by: Erik de Castro Lopo <erikd@mega-nerd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants