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

Bump dependencies #495

Merged
merged 3 commits into from
Jan 21, 2021
Merged

Bump dependencies #495

merged 3 commits into from
Jan 21, 2021

Conversation

kderme
Copy link
Contributor

@kderme kderme commented Jan 20, 2021

Main changes:

  • localStateQuery improvements.
  • ledger-specs using type families (Core.TxOut instead of Shelley.TxOut)

@kderme kderme force-pushed the kderme/bump branch 2 times, most recently from 5193a25 to b5f9a76 Compare January 20, 2021 10:36
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.

LGTM!

versionErrorMsg = Text.concat
[ Text.pack $ show version
, " is not enough. We need at least "
, Text.pack $ show minVersion ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Closing ] on the next line under the comma to match the rest of the code.

@@ -15,10 +15,10 @@ import Cardano.Chain.Common (CompactAddress, Lovelace, decodeAddressBa
toCompactAddress, unsafeGetLovelace)
import qualified Cardano.Chain.UTxO as Byron

import qualified Cardano.Ledger.Core as Core
Copy link
Contributor

Choose a reason for hiding this comment

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

The qualified imports in this project are far from optimal and I have been meaning to fix them.

As a start in the right direction, can we import that as Ledger ?

:: forall era. ShelleyBased era
=> Text -> Shelley.UTxO era -> Either Text Word64
getShelleyBalance :: forall era.
( Core.TxOut era ~ Shelley.TxOut era -- somewhere in ledger-spec, there is probably a better constraint synonym for these
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try to match the formatting?

I wish I had a tool that could enforce formatting in a way that is not horrible, I hate brittany and ormul??? or whatever its called.

Comment on lines +269 to +276
versionErrorMsg :: Text
versionErrorMsg = Text.concat
[ "The cardano-node version is too old. Please upgrade to a compatible "
, "cardano-node version. The db-sync requires a node that supports "
, textShow minVersion
, ", the one in use only supports "
, textShow version
]
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

kderme and others added 2 commits January 21, 2021 15:59
The dependency bump also includes update to the way the history
interpreter is acquired and that in turn requires NodeToClientV_8.
A nice error message has been added if this version requirement
is not met.
This logic remains from the earliest versions of db-sync, and even
pre-dates the OBFT version of Byron. It is no longer relevant for
Shelley and later.

From memory this code was added to handle rollbacks across epoch
boundaries (pre-OBFT we had Epoch Boundary Blocks) but since there
are no new EBBs (the old ones are still part of the chain), we can
no longer have a rollback across an epoch boundary with an EBB
involved. This code was also a involved in a mis-sync:

    #496

Not sure how the above happened and not able to reproduce it.
However, since this is pre-OBFT and Byron specific this code can
simply be removed. The worst that can happen is that there is still
a undiscovered bug, but at least the problem should be more
obvious with this code removed.

Closes: #496
Doing this means that when syncing the "db_tip_height" metric will
increment by this new value instead of incrementing by 2000 as
it was previously. The new value makes the metric easier to eyeball
for changes.
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