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

go: Update various dependencies #1249

Merged
merged 1 commit into from Dec 27, 2018
Merged

Conversation

Yawning
Copy link
Contributor

@Yawning Yawning commented Dec 26, 2018

  • Bump tendermint to 0.27.4

    • Tendermint consensus timeouts now use time.Duration.
    • The db ReverseIterator semantics have changed.
  • Bump iavl to 0.12.0-ekiden2

    • LoadVersionFast no longer exists.
  • Run go mod tidy to clean up go.sum.

This will fix #1158 and #1248.

@Yawning Yawning added p:1 Priority: core feature golang c:consensus/tendermint Category: Tendermint-based consensus c:breaking/consensus Category: breaking consensus changes labels Dec 26, 2018
@Yawning
Copy link
Contributor Author

Yawning commented Dec 26, 2018

The tests break horribly with consensus failures. I can't tell if it's because I screwed up when rebasing the iavl code or not because going back to the previous version of the fork just fails differently.

I don't think it's the ReverseIterator changes because the failure is identical even when ran with the tendermint's default db provider, so my current suspicion is the iavl code.

@Yawning Yawning force-pushed the yawning/feature/dependency-bumps branch from 6ad990e to 47eb1a0 Compare December 26, 2018 12:22
@Yawning Yawning changed the title WIP: go: Update various dependencies WIP - DO NOT MERGE: go: Update various dependencies Dec 26, 2018
@Yawning
Copy link
Contributor Author

Yawning commented Dec 26, 2018

So yeah, I broke something when I rebased/updated the iavl code because a clean copy of 0.12.0 with only EldestVersion passes tests.

@Yawning
Copy link
Contributor Author

Yawning commented Dec 26, 2018

Ok, this is an upstream iavl bug.

diff --git a/nodedb.go b/nodedb.go
index 2e00c19..3170edc 100644
--- a/nodedb.go
+++ b/nodedb.go
@@ -232,9 +232,9 @@ func (ndb *nodeDB) rootKey(version int64) []byte {
 }
 
 func (ndb *nodeDB) getLatestVersion() int64 {
-       if ndb.latestVersion == 0 {
+//     if ndb.latestVersion == 0 {
                ndb.latestVersion = ndb.getPreviousVersion(1<<63 - 1)
-       }
+//     }
        return ndb.latestVersion
 }

@Yawning
Copy link
Contributor Author

Yawning commented Dec 26, 2018

Ok, I see what the issue is. In the mux commit routine we do:

	blockHash, blockHeight, err := s.deliverTxTree.SaveVersion() 

		_, cerr := s.checkTxTree.LoadVersion(blockHeight)

The problem occurs because deliverTxTree and checkTxTree are two separate iavl tree instances each backed with their own iavl.nodeDB. checkTxTree.ndb.latestVersion is stale, but the old LoadVersionFast code did not use that value at all unless the latest version was explicitly requested (LoadVersion(0)).

The new LoadVersion code enforces that the version loaded is less than ndb.latestVersion (to match the slow LoadVersion semantics).

The "easy" fix (that would work for us) would be to have LoadVersion always update ndb.latestVersion from the backing database, but I don't see upstream taking that, since I don't think that instantiating two separate tree instances with the same backing store is a use case they want to support[0].

[0]: For reasons that should be painfully obvious. It's only "safe" to do since we NEVER call s.checkTxTree.SaveVersion().

@Yawning Yawning changed the title WIP - DO NOT MERGE: go: Update various dependencies go: Update various dependencies Dec 27, 2018
@Yawning
Copy link
Contributor Author

Yawning commented Dec 27, 2018

This needs the iavl change and one further edit to go.mod to point at a new tag, but it should be ready for review.

@Yawning Yawning requested a review from kostko December 27, 2018 06:37
@Yawning Yawning force-pushed the yawning/feature/dependency-bumps branch from e96e067 to 1910105 Compare December 27, 2018 06:52
 * Bump tendermint to 0.27.4
   * Tendermint consensus timeouts now use time.Duration.
   * The db ReverseIterator semantics have changed.

 * Bump iavl to 0.12.0-ekiden2
   * `LoadVersionFast` no longer exists.

 * Run `go mod tidy` to clean up `go.sum`.
@Yawning Yawning force-pushed the yawning/feature/dependency-bumps branch from 1910105 to 4e5abca Compare December 27, 2018 06:55
@codecov
Copy link

codecov bot commented Dec 27, 2018

Codecov Report

Merging #1249 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1249      +/-   ##
==========================================
+ Coverage   52.86%   52.94%   +0.08%     
==========================================
  Files         136      136              
  Lines       10645    10643       -2     
==========================================
+ Hits         5627     5635       +8     
+ Misses       4470     4465       -5     
+ Partials      548      543       -5
Impacted Files Coverage Δ
go/tendermint/db/bolt/bolt.go 72.61% <100%> (+0.68%) ⬆️
go/tendermint/tendermint.go 70.27% <100%> (-0.14%) ⬇️
go/tendermint/abci/mux.go 58.97% <100%> (ø) ⬆️
go/registry/tendermint/tendermint.go 77.19% <0%> (+0.7%) ⬆️
go/roothash/tendermint/tendermint.go 48.14% <0%> (+1.38%) ⬆️
go/epochtime/tendermint_mock/tendermint_mock.go 70% <0%> (+3.33%) ⬆️

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 b51fe6d...4e5abca. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 27, 2018

Codecov Report

Merging #1249 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1249      +/-   ##
==========================================
+ Coverage   52.86%   52.93%   +0.07%     
==========================================
  Files         136      136              
  Lines       10645    10643       -2     
==========================================
+ Hits         5627     5634       +7     
+ Misses       4470     4466       -4     
+ Partials      548      543       -5
Impacted Files Coverage Δ
go/tendermint/db/bolt/bolt.go 72.61% <100%> (+0.68%) ⬆️
go/tendermint/tendermint.go 70.72% <100%> (+0.31%) ⬆️
go/tendermint/abci/mux.go 58.97% <100%> (ø) ⬆️
go/scheduler/tests/tester.go 91.42% <0%> (-2.86%) ⬇️
go/registry/tendermint/tendermint.go 77.19% <0%> (+0.7%) ⬆️
go/roothash/tendermint/tendermint.go 48.14% <0%> (+1.38%) ⬆️
go/epochtime/tendermint_mock/tendermint_mock.go 70% <0%> (+3.33%) ⬆️

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 b51fe6d...4e5abca. Read the comment docs.

@Yawning Yawning merged commit 721fc4c into master Dec 27, 2018
@Yawning Yawning deleted the yawning/feature/dependency-bumps branch December 27, 2018 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:breaking/consensus Category: breaking consensus changes c:consensus/tendermint Category: Tendermint-based consensus golang p:1 Priority: core feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump tendermint to 0.27
2 participants