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

Restrict downgrade #6307

Merged
merged 23 commits into from
Feb 21, 2024
Merged

Conversation

matthew1001
Copy link
Contributor

@matthew1001 matthew1001 commented Dec 18, 2023

PR description

This PR aims to prevent accidental downgrade of Besu, which can potentially cause the DB to be irrevocably corrupted.

The approach I've used is as follows:

  • Add a new file VERSION_METADATA.json in the configured data path
    • Contains a single besuVersion field, e.g. {"besuVersion":"23.10.3"}
  • Add a new --version-compatibility-protection configuration option
  • Update the gradle prereqs to pull in org.apache.maven:maven-artifact to provide access to the maven ComparableVersion class
  • Add a function performVersionCompatibilityChecks() to VersionMetadata as the first function to call after configuration options have been validated.
    • performVersionCompatibilityChecks() throws an exception if the version in VERSION_METADATA.json is higher (when compared using the maven ComparableVersion class) than the version as recorded in the BesuCommand class implementation version

Any value after the first - character in the version number is ignored. This limits version comparison to the 3 calver digits, which is all that can be logically compared as higher or lower. An example of a version number that has trailing characters is 23.10.4-dev-c9338660 where the latest commit has been appended to the version. The -dev-c9338660 is ignored in the version comparison.

The validation logic is as follows:

  • If the VERSION_METADATA.json file doesn't exist, no further checks are made and the node starts. The VERSION_METADATA.json file is written to allow version checks from this point onwards.
  • If the file and besuVersion field are present and the version is lower than the installed/runtime version, Besu updates it to have the latest version in it and continues to start up
  • If the file and besuVersion field are present and the version is greater than the installed/runtime version, Besu fails to start if --version-compatibility-protection is set, because a lower version of Besu is being started than the version that most recently updated the file.

Fixed Issue(s)

Fixes #6266

Copy link

github-actions bot commented Dec 18, 2023

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

…ersion < version recorded in DB metadata.

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
…ates the Besu version in the metadata file to the downgraded version.

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
@matthew1001 matthew1001 added enhancement New feature or request doc-change-required Indicates an issue or PR that requires doc to be updated labels Dec 20, 2023
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
@non-fungible-nelson
Copy link
Contributor

LGTM functionality wise --> what you are proposing is a change to the existing default behavior (allowing downgrades). Should we consider the inverse (a flag to prevent downgrades or just switching to --prevent-downgrades)? It's really a coin-toss. I think users on Mainnet would be more likely to downgrade/upgrade more often and may benefit from the default remaining the same. I am open to either, however. This does not have to be a blocker.

@matthew1001
Copy link
Contributor Author

matthew1001 commented Dec 20, 2023

Thanks for the comments @non-fungible-nelson I'll give that some thought and reply properly tomorrow.

In the meantime I wanted to highlight that I've just updated the description and commits somewhat, as I originally had the checks happening in the RocksDB plugin but have decided that it's better that Besu can perform this check regardless of the storage provider being used. The main difference in the latest commits is the introduction of a new VERSION_METADATA.json file, rather than adding besuVersion as a field to the DATABASE_METADATA.json file.

@matthew1001 matthew1001 force-pushed the restrict-downgrade branch 5 times, most recently from d75e5b5 to 9dce227 Compare December 20, 2023 18:10
@matthew1001
Copy link
Contributor Author

Should we consider the inverse (a flag to prevent downgrades or just switching to --prevent-downgrades)? It's really a coin-toss. I think users on Mainnet would be more likely to downgrade/upgrade more often and may benefit from the default remaining the same.

Yeah I agree that it changes behaviour, but the main cost to a user who is expecting to be able to do this is to set --allow-downgrade and leave it turned on. The downside to someone who accidentally downgrades is potentially much worse, so I think I'd argue that it's a good reason for a behaviour/breaking change.

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
@matthew1001 matthew1001 marked this pull request as ready for review December 20, 2023 18:16
@matthew1001
Copy link
Contributor Author

Moving out of draft as I think this is ready for review

…ery big

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
@matthew1001
Copy link
Contributor Author

PR refactored to offer --version-compatibility-protection option which defaults to false, instead of --allow-downgrade that defaulted to true. See related discord discussion https://discord.com/channels/905194001349627914/1196772941619273779/1196773376556015666

@matthew1001 matthew1001 marked this pull request as ready for review January 24, 2024 13:35
@matthew1001
Copy link
Contributor Author

matthew1001 commented Jan 24, 2024

