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

fix(app): Stop panicking at startup when parsing the app version - release blocker #6888

Merged
merged 3 commits into from
Jun 9, 2023

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jun 9, 2023

Motivation

Zebra panics in some git/machine environments when running app_version() at startup.

When we changed the version to 1.0.0, this code would always have panicked, because it expected a pre-release version.

I also fixed a similar potential panic in the get_info RPC version code.

Close #6879

Specifications

https://semver.org/#semantic-versioning-specification-semver

For the RPC:
https://github.com/bitcoin/bips/blob/master/bip-0014.mediawiki#proposal

Complex Code or Requirements

The previous code had a bunch of undocumented assumptions. This code is longer but all the assumptions are spelled out, and the expect()s are justified.

Solution

  • Re-implement git version parsing, removing most of the panics
  • Remove a potential panic in the get_info() RPC

Review

This is an urgent release blocker.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

Follow Up Work

We could add tests for this parsing, but there isn't much point because the remaining panics should be impossible.

@teor2345 teor2345 added C-bug Category: This is a bug P-Critical 🚑 I-panic Zebra panics with an internal error message labels Jun 9, 2023
@teor2345 teor2345 requested a review from a team as a code owner June 9, 2023 01:13
@teor2345 teor2345 self-assigned this Jun 9, 2023
@teor2345 teor2345 requested review from upbqdn and removed request for a team June 9, 2023 01:13
@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #6888 (5fd4b09) into main (9959a6c) will decrease coverage by 0.16%.
The diff coverage is 32.20%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6888      +/-   ##
==========================================
- Coverage   77.72%   77.56%   -0.16%     
==========================================
  Files         310      310              
  Lines       41416    41475      +59     
==========================================
- Hits        32192    32172      -20     
- Misses       9224     9303      +79     

mergify bot added a commit that referenced this pull request Jun 9, 2023
@mergify mergify bot merged commit a18f47d into main Jun 9, 2023
@mergify mergify bot deleted the app-version-panic branch June 9, 2023 05:07
dconnolly pushed a commit that referenced this pull request Jun 12, 2023
…lease blocker (#6888)

* Fix a startup panic in app_version()

* Fix a potential RPC panic in get_info()

* Fix typo
@teor2345 teor2345 mentioned this pull request Jun 13, 2023
46 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug I-panic Zebra panics with an internal error message
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Release blocker: zebrad v1.0.0-rc.9 panics at start
2 participants