[CDEC-509] Remove HasCoreConfiguration and HasConfiguration

[ruhatch]

Description

This is the removal of the final Data.Reflection constraint in core! This PR removes the dbSerializeVersion configuration parameter, a so far unused value. Could reviewers please check that a node running the code before this commit continues to sync after this commit is applied.

Linked issue

CDEC-509

Type of change

• [~] 🐞 Bug fix (non-breaking change which fixes an issue)
• [~] 🛠 New feature (non-breaking change which adds functionality)
• [~] ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
• 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
• [~] 🔨 New or improved tests for existing code
• [~] ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

• I have read the style guide document, and my code follows the code style of this project.
• If my code deals with exceptions, it follows the guidelines.
• I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
• [~] CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

• [~] I have added tests to cover my changes.
• All new and existing tests passed.

QA Steps

1. Connect to the testnet running a node on develop and sync some blocks
2. Kill the node and then with the same state connect with a node from this PR
3. Verify that the DB is still read and the syncing continues

Screenshots (if available)

[erikd]

Probably worth rebasing that against current develop (its 38 commits behiind) to reduce the chances of merge errors when it is merged to develop.

Never mind, looks like I was looking at an old version of the branch.

[erikd]

I don't trust the DB code and we don't have any tests to give us some confidence we're not breaking anything. I therefore think its prudent to add tests before making this change,

[erikd]

Hmm, want to check that a little more carefully on Monday.

Marking this "requires changes" until I properly examine the consequences of this.

[erikd]

The weird thing is that from looking at the code, I expect this to break when accessing existing DBs. The fact that it doesn't freaks me out even more than if it did break.

I know its a pain in the neck, but we can we separate out the dbSerializeVersion parts of this PR and add a test to show that its safe. The idea would be to:

• Raise a PR with some tests that use dbSerializeVersion as it is now.
• Once that PR is merged, raise a PR removing dbSerializeVersion.

[ruhatch]

You're right, that's safer and will reduce the complexity of this PR. I'm getting on it now.

[ruhatch]

This changes the DB to write without a version, but read using:

dbDecodeIgnoreVersion :: forall v . Bi v => ByteString -> Either DBError v
dbDecodeIgnoreVersion bytes = case decodeFull' @v bytes of
    Right val -> Right val
    Left _    -> bimap DBMalformed snd $ decodeFull' @(Word8, v) bytes

which first tries to decode without a version and, if that fails, tries again with the version and drops it

[ruhatch]

That's why it doesn't break on old DBs :)

[ruhatch]

The tests are in place and passing. Ready to re-review, paying particular attention to this file

Reviewing again

[erikd]

Definitely, not worth doing in this PR but later we should go through the code base and killing completely useless type aliases like type X m = Y m.

[erikd]

Something wend way wrong with the changes to lib/configuration.yam.

Github reports:

14,646 additions, 14,676 deletions not shown because the diff is too large. Please use a local Git client to view these changes.

I suspect the "semantic changes only" version of the diff for that file would be 30 lines. We really need that version :)

[erikd]

Semanitc changes only in lib/configuration.yaml please :)

Also wondering it it might not be worthwhile keeping the function dbSerializeValue and defining it as:

dbSerializeValue = serialize'

which would result in a much smaller PR and the inliner should ensure there is zero performance difference.

PR has been updated

[erikd]

I compiled this version and ran it on an existing node DB (already synced to the 69th epoch or mainnet) and it ran fine.

I then deleted the the DB to force a full sync and that also seems to be fine and is currently at the 10th epoch.

[erikd]

Love that the code base has 400 fewer lines after this is merged :).