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

Add version 6.5 #30

Merged
merged 2 commits into from
Jul 22, 2021
Merged

Add version 6.5 #30

merged 2 commits into from
Jul 22, 2021

Conversation

ZenGround0
Copy link
Contributor

This is motivated by the effort to fix lotus pricelist to rely on version numbers filecoin-project/lotus#6652

Turns out versions are already an enum so this was easy.

Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

noice

@Stebalien
Copy link
Member

Discussed in standup:

This should work, but it's exposed over the API so there are some concerns that users are using it. The main use is, as far as I know, the lotus miner.

The correct way to do this update is to make a breaking version change to the API, which is always a bit annoying.

@ZenGround0
Copy link
Contributor Author

@Stebalien how terrible would it be to change from int to float if we make a breaking api change? The benefit would be that version numbers would look like users would expect: 6.5, 13.0. The cost would be using floats.

@Stebalien
Copy link
Member

Awful? Checking equality between floats is generally a bad idea and not every (base-10) decimal can even be represented (accurately) as a float (which are effectively base-2 decimals).

If we're going to make a breaking change, I'd make these versions:

  • 100
  • 200
  • 300
    ...

So we can stick a 250 in there.

We'd have to... rework a few cases where we assume that versions are mostly sequential, but that's not really a problem.

@ZenGround0 ZenGround0 merged commit ad9bfe5 into master Jul 22, 2021
@ZenGround0 ZenGround0 deleted the feat/half-version-fix-pricelist branch July 22, 2021 13:30
@whyrusleeping
Copy link
Member

Hey friends, people were using this and it not just breaks, it causes panics

@whyrusleeping
Copy link
Member

The panic we encountered comes from here: https://github.com/filecoin-project/lotus/blob/master/chain/actors/version.go#L37

Any version mismatch between this code and where youre getting the version number from will likely hit this.

@Stebalien
Copy link
Member

Ok, I think we can sort of fix this by "aliasing" the old versions in that code. I.e., manually checking:

if version < 100 {
    version = version*100
}

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.

4 participants