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

feat: Add PrepareCheckState and Precommit callbacks #14860

Merged
merged 23 commits into from
Apr 23, 2023

Conversation

prettymuchbryce
Copy link
Contributor

@prettymuchbryce prettymuchbryce commented Jan 31, 2023

Description

This PR introduces optional PrepareCheckState and Precommit callbacks. It allows modules to optionally include these methods.

These callbacks are similar to the existing callbacks for BeginBlock and EndBlock that modules may register, but slightly simpler they accept no arguments besides an sdk.Context, and they have no return values.

  • Precommit - Invoked during BaseApp.Commit() before the deliverState is written to the rootMultiStore. It is invoked with the deliverState. This could be useful for data/metrics pipelines that want to flush data to some external source, immediately before the deliverState for the block is written, and before any TransientStore within the deliverState has been reset.

  • PrepareCheckState - Invoked during BaseApp.Commit() after the deliverState has been written to the rootMultiStore, the deliverState has been emptied/reset, and the checkState has been set with the new block header. Invoked with the checkState of the next block. dYdX has introduced this functionality into our own fork as a way to prepare the checkState with some data immediately after the block has been committed.


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)

baseapp/abci.go Outdated
Comment on lines 458 to 494
// empty/reset the deliver state
app.deliverState = nil

if app.commiter != nil {
app.commiter(app.checkState.ctx)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Change potentially affects state.

Call sequence:

(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).Commit (baseapp/abci.go:426)

@facundomedica
Copy link
Member

Hello! I have some questions:

  1. What kind of data are you putting into checkState? Can this be done in PrepareProposal?
  2. What modules would do during Commit? They are going to be setting the data in checkState?
  3. Can you point out some use cases for this?

@prettymuchbryce
Copy link
Contributor Author

Hey @facundomedica. Great questions!

What kind of data are you putting into checkState?

Currently we are putting data related to our orderbook in the checkState. This is because in our design, validators match orders intra-block rather than during EndBlock/Commit. After the block has been committed, each validator needs to "unwind" their own orders in order to account for any matches included in the block by the proposer. This results in changes to the checkState.

Can this be done in PrepareProposal?

This can't be done in PrepareProposal as PrepareProposal is only called by the proposer, and not all validators.

What modules would do during Commit? They are going to be setting the data in checkState?

Yes. Currently modules can set data in the checkState. This data will then be present for transactions during CheckTx after the block has been committed.

Can you point out some use cases for this?

I can't think of any other use-cases other than our own. Our own use-case may also be unique given that we perform a high number of state writes during CheckTx for our orderbook. AFAIK the default modules don't do this, and the only state writes that occur during CheckTx are ones done in the AnteHandlers, like updating the sequence number, and deducting fees.

It's very possible that this may be something unique to our own use-case, and if that is true then we can continue to maintain a lightweight fork that includes this change for the time being.

@yihuang
Copy link
Collaborator

yihuang commented Feb 7, 2023

Is something like PrepareCheckState more explicit here, with Commit I would think modules can commit other db together with the main db.
By put stuff in checkState do you mean transient store?

@prettymuchbryce
Copy link
Contributor Author

Is something like PrepareCheckState more explicit here, with Commit I would think modules can commit other db together with the main db.

Yes I agree with your thinking here. It's not clear based on the current implementation that the state within the context is the checkState rather than the uncommitted deliverState. Renaming the callback from Commit to PrepareCheckState would help us clarify the intention of the callback, and would self-document the state that will be present in the context.

By put stuff in checkState do you mean transient store?

No this is not a TransientStore. Within our Commit callback, we modify the CacheMultiStore that has been branched for the checkState. This is the same CacheMultiStore that is present in the context during CheckTx.

@alexanderbez
Copy link
Contributor

So what I gather from this as the gist is that you want a way for all validators to modify the CheckTx state prior to the next block being called, as opposed to it being set to the resulting state after Commit as it is today?

If so, I don't immediately see anything wrong with this and I'm not opposed. I think clear and clean APIs with sufficient documentation would be enough for me to give a green light.

I don't want to encourage teams to maintain their own forks when it's not necessary :)

@itsdevbear
Copy link
Contributor

@alexanderbez +1 here, this would be helpful for our evm

@alexanderbez
Copy link
Contributor

Yeah sounds good. Feel free to push it over the finish line 👍

@itsdevbear
Copy link
Contributor

itsdevbear commented Feb 18, 2023

@alexanderbez would you be open to moving the hook to BEFORE the multistore is committed.

Our use case actually needs access to the transient store from the block.

Pros and cons to both I guess.

Unless we add PreCommit() PostCommit()?

Seems like @prettymuchbryce needs it after the commit though. So maybe this needs a broader discussion

@alexanderbez
Copy link
Contributor

@itsdevbear can you further elaborate on your use-case? The original proposal was to modify CheckTx after commitment. How does yours differ?

