-
Notifications
You must be signed in to change notification settings - Fork 148
go/consensus/cometbft/abci/system: Change events root #6289
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
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for oasisprotocol-oasis-core canceled.
|
f4a7992 to
4023dbf
Compare
4023dbf to
f9c9a08
Compare
| return nil | ||
| } | ||
| return merkle.RootHash(provableEvents), nil | ||
| return cmttypes.NewResults(mux.state.proposal.resultsDeliverTx).Hash() |
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.
Only DeliverTx events?
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.
Yes, this is how cometbft works.
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.
How do you prove other events?
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.
Sorry, my answer was wrong. What I meant was that since NewResults strips all "non-deterministic" fields, no events can be verified using this hash. That holds also for events from DeliverTx. However, you can verify transaction results but only fields Code, Data, GasWanted, GasUsed.
To prove events, one must use our Merkle root, which will change if we change events. Observe that this root doesn't hold information in which phase of the block execution event occurred (begin, ..., end) as all current consumers don't care about that, and the implementation simplifies as we don't need to create root for every phase.
f9c9a08 to
1113268
Compare
The events root in block metadata system transaction now includes all events, not just provable ones.
The results hash was added to the block metadata system transaction so that stateless clients can partially verify the latest block’s results.
1113268 to
5b2ad42
Compare
kostko
left a comment
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.
Would be good to validate this does not introduce any issues on Testnet and Mainnet.
No description provided.