Add results hash to header by ethanfrey · Pull Request #999 · tendermint/tendermint

ethanfrey · 2017-12-22T15:12:33Z

Address issue #952

This is now maintained in state and save to the db.

TODO:

add LastResultsHash data.Bytes to the header
It should be the merkle root of the {code, data} of the results from the previous block.
add extra methods to the BlockStore for storing all the results for a given block.
Then, to prove a result, we can make one call to the blockstore to load the BlockResults and easily compute the merkle proof
Expose modifications to RPC to make it easy for users to see which transactions in a block were invalid

melekes · 2017-12-22T17:26:12Z

So now we store ABCI results in two places: blockstore and tx_indexer (given user has enabled it). Did not this seem wasteful to you guys?

ethanfrey · 2017-12-22T20:04:40Z

Yup. But it is quite small (just the code (int32) and data (usually nil)) are stored here.
I guess I could go query the tx_indexer for the tx data one by one to reconstruct them, but I think that is a big performance hit for little space savings.

If this does bother you still, please change where it saves the data. Hash is calculated in state, that stays, but the save could change to use tx_indexer and the rpc also, and then we save a little space.

odeke-em · 2017-12-23T03:19:04Z

@ethanfrey am curious, what's the purpose of putting "Results" in the header?

ebuchman · 2017-12-25T15:47:59Z

So now we store ABCI results in two places: blockstore and tx_indexer (given user has enabled it). Did not this seem wasteful to you guys?

The point is to enable cheap queries for all the results in a block.

what's the purpose of putting "Results" in the header?

It's so light clients can verify Merkle proofs of the results of a transaction.

melekes · 2017-12-25T18:18:08Z

rpc/core/blocks.go

+// getBlock(h-1).Txs[5]
+//
+// ```shell
+// curl 'localhost:46657/block_results?height=10'


re ^^: so here we're fetching results for height=9, right?

changed this. now you get what you asked for

ebuchman · 2017-12-25T15:53:20Z

types/block.go

 	ValidatorsHash data.Bytes `json:"validators_hash"` // validators for the current block
 	ConsensusHash  data.Bytes `json:"consensus_hash"`  // consensus params for current block
 	AppHash        data.Bytes `json:"app_hash"`        // state after txs from the previous block
+	ResultsHash    data.Bytes `json:"results_hash"`    // root hash of all results from the txs from the previous block


Shouldn't we call it LastResultsHash ?

I see that none of the other "responses from the previous block" have Last in them, but the tx results seem different, since they really do apply to the previous block, whereas ValidatorsHash/ConsensusHash are a "change to be applied for the next block"

ebuchman · 2017-12-25T15:57:45Z

rpc/core/blocks.go

+//
+// Results are for the tx of the last block with the same index.
+// Thus response.results[5] is the results of executing
+// getBlock(h-1).Txs[5]


Doesn't seem right. This should have the same semantics as /commit. So block_results?height=h should return the results from executing the block at height h

ebuchman · 2017-12-25T15:59:06Z

state/state.go


+	// Store LastABCIResults along with hash
+	LastResults        types.ABCIResults // TODO: remove??
+	LastResultHash     []byte            // this is the one for the next block to propose


I think we only need the one LastResultHash like we only have the one AppHash

codecov-io · 2017-12-25T19:28:17Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@d844799). Click here to learn what that means.
The diff coverage is 74.11%.

@@            Coverage Diff             @@
##             develop     #999   +/-   ##
==========================================
  Coverage           ?   60.12%           
==========================================
  Files              ?      110           
  Lines              ?    10273           
  Branches           ?        0           
==========================================
  Hits               ?     6177           
  Misses             ?     3535           
  Partials           ?      561

State maintains LastResultsHash Verify that we can produce unique hashes for each result, and provide valid proofs from the root hash.

Add some tests. Behaves like saving validator set, except it always saves at each height instead of a reference to last changed.

Expose it in rpc client Move ABCIResults into tendermint/types from tendermint/state

ebuchman · 2017-12-27T01:49:17Z

@melekes want to take another look at this when you get a chance?

Will merge in the meantime

melekes · 2017-12-27T02:04:26Z

