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

Fix baseapp ante handler simulation mode bug #2847

Merged
merged 1 commit into from
Nov 20, 2018
Merged

Conversation

jaekwon
Copy link
Contributor

@jaekwon jaekwon commented Nov 17, 2018

Before, app.initializeContext was ignoring the ctx's MultiStore and setting a new one from the app, which was making things pretty confusing.

Also, before, context.multiStore() used to be unexposed, but there's no real need to because MultiStore doesn't expose the keys. So this is now exposed to make the PR here possible and simplify the baseapp logic a bit.

If context.MultiStore() were the CommitMultiStore, there is an least-authority security issue, because the CommitMultiStore exposes more than is safe for ante-handlers and handlers to work with, but with the current BaseApp implementation, this is not an issue as the context.MultiStore() passed in to ante-handler, handler, beginBlocker, and endBlocker are all CacheMultiStores.

Related issue: #2867
Related issue: #2824

@sunnya97
Copy link
Member

@jaekwon Why do we want it to use the checkTx context. It should be querying the latest committed block, not the local checkTx state.

@cwgoes
Copy link
Contributor

cwgoes commented Nov 19, 2018

Another cool thing would be to simulate transactions and then run queriers against the simulated state to see what changes occurred.

@codecov
Copy link

codecov bot commented Nov 20, 2018

Codecov Report

Merging #2847 into develop will decrease coverage by 0.08%.
The diff coverage is 89.47%.

@@             Coverage Diff             @@
##           develop    #2847      +/-   ##
===========================================
- Coverage    57.04%   56.96%   -0.09%     
===========================================
  Files          120      131      +11     
  Lines         8277     8582     +305     
===========================================
+ Hits          4722     4889     +167     
- Misses        3240     3363     +123     
- Partials       315      330      +15

@jaekwon jaekwon changed the title WIP: Make Queriers use checkState context but cache wrapped Make Queriers use checkState context but cache wrapped Nov 20, 2018
@jaekwon
Copy link
Contributor Author

jaekwon commented Nov 20, 2018

Merging for 9002.

@jaekwon jaekwon merged commit 47eed39 into develop Nov 20, 2018
@jaekwon jaekwon changed the title Make Queriers use checkState context but cache wrapped Fix baseapp ante handler simulation mode bug Nov 20, 2018
@cwgoes cwgoes deleted the jae/query_context branch November 20, 2018 10:01
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.

3 participants