@prettymuchbryce
Copy link
Contributor Author

@itsdevbear We recently had a similar requirement in our codebase as well, so we decided to add a second callback for PreCommit which was implemented by my colleague @dydxwill. Can you take a look here and let me know if it matches your thinking?

I wonder if there would be appetite to include both of these upstream. We can help clean things up and move both callbacks into this PR.

I think clear and clean APIs with sufficient documentation would be enough for me to give a green light.

@alexanderbez I'm curious what other documentation we should be thinking about other than just the Godoc comments. For example this section of the documentation, or other places I may be missing.

@itsdevbear
Copy link
Contributor

Our use case is to dump data from the previous block to an off chain store for indexing. Right now we are doing it in end block, which works but it's technically possible for the previous block to error out.

Doing it on commit feels much safer. But we can't do it before the ctx is reset. Since we were thinking storing this data in a transient store during the block and dumping to an off chain disk at the end.

@yihuang
Copy link
Collaborator

yihuang commented Feb 27, 2023

Our use case is to dump data from the previous block to an off chain store for indexing. Right now we are doing it in end block, which works but it's technically possible for the previous block to error out.

even if it error out, it'll be replayed eventually, so probably not a big deal?

@alexanderbez
Copy link
Contributor

@itsdevbear if it's for indexing, why don't you just use the state streamer. That's what it was designed for.

@prettymuchbryce I would say any baseapp or ABCI docs should have mention of this 👍

@itsdevbear
Copy link
Contributor

yeah @alexanderbez that was the other thing we were thinking. Will investigate that path as well.

Ty ser

either way I think pre and post still useful here.

@tac0turtle
Copy link
Member

@itsdevbear @prettymuchbryce would you be willing to come on the community call next week to discuss this feature sync so we can move this forward

@itsdevbear
Copy link
Contributor

@tacoturtle definitely down, we can also chat about: #14660 it's a relatively a critical for us one for us.

Can you send me the info / send a cal recurring cal invite?

@prettymuchbryce
Copy link
Contributor Author

I am down. Can you send me an invite?

@alexanderbez
Copy link
Contributor

@prettymuchbryce are you open to adding the post-hook in this PR as you did with the pre-hook, so we can achieve the needs of both teams? Happy to review and approve.

@prettymuchbryce prettymuchbryce force-pushed the prettymuchbryce/add-commiter branch from 5066667 to cda4b81 Compare March 22, 2023 19:10
types/module/module.go Outdated Show resolved Hide resolved
baseapp/abci.go Outdated
Comment on lines 485 to 494
// Commit. Use the header from this latest block.
app.setState(runTxModeCheck, header)

if app.precommiter != nil {
app.precommiter(app.deliverState.ctx)
}

// empty/reset the deliver state
app.deliverState = nil

Copy link
Contributor

Choose a reason for hiding this comment

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

Change potentially affects state.

Call sequence:

(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).Commit (baseapp/abci.go:452)

telemetry/wrapper.go Outdated Show resolved Hide resolved
@@ -456,14 +456,18 @@ func (app *BaseApp) Commit() abci.ResponseCommit {
header := app.deliverState.ctx.BlockHeader()
retainHeight := app.GetBlockRetentionHeight(header.Height)

if app.precommiter != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@itsdevbear Was curious if this is where you envisioned this being invoked. At this point, your TransientStore will not have been cleared yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense to me. If we are passing in the ctx without nuking it, I don't think it really matters where it happens.

@prettymuchbryce
Copy link
Contributor Author

As far as I can tell the remaining CI failures in this PR are not related to my changes. Let me know if I am missing something.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm!

As far as I can tell the remaining CI failures in this PR are not related to my changes. Let me know if I am missing something.

Yes, these CI failures are unrelated.

@julienrbrt julienrbrt self-assigned this Apr 4, 2023
runtime/app.go Show resolved Hide resolved
runtime/app.go Show resolved Hide resolved
baseapp/options.go Outdated Show resolved Hide resolved
@itsdevbear
Copy link
Contributor

@prettymuchbryce we've been using this branch, has been good so far.

Copy link
Member

@facundomedica facundomedica left a comment

Choose a reason for hiding this comment

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

LGTM. This might change a bit with the implementation of abci 1.0, but I don't foresee big changes given that Commit() will remain pretty much the same iirc

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

utACK. we can do a follow up pr to add docs

@tac0turtle tac0turtle added this pull request to the merge queue Apr 23, 2023
@itsdevbear
Copy link
Contributor

⚡️🎉

Merged via the queue into cosmos:main with commit 93d64cc Apr 23, 2023
@julienrbrt
Copy link
Member

julienrbrt commented Apr 24, 2023

Tagged cosmossdk.io/api v0.4.1 with this.

@itsdevbear
Copy link
Contributor

Pulled sdk main into polaris, it works great.

Thanks @prettymuchbryce for pushing this across.

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.

10 participants