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: use deliverState in prepare and processProposal at height 1 #14505

Merged
merged 21 commits into from
Jan 8, 2023

Conversation

facundomedica
Copy link
Member

@facundomedica facundomedica commented Jan 5, 2023

Description

Ref: #14446
Audit: #13951

Issue: State couldn't be queried on PrepareProposal and ProcessProposal on the first block, so you couldn't get params or get stuff changed during the InitChain (mainly genesis).

We now use a cached context from deliverState during the first block. This allows us to query any state changed during InitChain (which is done on deliverState).
After the first block we use prepareProposalState or processProposalState, as deliverState is not initialized and there's no state being changed prior to PrepareProposal (in the ABCI flow there's no step prior after the first block is produced).

A check for invalid height was added to PrepareProposal as it's invalid to pass anything < 1. I don't see this happening in real life but there were some invalid tests.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@facundomedica facundomedica added the backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release label Jan 5, 2023
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.

PHENomenal. Great job. Thanks 👏

The more folks that understand ABCI++ the better 🙌

Copy link
Contributor

@JeancarloBarrios JeancarloBarrios left a comment

Choose a reason for hiding this comment

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

Nice!!

baseapp/abci.go Outdated
}

ctx := app.prepareProposalState.ctx
// Here we use deliverState on the first block given that we want to be able
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious - why is the state with changes that InitChain made not available to the app?

Copy link
Contributor

@alexanderbez alexanderbez Jan 5, 2023

Choose a reason for hiding this comment

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

wdym "not available to the app"?

After InitChain, changes are not committed. They're technically staged/cached on the (cached) root multi-store. This isn't what prepareProposal state is based off of until Commit is called.

Copy link
Member Author

Choose a reason for hiding this comment

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

InitChain initializes and applies changes (params, genesis and such) to deliverState, which is not persisted until Commit(): see here.
In Commit() deliverState is set to nil while the other states are set to the latest header (that's why we were seeing the previous block height in PrepareProposal before adding WithBlockHeight(req.Height) in this PR).

So PrepareProposal and ProcessProposal will always be seeing persisted state (from the previous produced block), except for the first block that we feed it with the genesis state. Also it's good to note that we shouldn't be making any persistable state changes during these stages.

Also BeginBlock is the one in charge of setting deliverState; except when we are at the first block: in that case we just want to update the context retaining any state changes made in InitChain. see here

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks for the explanation. That makes sense.

Random alternative thoughts:

  1. Would it be better to dedicate height == 1 to InitChain only? meaning that the at height 1, CosmosSDK just runs InitChain and then calls Commit. No special logic to override the ctx state to be the deliverState which has not been committed. My main reasoning around this is if there would be any weird behavior for the PrepareProposal to be based off of state that has not been committed to state.
  2. Similarly, is it possible to introduce some special height 0 that does the InitChain and Commit only

baseapp/abci.go Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Jan 6, 2023

[Cosmos SDK] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Contributor

@ttl33 ttl33 left a comment

Choose a reason for hiding this comment

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

lgtm! thank you for the fix and the explanations 👍

left some random alternative thoughts out of curiosity

@alexanderbez alexanderbez enabled auto-merge (squash) January 8, 2023 15:49
@alexanderbez
Copy link
Contributor

@facundomedica lets resolve the merge conflict and get this merged 🙌

@alexanderbez alexanderbez merged commit 3fc4f33 into main Jan 8, 2023
@alexanderbez alexanderbez deleted the facu/fix-prep-prop-2 branch January 8, 2023 19:29
mergify bot pushed a commit that referenced this pull request Jan 8, 2023
)

(cherry picked from commit 3fc4f33)

# Conflicts:
#	CHANGELOG.md
tac0turtle pushed a commit that referenced this pull request Jan 9, 2023
…kport #14505) (#14533)

Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release C:ABCI C:baseapp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants