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

BaseApp.Info should return a value for AppVersion #7487

Closed
4 tasks
ebuchman opened this issue Oct 8, 2020 · 8 comments · Fixed by #8897
Closed
4 tasks

BaseApp.Info should return a value for AppVersion #7487

ebuchman opened this issue Oct 8, 2020 · 8 comments · Fixed by #8897
Assignees
Labels
Milestone

Comments

@ebuchman
Copy link
Member

ebuchman commented Oct 8, 2020

Summary of Bug

ResponseInfo does not contain values for the app version fields.

There is an expectation from Tendermint that the ResponseInfo.Version contains the software version, and that ResponseInfo.AppVersion contains a "protocol version". The software version should be a semver string, the protocol version should be an integer that increments with every breaking change.

Currently, the SDK returns neither, so they are both left blank.

These are really just nice to haves and informative, but the AppVersion is included in block headers. Since these are currently omitted, the AppVersion defaults to 0 and is actually omitted in the JSON representation of blocks.

Version

I think all, but certainly master.

Steps to Reproduce

Run an SDK node. curl localhost:26657/status. See app: 0 field. In this endpoint it shows as zero but if you look at a block, the app field will be missing altogether from version.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@robert-zaremba
Copy link
Collaborator

This must be linked to the binary version. So how about using a binary versioning and use x/updateto keep track of the previously running version?

@alessio alessio modified the milestones: v0.41.X, v0.42 Feb 26, 2021
@amaury1093
Copy link
Contributor

Had a quick chat with @technicallyty, so as per #7487 (comment), we'll have a new key/value pair in x/upgrade's store to track the previously running version, and add a public UpgradeKeeper.SetAppVersion(v uint64) method on the upgrade keeper.

On each upgrade, x/upgrade will automatically increment the version inside the store. If the app developer wants to override this value (for whatever reason), they can always just call app.UpgradeKeeper.SetAppVersion(...) inside their NewSimApp function.

Does that sound like a good direction?

@aaronc
Copy link
Member

aaronc commented Mar 2, 2021

Yes it does @AmauryM

@robert-zaremba
Copy link
Collaborator

If this values are not used, how about renaming them? They may sound confusing, eg: AppVersion sounds more like the software version. So how about:

  • Version -> build version (software version)
  • ProtoVersion -> the protocol version which only tracks breaking changes as per x/update

@robert-zaremba
Copy link
Collaborator

Re the x/update version update mechanism. Why exposing SetAppVersion - this could create some inconsistencies. If needed, we can always add it later. IMHO, if not needed now - it's better to not do it.

For the version bump, how about this (not sure if this is how you think about it @AmauryM ):

  • x/upgrade has it's private mechanism to increment the version
  • whenever the x/upgrade is invoked and there is a new entry in the modules upgrade, we will increment the ProtoVersion. The module should assure that maximum one increment is done per one block. So 2 entries (for 2 modules) in x/upgrade will only bump the ProtoVersion by 1.

@aaronc
Copy link
Member

aaronc commented Mar 3, 2021

Where does ProtoVersion come in? IMHO protobuf versions shouldn't be incremented like that and should just be manager with v1, v2 suffixes.

I more or less agree about keeping the upgrade keeper version private though - although it should be possible to set a different initial version for migrating from chain restarts to x/upgrade.

@robert-zaremba
Copy link
Collaborator

@aaronc With ProtoVersion I was thinking about protocol version as noted in the OP:

ResponseInfo.AppVersion contains a "protocol version"

So a rename from AppVersion. We can call it ProtocolVersion if this creates confusions with protobuf.

@aaronc
Copy link
Member

aaronc commented Mar 3, 2021

The field name comes from tendermint so out of scope here FYI

@aaronc aaronc removed the in-review label Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants