Sort validators by voting power #556
Conversation
* Domain types * DomainType derive macro * DomainType added to all amino_types
|
The failing JSON tests mostly come from the changes in #537 and they are unrelated to this change. (I'll double check.) #537 is big enough already that fixing all the changes there would make the change way too big to review. There is an open issue (#503 and #505) that deal with the integration tests and JSON tests fixes. The JSON test fixes will also require changes to the JSON data structures but adding that into #537 again would have bloated that PR. Either we can merge this into #537 and deal with JSON tests in there or deal with #537 first and then come back to this PR. |
|
I didn't see that this PR is against |
brapse
dismissed stale reviews from liamsi and melekes
The base branch was changed.
Co-authored-by: Romain Ruetschi <romain@informal.systems>
|
This PR is fine but I'm wary of approving it without fixing the tests first (as it now targets master). |
|
All right, I've started fixing the JSON tests in #563 but I must say, I believe it's a lot of wasted time. Most of our testing is generated by Shivani's test generator and updating them manually because of JSON incompatibilities is a task worthy of Sisyphos. All Tendermint tests are already fixed, but you don't see that because tests are mingled together for dependent crates: tendermint-rpc, tendermint-light-client and tendermint-light-node all fail because they need to be updated to Tendermint Core v0.34 too. Separating the fixes for downward dependencies seem to make sense to me. I'm not expected to wait with updating Tendermint just because IBC-rs broke but now I need to wait with continuing work on Tendermint because the light-client tests broke. In #563 I've separated out the tests in GitHub Actions so it shows you better what's broken and what's not. I believe if we want all our tests to succeed at all times, we better separate light-client and rpc into its own repo very soon. Each breaking update will have downward ripples creating a bunch of work that is unrelated to Tendermint Core but Tendermint Core development halts because of those breaking changes. I don't believe this will be sustainable in the long term. I've changed 41 JSON files so far, manually, because Tendermint Core decided that JSON commit.round should be encoded as an integer instead of a double-quoted-integer, and the same for parts_header.total. Because of the protobuf encoding, I'll have to recalculate hashes and after this PR goes in, I might need to recalculate hashes again. Shivani is working on publishing her testing generation but it's not released yet. I don't have the tools to automatically update the JSON files and I'm stuck with any other work because of that. I've done what I could today, but part of my deep flow week will have to go into this, if we can't come up with another solution. |
|
What are the alternatives to unblock you here?
If possible, I think 1. is the best route but I don't really know how much effort it really would be (Shivani could clarify this though). If 2. can be realized in a short time (putting an alternative in place), it could also be a viable option. |
|
I think in order to unblock progress we should go with (2) while working on (1). Also we may be moving away from the Go generators in favor of the new Rust generators based on the TLA+ models, but there may be tests here that aren't covered by existing models that we'd need to extend the new testgen stuff too. Maybe @andrey-kuprianov and @Shivani912 can look into this and see what it would take to replace this with the new testgen stuff. But in any case sorry to hear so much time was put into manually updating the files, probably best to deprecate for now so we can move forward but obviously get better testing done for the release |
|
Closing via #565 . |
@greg-szabo has fixed some tests for now, and disabled the others in #563. The disabled single_step ones can be quite easily replaced with the MBT-generated ones, as it seems to me, pending finishing the discussion and merge of #546. Fixing the disabled bisection ones will either require to extend MBT to bisection, or to adapt @Shivani912 's GO code. I would vote for the former. @greg-szabo I am also sorry to learn you've had to invest a lot of your time in fixing the test cases manually. I am sure as soon as we adopt the MBT/Testgen combination, such cases will no longer occur -- we will be able to adapt to any changes in the upstream Tendermint Core via easy changes to Testgen. |
I agree with @andrey-kuprianov, it's best we focus on adapting Testgen for the tests required. With the Go generator, it doesn't have a good architecture which failed it. Major learnings there. Sorry @greg-szabo, you had to go through all the pain of fixing it manually! |
Closes #506 . Needs #537 (opened it against that).