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

Rename transient store key to be a unique key. #2013

Merged
merged 2 commits into from
Aug 14, 2018

Conversation

ValarDragon
Copy link
Contributor

This caused an error with non-determinism between nodes with same
gaiad version and genesis. Fixes the issue (we think) for what we saw in internal testnets.


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

This caused an error with non-determinism between nodes with same
gaiad version and genesis.
@codecov
Copy link

codecov bot commented Aug 13, 2018

Codecov Report

Merging #2013 into release/v0.24.0 will not change coverage.
The diff coverage is 100%.

@@               Coverage Diff                @@
##           release/v0.24.0    #2013   +/-   ##
================================================
  Coverage            64.86%   64.86%           
================================================
  Files                  115      115           
  Lines                 6862     6862           
================================================
  Hits                  4451     4451           
  Misses                2127     2127           
  Partials               284      284

@cwgoes
Copy link
Contributor

cwgoes commented Aug 13, 2018

This looks fine, but can we add a testcase which fails before and passes now?

I think this is a result of rootmultistore.nameToKey

@ValarDragon
Copy link
Contributor Author

I think we should run make localnet-start on CI to catch this class of error. @jackzampolin is working on this.

I think we also need a conditional to panic on duplicate store key in the SDK. I can work on a testcase for this though.

@ValarDragon
Copy link
Contributor Author

Upon further thought, I don't think writing a testcase is a good utilization of time. We'd be relying on the functionality of registering duplicate keys being allowed, but if we want to remove that as a possibility it would likely be simpler to just write that fix.

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Overall, I agree @ValarDragon. In can't hurt to add both (remove functionality and add test). But for now, I think this will suffice.

PENDING.md Outdated
@@ -7,7 +7,8 @@ BREAKING CHANGES
* Gaia CLI (`gaiacli`)

* Gaia

* Make the transient store key use a distinct store key.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we mark/tag the PR (#2013) here?

@@ -78,7 +78,7 @@ func NewGaiaApp(logger log.Logger, db dbm.DB, traceStore io.Writer, baseAppOptio
keyGov: sdk.NewKVStoreKey("gov"),
keyFeeCollection: sdk.NewKVStoreKey("fee"),
keyParams: sdk.NewKVStoreKey("params"),
tkeyParams: sdk.NewTransientStoreKey("params"),
tkeyParams: sdk.NewTransientStoreKey("transient_params"),
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@ValarDragon ValarDragon dismissed alexanderbez’s stale review August 14, 2018 01:28

PR comment addressed!

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jackzampolin jackzampolin merged commit 1ae5466 into release/v0.24.0 Aug 14, 2018
@jackzampolin jackzampolin deleted the dev/fix_non_determinism_storekey branch August 14, 2018 02:09
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