I've manually re-tested with the new --version-compatibility-protection:

  • Without it set, the VERSION_METADATA.json file is populated if it doesn't exist, but no version check is made
  • With it set, if the version hasn't changed from what is in VERSION_METADATA.json Besu starts as expected
  • With it set, if the version is older than the version in VERSION_METADATA.json Besu fails to start with
    • Besu version 24.1.2 is lower than version 24.1.3 that last started. Remove --version-compatibility-protection option to allow Besu to start at the lower version (warning - this may have unrecoverable effects on the database).

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
matthew1001 and others added 3 commits January 29, 2024 15:18
Signed-off-by: Matt Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of edits

matthew1001 and others added 6 commits February 21, 2024 11:13
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
…/VersionMetadata.java

Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Matt Whitehead <matthew1001@hotmail.com>
…/VersionMetadata.java

Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Matt Whitehead <matthew1001@hotmail.com>
Signed-off-by: Matt Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
@matthew1001 matthew1001 enabled auto-merge (squash) February 21, 2024 14:57
@fab-10 fab-10 added the doc-change-required Indicates an issue or PR that requires doc to be updated label Feb 21, 2024
@matthew1001 matthew1001 merged commit 59da092 into hyperledger:main Feb 21, 2024
87 checks passed
@alexandratran alexandratran removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Feb 27, 2024
suraneti pushed a commit to suraneti/besu that referenced this pull request Feb 28, 2024
* Add Besu version to DB metadata. Check for downgrades and reject if version < version recorded in DB metadata.

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Add --allow-downgrade CLI arg. If set it allows the downgrade and updates the Besu version in the metadata file to the downgraded version.

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Update gradle verification XML

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Add and update tests

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Refactoring

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Remove versioning from RocksDB, now in separate VERSION_DATADATA.json

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Tidy up and tests for the new class

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Move downgrade logic into VersionMetadata as BesuCommand is already very big

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Add more tests

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Refactor the naming of the option to version-compatibility-protection

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Remove remaining references to allow-downgrade

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Rename test

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Update comments

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Metadata verification update

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* gradle fix

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Enable version downgrade protection by default for non-named networks

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Fix default logic

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Update ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/VersionMetadata.java

Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Matt Whitehead <matthew1001@hotmail.com>

* Update ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/VersionMetadata.java

Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Matt Whitehead <matthew1001@hotmail.com>

* mock-maker-inline no longer needed

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

---------

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matt Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matt Whitehead <matthew1001@hotmail.com>
Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net>
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
* Add Besu version to DB metadata. Check for downgrades and reject if version < version recorded in DB metadata.

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Add --allow-downgrade CLI arg. If set it allows the downgrade and updates the Besu version in the metadata file to the downgraded version.

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Update gradle verification XML

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Add and update tests

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Refactoring

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Remove versioning from RocksDB, now in separate VERSION_DATADATA.json

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Tidy up and tests for the new class

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Move downgrade logic into VersionMetadata as BesuCommand is already very big

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Add more tests

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Refactor the naming of the option to version-compatibility-protection

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Remove remaining references to allow-downgrade

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Rename test

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Update comments

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Metadata verification update

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* gradle fix

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Enable version downgrade protection by default for non-named networks

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Fix default logic

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Update ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/VersionMetadata.java

Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Matt Whitehead <matthew1001@hotmail.com>

* Update ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/VersionMetadata.java

Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Matt Whitehead <matthew1001@hotmail.com>

* mock-maker-inline no longer needed

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

---------

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matt Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matt Whitehead <matthew1001@hotmail.com>
Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: amsmota <antonio.mota@citi.com>
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
* Add Besu version to DB metadata. Check for downgrades and reject if version < version recorded in DB metadata.

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Add --allow-downgrade CLI arg. If set it allows the downgrade and updates the Besu version in the metadata file to the downgraded version.

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Update gradle verification XML

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Add and update tests

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Refactoring

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Remove versioning from RocksDB, now in separate VERSION_DATADATA.json

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Tidy up and tests for the new class

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Move downgrade logic into VersionMetadata as BesuCommand is already very big

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Add more tests

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Refactor the naming of the option to version-compatibility-protection

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Remove remaining references to allow-downgrade

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Rename test

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Update comments

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Metadata verification update

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* gradle fix

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Enable version downgrade protection by default for non-named networks

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Fix default logic

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Update ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/VersionMetadata.java

Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Matt Whitehead <matthew1001@hotmail.com>

* Update ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/VersionMetadata.java

Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Matt Whitehead <matthew1001@hotmail.com>

* mock-maker-inline no longer needed

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

---------

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matt Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matt Whitehead <matthew1001@hotmail.com>
Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: amsmota <antonio.mota@citi.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accidental downgrade followed by re-upgrade leaves node broken
6 participants