-
Notifications
You must be signed in to change notification settings - Fork 217
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
Listing transaction when node is still in the Byron era may fail with 500 #1971
Comments
That is the case indeed on mainnet currently:
|
As "expected", though the server should really catch this error and return something more meaningful to users. |
@piotr-iohk I'm not sure how to reproduce this properly (I don't have a byron wallet on mainnet). |
@KtorZ the error is catched and rethrown: So:
|
Forgot to write down a step: The node (and wallet) needs to be syncing in the byron part of the chain. Once it reaches the hard-fork to Shelley, no time interpreter queries will fail anymore. Description updated. |
Btw, when the wallet and node is still in Byron, |
@hasufell, possible to reproduce also on current testnet:
|
2059: Catch PastHorizon Exception and return 400 with descriptive Error r=KtorZ a=hasufell # Issue Number #1971 # Overview - catch the exception and return 400 with descriptive Error # Comments - should this be unit tested? If so, how? Co-authored-by: Julian Ospald <[email protected]> Co-authored-by: Julian Ospald <[email protected]>
2059: Catch PastHorizon Exception and return 400 with descriptive Error r=hasufell a=hasufell # Issue Number #1971 # Overview - catch the exception and return 400 with descriptive Error # Comments - should this be unit tested? If so, how? Co-authored-by: Julian Ospald <[email protected]> Co-authored-by: Julian Ospald <[email protected]>
LGTM.
@hasufell @KtorZ I don't suppose we can have automated test for this? (current integration set-up wouldn't allow for such a test or would be flaky I guess as we're testing past hard-fork). Is there perhaps a way to have a test "lower" in the code? If not then maybe I'll create a manual scenario... |
@piotr-iohk it is indeed a bit tricky to run an integration on this; we do have a callback triggered when the cluster is up and in the byron era, before triggering the hardfork, but the wallet server isn't started yet at this stage. We could, start it and run a subset of the specs, and then proceed as normal. That's a bit of busywork for just testing an error message though, yet; that is an error which can only happen / makes sense testing in a real context and that can only be tested at an integration level. |
I see now that there is a similar situation like in the original bug also with listing pools.
In the log there is:
@hasufell shall I create a separate bug or perhaps it is easy enough to address under this issue? |
@piotr-iohk let me check |
So the issue is in: This API is hard to change, but it'll likely leak IO Exceptions in other places too. |
2094: Return descriptive 503 when listing stake pools with uncomplete sync r=hasufell a=hasufell Wrt #1971 Co-authored-by: Julian Ospald <[email protected]>
2094: Return descriptive 503 when listing stake pools with uncomplete sync r=KtorZ a=hasufell Wrt #1971 Co-authored-by: Julian Ospald <[email protected]>
2094: Return descriptive 503 when listing stake pools with uncomplete sync r=hasufell a=hasufell Wrt #1971 Co-authored-by: Julian Ospald <[email protected]>
LGTM. Both listing tx and listing sp while node is in the Byron era result in a nice
|
Context
With #1869, we use a
TimeInterpreter
from the node for time and slotting conversions. The conversions fails if "Past the Horizon". From genesis to close to the fork, the node will have a limited horizon.Steps to Reproduce
Expected behavior
Actual behavior
500: Something went wrong
Note: This shouldn't happen when the node is in Shelley.
Resolution
QA
The text was updated successfully, but these errors were encountered: