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

blockchain: Remove compression version param. #2547

Merged

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Jan 9, 2021

This requires #2540.

Over the years it has become increasingly obvious that storing multiple versioned formats in the database in an attempt to avoid migrations leads to code that is super hard to reason about and for which it is also difficult to assert correctness.

This is the case because it results in a combinatorial explosion of cases that must be handled. For example, as soon as you have 3 versions, you're already up to 8 variants you have to handle properly and test, and it only gets exponentially worse with each new version.

Due to this, it is greatly preferred to perform a single migration that handles the conversion logic once. This allows the rest of the code, especially in the critical paths, to work solely with the latest version and therefore it stays much cleaner, easier to validate for correctness, and is generally easier to reason about.

With that in mind, this removes the compression version parameter from the functions that deal with serializing and deserializing compressed scripts. Since there is only currently a single version it does not require any migrations.

However, since the existing migration code for older versions was passing in the old version parameter, new v1 functions have been added to the upgrade code to ensure stability there.

@davecgh davecgh added this to the 1.7.0 milestone Jan 9, 2021
@davecgh davecgh force-pushed the blockchain_remove_compression_version_param branch from b07bd11 to 745547d Compare January 9, 2021 19:16
@davecgh davecgh force-pushed the blockchain_remove_compression_version_param branch 2 times, most recently from cfb953c to 83fcede Compare January 10, 2021 22:24
Copy link
Member

@dnldd dnldd left a comment

Choose a reason for hiding this comment

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

Looks good but needs a rebase.

@davecgh davecgh force-pushed the blockchain_remove_compression_version_param branch from 83fcede to 9568d1a Compare January 11, 2021 17:12
Over the years it has become increasingly obvious that storing multiple
versioned formats in the database in an attempt to avoid migrations
leads to code that is super hard to reason about and for which it is
also difficult to assert correctness.

This is the case because it results in a combinatorial explosion of
cases that must be handled.  For example, as soon as you have 3
versions, you're already up to 8 variants you have to handle properly
and test, and it only gets exponentially worse with each new version.

Due to this, it is greatly preferred to perform a single migration that
handles the conversion logic once.  This allows the rest of the code,
especially in the critical paths, to work solely with the latest version
and therefore it stays much cleaner, easier to validate for correctness,
and is generally easier to reason about.

With that in mind, this removes the compression version parameter from
the functions that deal with serializing and deserializing compressed
scripts.  Since there is only currently a single version it does not
require any migrations.

However, since the existing migration code for older versions was
passing in the old version parameter, new v1 functions have been added
to the upgrade code to ensure stability there.
@davecgh davecgh force-pushed the blockchain_remove_compression_version_param branch from 9568d1a to fdb4b97 Compare January 15, 2021 04:22
@davecgh davecgh merged commit fdb4b97 into decred:master Jan 15, 2021
@davecgh davecgh deleted the blockchain_remove_compression_version_param branch January 15, 2021 04:25
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