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

Improvements to slotting conversions / TimeInterpreter #1943

Merged
merged 13 commits into from
Jul 28, 2020

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented Jul 23, 2020

Issue Number

#1869 / #1960

Overview

Comments

  • I thought queries in Shelley could fail. This is wrong. Once in Shelley (for now the final era), they have unlimited forecast range. However, if the node is in Byron, we need to be careful.

@Anviking Anviking self-assigned this Jul 23, 2020
@Anviking Anviking linked an issue Jul 23, 2020 that may be closed by this pull request
@KtorZ KtorZ force-pushed the rvl/1870/bump-deps branch 3 times, most recently from c8b3323 to 8f756c0 Compare July 24, 2020 07:06
@Anviking Anviking force-pushed the anviking/1869/improve-time-interpreter branch from 9392ea0 to 1d99265 Compare July 24, 2020 09:55
@KtorZ KtorZ force-pushed the rvl/1870/bump-deps branch 3 times, most recently from f580554 to 3ade904 Compare July 24, 2020 11:04
@Anviking Anviking force-pushed the anviking/1869/improve-time-interpreter branch from f928fd0 to a2d93ed Compare July 24, 2020 11:27
@KtorZ KtorZ force-pushed the rvl/1870/bump-deps branch 2 times, most recently from 8539d1a to a05514b Compare July 24, 2020 20:41
@Anviking Anviking force-pushed the anviking/1869/improve-time-interpreter branch from a2d93ed to 563dc47 Compare July 24, 2020 22:40
@KtorZ KtorZ force-pushed the rvl/1870/bump-deps branch from a05514b to 11f35b4 Compare July 25, 2020 10:11
Base automatically changed from rvl/1870/bump-deps to master July 25, 2020 17:08
@Anviking Anviking force-pushed the anviking/1869/improve-time-interpreter branch from 563dc47 to 376376a Compare July 25, 2020 20:28
@Anviking Anviking force-pushed the anviking/1869/improve-time-interpreter branch 2 times, most recently from cea9eb0 to a1cc8c6 Compare July 27, 2020 14:52
@Anviking Anviking changed the title WIP: Improvements to slotting conversions / TimeInterpreter Improvements to slotting conversions / TimeInterpreter Jul 27, 2020
@paweljakubas paweljakubas mentioned this pull request Jul 27, 2020
7 tasks
@Anviking Anviking marked this pull request as ready for review July 27, 2020 16:00
@Anviking Anviking force-pushed the anviking/1869/improve-time-interpreter branch from ae685c2 to 1e232f8 Compare July 27, 2020 16:30
Left e -> do
traceWith tr $ MsgInterpreterPastHorizon (pretty query) e
throwIO e
liftIO $ traceWith tr $ MsgInterpreterPastHorizon (pretty query) e
Copy link
Member Author

@Anviking Anviking Jul 27, 2020

Choose a reason for hiding this comment

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

MsgInterpreterPastHorizon shouldn't be critical anymore, as it might be expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

I lowered to Info and removed the log line about it being unexpected.

@Anviking
Copy link
Member Author

bors try

iohk-bors bot added a commit that referenced this pull request Jul 27, 2020
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 28, 2020

try

Build succeeded

Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

There is so much unsafeRunExceptT here. It makes our code look unsafe.
This is replacing an actual exception type PastHorizonException with userError and a string message, which is worse.

Why not let all the time/slot conversion functions throw PastHorizonException, wherever unsafeRunExceptT is currently used. In the other "safe" cases, catch the exception.

Alternatively, replace unsafeRunExceptT with a new util runNonFailingQuery (or some name like that). This will be basically runNonFailingQuery q = q >>= either throwIO pure. The advantage of this is that it's obvious where the exception could occur, and userError is not used.

And also we should sprinkle HasCallStack liberally - otherwise tracking down errors will be hard.

