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.

Sign up for GitHub

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

Jump to bottom

ABCI compatibility with 0.17.0 #249

Closed
liamsi opened this issue May 2, 2020 · 1 comment
Closed

ABCI compatibility with 0.17.0 #249

liamsi opened this issue May 2, 2020 · 1 comment

Comments

@liamsi
Copy link
Member

liamsi commented May 2, 2020 •
edited
Loading

Note that integration tests on master are failing again. Now with:

---- rpc::abci_info stdout ----
thread 'rpc::abci_info' panicked at 'assertion failed: `(left == right)`
  left: `"0.17.0"`,
 right: `"0.16.2"`', tendermint/tests/integration.rs:37:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

What is odd about this is that this fails although there was no new release yet (this is from pending):

[abci] #4704 Add ABCI methods ListSnapshots, LoadSnapshotChunk, OfferSnapshot, and ApplySnapshotChunk for state sync snapshots. ABCIVersion bumped to 0.17.0.

https://github.com/tendermint/tendermint/blob/master/CHANGELOG_PENDING.md#v0335

While it is cool to be warned early on about changes to come it would also be good if there was a mechanism to stick to a particular tendermint release (e.g. currently we are aiming for compatibility with 0.33).

related:
@liamsi liamsi mentioned this issue May 3, 2020
Closed
5 tasks
This was referenced May 7, 2020
Merged
Merged
@liamsi
Copy link
Member Author

liamsi commented May 7, 2020

Closing this for now. The current version bump was fixed by @greg-szabo in #256
The other concerns around this (how do we keep up to date with abci changes and do we want to be warned about changes before releases and should we maybe fix integration tests to a particular version), will bubble up again soon enough.

@liamsi liamsi closed this as completed May 7, 2020
@liamsi liamsi mentioned this issue May 27, 2020
Closed
shonfeder pushed a commit to shonfeder/tendermint-rs that referenced this issue Jun 15, 2020
Remove test for hardcoded ABCI version
b452fcc 
Proposes one solution to the underlying issue behind

- informalsystems#249
- informalsystems#238
- informalsystems#233

My sense is that, if we want to set a strict abci version requirement
for the rpc client, then we should put that in the source code itself.
E.g., we might put a check on the client the abci version is within a
version range which is known to be supported. If the version is outside
that range, we could either error out or log errors/warning to alert
users that we don't guarantee compatibility.

The current approach of hardcoding in a version in the integration test
tiself seems to create a lot of busy work due to uninformative test
failures, and I'm not sure if delivers value. If the integration tests
are meant to test that the RPC client integrates correctly with ACBI,
should we really consider that integration has failed if everything
works as expected while interfacing with an older (or newer) version?
shonfeder pushed a commit to shonfeder/tendermint-rs that referenced this issue Jun 15, 2020
Remove test for hardcoded ABCI version
adc3101 
Hardcoding a test for the version in this way makes it impossible to run the
integration tests against different ABCI versions.

This change proposes one solution to address this problem and to the
underlying issue behind, e.g.,

- informalsystems#249
- informalsystems#238
- informalsystems#233

My sense is that, if we want to set a strict abci version requirement
for the rpc client, then we should put that in the source code itself.

E.g., we might put a check on the client that ensures the abci version
is within a specified version range known to be supported. If the
version is outside that range, we could either error out or log
errors/warning to alert users that we don't guarantee compatibility.

However, the current approach of hardcoding in a version in the
integration test seems to create a lot of busy work due to uninformative
test failures and it's not obvious what value it delivers. If the
integration tests are meant to test that the RPC client integrates
correctly with ACBI, should we really consider integration to have
failed when everything works as expected while interfacing with an
older (or newer) version?

Signed-off-by: Shon Feder <shon@informal.systems>
shonfeder pushed a commit to shonfeder/tendermint-rs that referenced this issue Jun 15, 2020
Remove test for hardcoded ABCI version
3b04f31 
Hardcoding a test for the version in this way makes it impossible to run the
integration tests against different ABCI versions.

This change proposes one solution to address this problem and to the
underlying issue behind, e.g.,

- informalsystems#249
- informalsystems#238
- informalsystems#233

My sense is that, if we want to set a strict abci version requirement
for the rpc client, then we should put that in the source code itself.

E.g., we might put a check on the client that ensures the abci version
is within a specified version range known to be supported. If the
version is outside that range, we could either error out or log
errors/warning to alert users that we don't guarantee compatibility.

However, the current approach of hardcoding in a version in the
integration test seems to create a lot of busy work due to uninformative
test failures and it's not obvious what value it delivers. If the
integration tests are meant to test that the RPC client integrates
correctly with ACBI, should we really consider integration to have
failed when everything works as expected while interfacing with an
older (or newer) version?

Signed-off-by: Shon Feder <shon@informal.systems>
shonfeder pushed a commit to shonfeder/tendermint-rs that referenced this issue Jun 15, 2020
Remove test for hardcoded ABCI version
5585b41 
Hardcoding a test for the version in this way makes it impossible to run the
integration tests against different ABCI versions.

This change proposes one solution to address this problem and to the
underlying issue behind, e.g.,

- informalsystems#249
- informalsystems#238
- informalsystems#233

My sense is that, if we want to set a strict abci version requirement
for the rpc client, then we should put that in the source code itself.

E.g., we might put a check on the client that ensures the abci version
is within a specified version range known to be supported. If the
version is outside that range, we could either error out or log
errors/warning to alert users that we don't guarantee compatibility.

However, the current approach of hardcoding in a version in the
integration test seems to create a lot of busy work due to uninformative
test failures and it's not obvious what value it delivers. If the
integration tests are meant to test that the RPC client integrates
correctly with ACBI, should we really consider integration to have
failed when everything works as expected while interfacing with an
older (or newer) version?

Signed-off-by: Shon Feder <shon@informal.systems>
liamsi pushed a commit that referenced this issue Jun 15, 2020
Add integration test pinned to tendermint-go v0.33.5 (#324)
704e244 
* Add integration test pinned to tendermint-go v0.33

Closes #304

Also renames test-integration-ignored to test-integration-latest.

With this change, CI runs two integration tests:

1. A `stable` test to protect against regressions, run against a pinned
version of the tendermint/tendermint docker image.
2. A `latest` test to track whether we're maintaining parity with the
latest development version of tendermint-go.

Signed-off-by: Shon Feder <shon@informal.systems>

* Remove test for hardcoded ABCI version

Hardcoding a test for the version in this way makes it impossible to run the
integration tests against different ABCI versions.

This change proposes one solution to address this problem and to the
underlying issue behind, e.g.,

- #249
- #238
- #233

My sense is that, if we want to set a strict abci version requirement
for the rpc client, then we should put that in the source code itself.

E.g., we might put a check on the client that ensures the abci version
is within a specified version range known to be supported. If the
version is outside that range, we could either error out or log
errors/warning to alert users that we don't guarantee compatibility.

However, the current approach of hardcoding in a version in the
integration test seems to create a lot of busy work due to uninformative
test failures and it's not obvious what value it delivers. If the
integration tests are meant to test that the RPC client integrates
correctly with ACBI, should we really consider integration to have
failed when everything works as expected while interfacing with an
older (or newer) version?

Signed-off-by: Shon Feder <shon@informal.systems>

* Update changelog

Signed-off-by: Shon Feder <shon@informal.systems>
@greg-szabo greg-szabo mentioned this issue Aug 4, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@liamsi