Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: VoteExtension Slinky logic #1139
feat: VoteExtension Slinky logic #1139
Changes from all commits
6c462b4
70c5aed
ae7fe4b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we updating market prices in
ExtendVote
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because VEs are generated before deliver tx runs.
When we tested the protocol changes initially we ran into the following scenario:
N-1
runs and suggests a price change to priceNew
.N
and the price change is accepted into the block.N
and suggests to change the price toNew
(orNew + Delta
whereDelta
is smaller than the required min price change) since DeliverTx hasn't actually changed the x/prices state.N+1
runs and PrepareProposal creates anUpdateMarketPrices
Tx which is rejected in ProcessProposal because of themin_price_change
constraint being violated.The chain in this case would perpetually propose an UpdateMarketPrices tx which would be rejected and it would never make progress.
The solution to this was to execute the changes to the PricesKeeper in
ExtendVote
before callingGetValidMarketPriceUpdates
. This way all suggested price changes are based on the state the prices will be in after the current block is applied.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIuc, Since ExtendVote for block
N
occurs before blockN
is committed, the price updates proposed by blockN
may not be valid?I think I'm a little confused too, because I thought if in Block
N+1
received a consensus through vote extension whichNew + Delta
whereDelta
is less thanmin_price_change
, PrepareProposal would just ignore that price and would not create anUpdateMarketPrices
Tx with a price updateThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. The changes are valid as long as we first apply the changes in block N.
No, the Default prepare handler which uses the existing logic calls the Prices keeper to generate the tx which means prices with less than min_price_change movement are ignored.
In the Slinky prepare handler we just aggregate over the data in the VEs so the responsibility for making sure that data is valid is in the ExtendVoteHandler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have prices transaction as the last one in a block as things such as order matches are created based on old prices. If we update prices in ExtendVote, that will potentially make other transactions invalid