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

Compare protocol versions according to SemVer 2.0.0 #3360

Merged
merged 4 commits into from
Oct 7, 2020

Conversation

tjanez
Copy link
Member

@tjanez tjanez commented Oct 1, 2020

Since we bumped the protocol versions to version 1.0.0 with the release of Oasis Core 20.10, we also need to modify how we compare and detect backward-incompatible changes to follow SemVer 2.0.0 rules.

From now onwards, only a change in a protocol's major version signifies a backward-incompatible change.

NOTE: The change in this PR would require a major bump in runtime committee protocol's version, but since it was already bumped in #3346, we don't need to bump it again.

@tjanez tjanez added the c:common Category: common libraries label Oct 1, 2020
@codecov
Copy link

codecov bot commented Oct 1, 2020

Codecov Report

Merging #3360 into master will decrease coverage by 0.12%.
The diff coverage is 90.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3360      +/-   ##
==========================================
- Coverage   66.09%   65.97%   -0.13%     
==========================================
  Files         370      370              
  Lines       33236    33236              
==========================================
- Hits        21967    21926      -41     
- Misses       8028     8081      +53     
+ Partials     3241     3229      -12     
Impacted Files Coverage Δ
go/runtime/host/protocol/connection.go 57.20% <0.00%> (-0.83%) ⬇️
go/common/version/version.go 80.00% <100.00%> (ø)
go/consensus/tendermint/abci/mux.go 58.23% <100.00%> (-3.38%) ⬇️
go/consensus/tendermint/api/genesis.go 73.49% <100.00%> (ø)
go/consensus/tendermint/full/statesync.go 67.92% <100.00%> (ø)
go/consensus/tendermint/seed/seed.go 68.66% <100.00%> (ø)
go/consensus/tendermint/tests/genesis/genesis.go 96.92% <100.00%> (ø)
go/worker/common/p2p/p2p.go 70.17% <100.00%> (ø)
go/runtime/scheduling/simple/simple.go 78.57% <0.00%> (-11.91%) ⬇️
go/staking/api/grpc.go 49.57% <0.00%> (-8.41%) ⬇️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24c3a61...fb701bd. Read the comment docs.

@tjanez tjanez force-pushed the tjanez/change-protocol-version-compare branch from c3871a6 to 4bdfc4b Compare October 2, 2020 11:12
@tjanez tjanez marked this pull request as ready for review October 2, 2020 11:21
@@ -59,7 +59,7 @@ func (sp *stateProvider) State(ctx context.Context, height uint64) (tmstate.Stat
InitialHeight: sp.genesisDocument.InitialHeight,
}
// XXX: This will fail in case an upgrade happened in-between.
state.Version.Consensus.App = version.ConsensusProtocol.ToU64()
state.Version.Consensus.App = version.TendermintAppVersion
Copy link
Member

Choose a reason for hiding this comment

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

While reviewing this line I discovered that the state sync test was broken (see #3369). Thanks @tjanez and Codecov inline warnings! :-)

@kostko
Copy link
Member

kostko commented Oct 5, 2020

Rebase and don't forget updates due to #3361 (comment).

tjanez added 4 commits October 7, 2020 11:31
Include it in Developer Documentation's index and GitBook's sidebar.
This should be useful for comparint protocol versions for
backward-incompatible changes.
Since we bumped the protocol versions to version 1.0.0 with the release
of Oasis Core 20.10, we also need to modify how we compare and detect
backward-incompatible changes to follow SemVer 2.0.0 rules.

From now onwards, only a change in a protocol's major version signifies
a backward-incompatible change.

go/common/version: Introduce TendermintAppVersion variable which is the
Tendermint ABCI application's version and should be compatible with
Tendermint's version checks.
It is no longer used.

runtime/src/common/version.rs: Remove corresponding Version's
major_minor() method.
@tjanez tjanez force-pushed the tjanez/change-protocol-version-compare branch from 4bdfc4b to fb701bd Compare October 7, 2020 09:32
@tjanez
Copy link
Member Author

tjanez commented Oct 7, 2020

Rebase and don't forget updates due to #3361 (comment).

Done.

@tjanez tjanez merged commit f580ad1 into master Oct 7, 2020
@tjanez tjanez deleted the tjanez/change-protocol-version-compare branch October 7, 2020 10:09
tjanez added a commit that referenced this pull request Nov 3, 2020
Since #3360 has been
merged, we stated that from now onwards, only a change in a protocol's
major version signifies a backward-incompatible change.

The change in #3458 is
a breaking change, hence we should bump the consensus protocol's major
version.
tjanez added a commit that referenced this pull request Nov 3, 2020
Since #3360 has been
merged, we stated that from now onwards, only a change in a protocol's
major version signifies a backward-incompatible change.

The change in #3458 is
a breaking change, hence we should bump the consensus protocol's major
version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:common Category: common libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants