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.

Sign up for GitHub

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

Jump to bottom

Use new GetChainBlockNo and GetChainPoint queries in query tip #3179

Merged
merged 2 commits into from
Sep 30, 2021
Merged

Use new GetChainBlockNo and GetChainPoint queries in query tip #3179

merged 2 commits into from
Sep 30, 2021

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Sep 8, 2021

No description provided.

@newhoggy newhoggy force-pushed the newhoggy/new-GetHeaderStateTip-query branch 2 times, most recently from 1f1531b to 0579b3d Compare September 11, 2021 07:36
@newhoggy
Copy link
Contributor Author

Output against updated node.

CARDANO_NODE_SOCKET_PATH=example/node-bft1/node.sock  $(./scripts/bin-path.sh cardano-cli) query tip --testnet-magic 42 --cardano-mode
{
    "epoch": 0,
    "hash": "e26ae91f83160b5bcda6a4496bdd8f0aefc392eed6e510d9452446183e0fa894",
    "slot": 42,
    "block": 1,
    "era": "Alonzo",
    "syncProgress": "100.00"
}

Fall back when using earlier alonzo era node:

CARDANO_NODE_SOCKET_PATH=example/node-bft1/node.sock  $(./scripts/bin-path.sh cardano-cli) query tip --testnet-magic 42 --cardano-mode
Warning: Local header state query unavailable.  Falling back to chain sync query
{
    "epoch": 22,
    "hash": "85d006f24c4458d403391b211757c2659afbd3bcdab90c52568da947a1a4e177",
    "slot": 33828,
    "block": 2365,
    "era": "Alonzo",
    "syncProgress": "100.00"
}

dcoutts
dcoutts previously requested changes Sep 11, 2021
View reviewed changes
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

See the suggestion below about reusing the existing API type for the tip.

cardano-api/src/Cardano/Api/Query.hs Outdated Show resolved Hide resolved
@dcoutts
Copy link
Contributor

dcoutts commented Sep 11, 2021

And presumably you'll want to rename the PR since you're using GetHeaderStateTip and not GetCurrentBlockNo.

@newhoggy newhoggy force-pushed the newhoggy/new-GetHeaderStateTip-query branch from 0579b3d to d2b1c98 Compare September 13, 2021 12:32
dnadales
dnadales reviewed Sep 14, 2021
View reviewed changes
cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs Outdated

mLocalState <- hushM (first ShelleyQueryCmdAcquireFailure eLocalState) $ \e ->
liftIO . T.hPutStrLn IO.stderr $ "Warning: Local state unavailable: " <> renderShelleyQueryCmdError e

chainTip <- case mLocalState >>= O.mHeaderStateTip of
Copy link
Member

Choose a reason for hiding this comment

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

If I'm not mistaken chainTip and the long code block needed to compute it is only needed to calculate tipSlotNo. I'd rather have this code block abstracted away to avoid obscuring the logic of runQueryTip.

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 chain tip is actually used later on as well.

@newhoggy newhoggy force-pushed the newhoggy/new-GetHeaderStateTip-query branch from 8d714ba to 33d8ffe Compare September 15, 2021 13:14
@newhoggy newhoggy mentioned this pull request Sep 15, 2021
Merged
@newhoggy newhoggy force-pushed the newhoggy/new-GetHeaderStateTip-query branch from 33d8ffe to e6a36f4 Compare September 24, 2021 12:45
@newhoggy newhoggy marked this pull request as ready for review September 24, 2021 13:02
@newhoggy newhoggy dismissed dcoutts’s stale review September 24, 2021 13:02

Comments addressed.

@newhoggy newhoggy changed the title Use new GetCurrentBlockNo query Use new GetChainBlockNo and GetChainPoint queries in query tip Sep 24, 2021
@newhoggy newhoggy force-pushed the newhoggy/new-GetHeaderStateTip-query branch from c15d310 to 60c051b Compare September 24, 2021 13:05
Jimbo4350
Jimbo4350 previously requested changes Sep 27, 2021
View reviewed changes
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

Nice. A few comments.

