-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
refactor(core): use working hash in finalizeblock instead of flush #16065
Conversation
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.
LGTM. There is one doubt I have which I left a comment on.
// state transitions will be flushed to disk and as a result, but we already have | ||
// an application Merkle root. | ||
func (app *BaseApp) workingHash() []byte { | ||
app.finalizeBlockState.ms.Write() |
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.
What's the reason for this call?
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.
since we moved the writing from comit to working hash now after Initchain & commit are called there is still no state, so we need to write to disk in order to call commit
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.
I think it's still a misnomer to call this workingHash
as the name implies it's idempotent. But in reality, this now has side affects. Perhaps consider moving the flush/write to disk back to Commit
.
baseapp/abci.go
Outdated
func (app *BaseApp) flushCommit() storetypes.CommitID { | ||
app.finalizeBlockState.ms.Write() | ||
|
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.
the name of flashCommit
doesn't make sense since get rid of Write
, ?
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.
ill change to commit to make it clearer, thanks for the pointer
Description
ref #12272
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
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...
!
in the type prefix if API or client breaking change