LGTM

) * Add `CMT_HOME` (or remove it?) (tendermint#983) Closes tendermint#982 Added `CMT_HOME` everywhere `CMTHOME` is used. ### Notes to reviewers This could be fixed the opposite way, by removing the only reference to `CMT_HOME` in the code, and also the reference in `UPGRADING.md` (two lines of code). However, the reference in `UPGRADING.md`, which is part of our documentation, is already present in `v0.34.x` (not in `v0.37.x` though!). That's why this PR introduces `CMT_HOME` to work in equal conditions as `CMTHOME`. If reviewers lean toward removing `CMT_HOME` from the doc in `v0.34.x` (and unreleased `v0.38.x` and `main`), I can do it easily. --- #### PR checklist - [x] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments (cherry picked from commit b7be568) # Conflicts: # UPGRADING.md # cmd/cometbft/commands/root_test.go * Revert "Add `CMT_HOME` (or remove it?) (tendermint#983)" * Add `CMT_HOME` (or remove it?) (tendermint#983) Closes tendermint#982 Added `CMT_HOME` everywhere `CMTHOME` is used. This could be fixed the opposite way, by removing the only reference to `CMT_HOME` in the code, and also the reference in `UPGRADING.md` (two lines of code). However, the reference in `UPGRADING.md`, which is part of our documentation, is already present in `v0.34.x` (not in `v0.37.x` though!). That's why this PR introduces `CMT_HOME` to work in equal conditions as `CMTHOME`. If reviewers lean toward removing `CMT_HOME` from the doc in `v0.34.x` (and unreleased `v0.38.x` and `main`), I can do it easily. --- - [x] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: Sergio Mena <sergio@informal.systems>

ethanfrey requested review from ebuchman and melekes as code owners December 22, 2017 15:12

ethanfrey changed the title ~~WIP: Add results hash to header~~ Add results hash to header Dec 22, 2017

ebuchman mentioned this pull request Dec 25, 2017

Unify the response protobuf types tendermint/abci#165

Closed

melekes reviewed Dec 25, 2017

View reviewed changes

ebuchman force-pushed the feature/result-hash-header branch from 144d2b9 to d0c0f4d Compare December 25, 2017 19:02

ebuchman reviewed Dec 25, 2017

View reviewed changes

melekes approved these changes Dec 25, 2017

View reviewed changes

ethanfrey and others added 9 commits December 26, 2017 19:24

Add ABCIResults with Hash and Proof to State

f870a49

State maintains LastResultsHash Verify that we can produce unique hashes for each result, and provide valid proofs from the root hash.

Save/Load Results for every height

632cc91

Add some tests. Behaves like saving validator set, except it always saves at each height instead of a reference to last changed.

Add ResultHash to header

58c5df7

Add /block_results?height=H as rpc endpoint

d65234e

Expose it in rpc client Move ABCIResults into tendermint/types from tendermint/state

consolidate saveResults/SaveABCIResponses

73fb1c3

fixes

4171bd3

rpc: getHeight helper function

801e3df

call it LastResultsHash

028ee58

remove LastConsensusParams

051c270

ebuchman force-pushed the feature/result-hash-header branch from d0c0f4d to bfcb40b Compare December 27, 2017 01:00

validate block.ValidatorsHash

bfcb40b

ebuchman mentioned this pull request Dec 27, 2017

Add events to results hash #1007

Closed

ebuchman added 2 commits December 26, 2017 20:08

rpc: fix getHeight

336c2f4

fix test

f80f644

ebuchman approved these changes Dec 27, 2017

View reviewed changes

ebuchman merged commit f95b752 into develop Dec 27, 2017

ebuchman deleted the feature/result-hash-header branch December 27, 2017 01:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add results hash to header#999

Add results hash to header#999
ebuchman merged 12 commits intodevelopfrom
feature/result-hash-header

ethanfrey commented Dec 22, 2017 •

edited

Loading

Uh oh!

melekes commented Dec 22, 2017

Uh oh!

ethanfrey commented Dec 22, 2017

Uh oh!

odeke-em commented Dec 23, 2017

Uh oh!

ebuchman commented Dec 25, 2017

Uh oh!

melekes Dec 25, 2017

Uh oh!

ebuchman Dec 25, 2017

Uh oh!

ebuchman Dec 25, 2017

Uh oh!

ebuchman Dec 25, 2017

Uh oh!

ebuchman Dec 25, 2017

Uh oh!

codecov-io commented Dec 25, 2017 •

edited

Loading

Uh oh!

ebuchman commented Dec 27, 2017

Uh oh!

melekes commented Dec 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

ethanfrey commented Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

melekes commented Dec 22, 2017

Uh oh!

ethanfrey commented Dec 22, 2017

Uh oh!

odeke-em commented Dec 23, 2017

Uh oh!

ebuchman commented Dec 25, 2017

Uh oh!

melekes Dec 25, 2017

Choose a reason for hiding this comment

Uh oh!

ebuchman Dec 25, 2017

Choose a reason for hiding this comment

Uh oh!

ebuchman Dec 25, 2017

Choose a reason for hiding this comment

Uh oh!

ebuchman Dec 25, 2017

Choose a reason for hiding this comment

Uh oh!

ebuchman Dec 25, 2017

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Dec 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ebuchman commented Dec 27, 2017

Uh oh!

melekes commented Dec 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ethanfrey commented Dec 22, 2017 •

edited

Loading

codecov-io commented Dec 25, 2017 •

edited

Loading