Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the LocalStateQueryServer #1507

Merged
merged 11 commits into from
Jan 29, 2020
Merged

Conversation

mrBliss
Copy link
Contributor

@mrBliss mrBliss commented Jan 27, 2020

Closes #1366.

@mrBliss mrBliss added networking consensus issues related to ouroboros-consensus labels Jan 27, 2020
@mrBliss mrBliss requested review from edsko and dcoutts January 27, 2020 13:38
@mrBliss
Copy link
Contributor Author

mrBliss commented Jan 27, 2020

TODO:

  • Write tests for the LocalStateQueryServer

@mrBliss mrBliss force-pushed the mrBliss/local-state-query branch 3 times, most recently from 0554eae to 5b07c4f Compare January 28, 2020 07:36
@mrBliss mrBliss marked this pull request as ready for review January 28, 2020 07:37
class QueryLedger blk where

-- | Different queries supported by the ledger
data family Query blk :: *
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make this is type class parameter instead so that we can have multiple Query types for a single blk. Result can remain a data family.

@@ -1101,6 +1108,7 @@ type LimitedApp' m peer blk unused1 unused2 =
(AnyMessage (TxSubmission (GenTxId blk) (GenTx blk)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turn this into a Lazy.ByteString to be uniform and compatible with the plan to use a single functor (either Const Lazy.ByteString or AnyMessage) for NetworkApplication.

@mrBliss mrBliss force-pushed the mrBliss/local-state-query branch 2 times, most recently from 602e134 to 3739854 Compare January 29, 2020 08:54
Copy link
Contributor

@edsko edsko left a comment

Choose a reason for hiding this comment

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

Nice, great to have a typed solution to this :)

Querying the ledger state can only happen for a ledger state corresponding to
a block, not to a ledger.
@mrBliss mrBliss force-pushed the mrBliss/local-state-query branch from 3739854 to 50f3dd1 Compare January 29, 2020 09:36
@mrBliss
Copy link
Contributor Author

mrBliss commented Jan 29, 2020

@coot @dcoutts Can we merge this without breaking network compatibility if we don't change the NodeToClientVersion and the NodeToClientVersionData? I.e., would that be the same as implicitly supporting the new node-to-client protocol without advertising it?

@mrBliss mrBliss requested a review from coot January 29, 2020 13:43
@mrBliss
Copy link
Contributor Author

mrBliss commented Jan 29, 2020

@coot Edsko already reviewed the consensus side of this PR. You only have to look at the changes in ouroboros-network.

@mrBliss mrBliss force-pushed the mrBliss/local-state-query branch from 50f3dd1 to f0fa03a Compare January 29, 2020 13:52
@mrBliss
Copy link
Contributor Author

mrBliss commented Jan 29, 2020

@coot @dcoutts Can we merge this without breaking network compatibility if we don't change the NodeToClientVersion and the NodeToClientVersionData? I.e., would that be the same as implicitly supporting the new node-to-client protocol without advertising it?

Answer: "network compatibility" is determined by the NodeToClientProtocols data type. So as long as we don't change it yet, we can merge this. I'll create a separate PR that enables the protocol.

This new abstraction replaces `getPastLedger` and can, in the future, allow
for more efficient rolling forward.

For example, when requesting a ledger state 100 blocks after some ledger
snapshot we have in memory, we'll need to apply 100 blocks to the ledger. This
won't change with the `LedgerCursor` abstraction, but if we then starting
rolling forward and requesting the ledger states 101, 102, 103, ... blocks
after our in-memory ledger snapshot, we only have to apply one block each
time, instead of having to apply 101, 102, 103, ... blocks each time!

This optimisation has not yet been implemented, but by introducing the
abstraction now, we can later implement it without having to change the API.
It was storing both the last applied point (slot + hash) and hash. We only
need to store the former.
This will be used for the LocalStateQuery protocol.
As the `LocalStateQueryServer` will send this over the network in the form of
`Result ByronBlock`, we must be careful not to break network compatibility
with clients, hence the golden test.
To actually enable it, we still need to add it to `NodeToClientProtocols`.
This means we're not breaking "network compatibility" yet.
The protocol will now guarantee that the response to a Query has the right
type.
@mrBliss mrBliss force-pushed the mrBliss/local-state-query branch from f0fa03a to 0ec858c Compare January 29, 2020 15:38
@mrBliss
Copy link
Contributor Author

mrBliss commented Jan 29, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request Jan 29, 2020
1507: Implement the  LocalStateQueryServer  r=mrBliss a=mrBliss

Closes #1366.

Co-authored-by: Thomas Winant <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jan 29, 2020

@iohk-bors iohk-bors bot merged commit 0ec858c into master Jan 29, 2020
@iohk-bors iohk-bors bot deleted the mrBliss/local-state-query branch January 29, 2020 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a server implementation of the LocalStateQuery protocol
2 participants