cardano-api/src/Cardano/Api/Query.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Query.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs
eLocalState <- liftIO $ executeLocalStateQueryExpr localNodeConnInfo Nothing $ \ntcVersion -> do
era <- queryExpr (QueryCurrentEra CardanoModeIsMultiEra)
eraHistory <- queryExpr (QueryEraHistory CardanoModeIsMultiEra)
mChainBlockNo <- if ntcVersion >= NodeToClientV_10
Copy link
Contributor

Choose a reason for hiding this comment

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

We really need to come up with a better way of handling which queries are available for a particular version. Is this information available in the consensus library?

Copy link
Contributor Author

@newhoggy newhoggy Sep 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

At the moment, it's documented in the code here: https://github.com/input-output-hk/ouroboros-network/blob/c25d36e62549d5bec9876cb57d1c52ee41bdec3a/ouroboros-network/src/Ouroboros/Network/NodeToClient/Version.hs#L45

cardano-cli/src/Cardano/CLI/Shelley/Output.hs Outdated Show resolved Hide resolved
@newhoggy newhoggy force-pushed the newhoggy/new-GetHeaderStateTip-query branch 3 times, most recently from b36ffdc to 8c25e4e Compare September 29, 2021 12:25
@newhoggy newhoggy force-pushed the newhoggy/new-GetHeaderStateTip-query branch from 8c25e4e to a4633d9 Compare September 29, 2021 12:36
Jimbo4350
Jimbo4350 reviewed Sep 29, 2021
View reviewed changes
cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs
Just chainTip -> return chainTip

-- The chain tip is unavailable via local state query because we are connecting with an older
-- node to client protocol so we use chain sync instead which necessitates another connection.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to encode this information in the data type. This is ok for now but (like I said above) we need to improve how we handle query availability in a given node version.

Jimbo4350
Jimbo4350 approved these changes Sep 29, 2021
View reviewed changes
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM! Can you clean up the commits?

newhoggy
newhoggy commented Sep 29, 2021
View reviewed changes
cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs
then Just <$> queryExpr QueryChainBlockNo
else return Nothing
mChainPoint <- if ntcVersion >= NodeToClientV_10
then Just <$> queryExpr (QueryChainPoint CardanoMode)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm curious if there is a way to not pass in CardanoMode the constructor argument like this.

newhoggy
newhoggy commented Sep 29, 2021
View reviewed changes
cardano-api/src/Cardano/Api/Query.hs
-> r'
_ -> fromConsensusQueryResultMismatch

fromConsensusQueryResult (QueryChainPoint mode) q' r' =
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 reason I need the mode is because below, fromConsensusPointInMode requires mode.

@newhoggy newhoggy force-pushed the newhoggy/new-GetHeaderStateTip-query branch from a4633d9 to e82842e Compare September 30, 2021 14:05
@newhoggy
Copy link
Contributor Author

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 30, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 2081b68 into master Sep 30, 2021
@iohk-bors iohk-bors bot deleted the newhoggy/new-GetHeaderStateTip-query branch September 30, 2021 14:46
@newhoggy newhoggy mentioned this pull request Nov 9, 2021
Merged
iohk-bors bot added a commit that referenced this pull request Nov 9, 2021
@iohk-bors @newhoggy
Merge #3355
ab9e9a3 
3355: Delete obsolete function executeLocalStateQueryExprWithChainSync r=newhoggy a=newhoggy

The function was necessary previously because it was not possible to query for chain tip from local state queries.  This changed with this PR: #3179 which provided `QueryChainBlockNo`
 and `QueryChainPoint`.

This means `executeLocalStateQueryExprWithChainSync` is no longer used nor needed.

Co-authored-by: John Ky <john.ky@iohk.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts

@dnadales dnadales Awaiting requested review from dnadales

@DavidEichmann DavidEichmann Awaiting requested review from DavidEichmann

@erikd erikd Awaiting requested review from erikd

@JaredCorduan JaredCorduan Awaiting requested review from JaredCorduan

@nc6 nc6 Awaiting requested review from nc6

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@newhoggy @dcoutts @dnadales @Jimbo4350