@@ -1024,4 +1037,4 @@ instance HasSeverityAnnotation (NetworkLayerLog b) where
MsgWatcherUpdate{} -> Debug
MsgChainSyncCmd cmd -> getSeverityAnnotation cmd
MsgInterpreter{} -> Debug
MsgInterpreterPastHorizon{} -> Critical
MsgInterpreterPastHorizon{} -> Info
Copy link
Member

Choose a reason for hiding this comment

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

Is this change really sound 🤔 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the reasoning is that we have actually some cases where the PastHorizonException is "normal" and somewhat expected.

But perhaps we can divide slot/time conversions into two categories:

  1. Where it's our bug if the conversion fails.
  2. Where any conversion errors are because of the user inputting a date which is beyond the safe zone. So an error not a bug.

Anviking added 5 commits July 28, 2020 12:47
On launch, the TimeInterpreter may not have been fetched from the node.

Instead returning the singleEraInterpreter for the first era, it seems
safer to block until fetched.

I imagine there could be race conditions where we would sometimes return
completely wrong time data when in Shelley, just after starting the
node. Not completely sure, but I hope there shouldn't be any drawbacks
with blocking.
@KtorZ KtorZ force-pushed the anviking/1869/improve-time-interpreter branch from 15ec465 to 351a1b1 Compare July 28, 2020 10:55
@KtorZ
Copy link
Member

KtorZ commented Jul 28, 2020

bors try

iohk-bors bot added a commit that referenced this pull request Jul 28, 2020
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 28, 2020

try

Build failed

@Anviking Anviking force-pushed the anviking/1869/improve-time-interpreter branch from bfeefe5 to 78561c1 Compare July 28, 2020 11:36
@Anviking
Copy link
Member Author

bors try

iohk-bors bot added a commit that referenced this pull request Jul 28, 2020
Anviking and others added 2 commits July 28, 2020 13:41
  Instead, we do not push the exception down to every caller but rather, throw it as an exception in the network layer. The rationale is that, this exception can only occur when both conditions are met:

  a) The node is still syncing and doesn't yet know about any hard-fork.
  b) A time beyond the node's foreseeable future is queried.

  While syncing in Byron these two conditions can't be met (times referenced in blocks are neccessarily before the node's tip and can't be beyond its foreseeable future. There's the case of delegation certificates and or transaction TTL but these only exists in Shelley, where the foreseeable future is so far, unlimited.
  Yet, there are points in the API where a time that is far beyond the node's tip can be provided and that is:

  - As filtering parameter when listing transactions.
  - As current time when looking at network parameters.
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 28, 2020

try

Build succeeded

@KtorZ KtorZ force-pushed the anviking/1869/improve-time-interpreter branch from 78561c1 to c46082b Compare July 28, 2020 12:38
@KtorZ KtorZ merged commit aab4ddf into master Jul 28, 2020
@KtorZ KtorZ deleted the anviking/1869/improve-time-interpreter branch July 28, 2020 12:39
@KtorZ
Copy link
Member

KtorZ commented Jul 28, 2020

Waited for the "trying" branch to pass. Then pushed a little update that reshaping the git history and, lowering the "PastHorizon" error from Critical to Error. Here was the diff:

$ git diff HEAD origin/anviking/1869/improve-time-interpreter 
diff --git a/lib/shelley/src/Cardano/Wallet/Shelley/Network.hs b/lib/shelley/src/Cardano/Wallet/Shelley/Network.hs
index 833ff04f2..971cfbfc4 100644
--- a/lib/shelley/src/Cardano/Wallet/Shelley/Network.hs
+++ b/lib/shelley/src/Cardano/Wallet/Shelley/Network.hs
@@ -1051,4 +1051,4 @@ instance HasSeverityAnnotation (NetworkLayerLog b) where
         MsgWatcherUpdate{}                 -> Debug
         MsgChainSyncCmd cmd                -> getSeverityAnnotation cmd
         MsgInterpreter{}                   -> Debug
-        MsgInterpreterPastHorizon{}        -> Error
+        MsgInterpreterPastHorizon{}        -> Critical

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RESOLVING ISSUE Mark a PR as resolving issues, for auto-generated CHANGELOG
Projects
None yet
3 participants