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

[consensus] Add check block in consensus #46

Merged
merged 26 commits into from
Sep 12, 2020
Merged

Conversation

hanyunx
Copy link

@hanyunx hanyunx commented Aug 20, 2020

CheckBlock is called after a proposed block is received and before prevote.
part 2/3 for #26

@hanyunx hanyunx requested a review from a team August 20, 2020 20:42
@hanyunx hanyunx changed the title [consesnsus]Add check block in consensus [consensus]Add check block in consensus Aug 20, 2020
Copy link

@ninjaahhh ninjaahhh left a comment

Choose a reason for hiding this comment

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

ABCIx will compare the hash of the returned Tags with LastResultHash in header (LastResultHash will be renamed to ResultHash in TendermintX). If the comparison fails, TendermintX will treat the block as invalid.

consensus/state.go Outdated Show resolved Hide resolved
state/execution.go Outdated Show resolved Hide resolved
@ninjaahhh
Copy link

invalid check block response includes following scenarios

  1. network / tendermint errors (failed to call proxy.CheckBlock
  2. application errors (non-zero code in response)
  3. response includes invalid transactions (errors inside deliver tx responses)
  4. committed hash mismatch between header and checkBlock response

Copy link

@ninjaahhh ninjaahhh left a comment

Choose a reason for hiding this comment

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

unit tests please

proto/tendermint/abcix/types.proto Outdated Show resolved Hide resolved
state/execution.go Outdated Show resolved Hide resolved
@ninjaahhh ninjaahhh changed the title [consensus]Add check block in consensus [consensus] Add check block in consensus Aug 24, 2020
@ninjaahhh
Copy link

we also need to figure out the timeout mechanism for prevote - since CheckBlock may take longer time, we need compensate this part of time accordingly

@codecov-commenter
Copy link

Codecov Report

Merging #46 into master will increase coverage by 0.50%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
+ Coverage   60.82%   61.32%   +0.50%     
==========================================
  Files         269      271       +2     
  Lines       28290    28508     +218     
==========================================
+ Hits        17206    17483     +277     
+ Misses       9576     9516      -60     
- Partials     1508     1509       +1     
Impacted Files Coverage Δ
abcix/adapter/adapter.go 27.88% <0.00%> (-0.28%) ⬇️
state/execution.go 78.92% <94.11%> (+7.79%) ⬆️
consensus/state.go 72.31% <100.00%> (+0.81%) ⬆️
crypto/sr25519/pubkey.go 37.93% <0.00%> (-6.90%) ⬇️
mempool/mempool.go 83.33% <0.00%> (ø)
libs/llrb/llrb.go 92.59% <0.00%> (ø)
libs/llrb/interface.go 100.00% <0.00%> (ø)
mempool/clist_mempool.go 82.13% <0.00%> (+0.42%) ⬆️
p2p/pex/pex_reactor.go 80.95% <0.00%> (+0.52%) ⬆️
... and 12 more

consensus/common_test.go Outdated Show resolved Hide resolved
state/execution_test.go Show resolved Hide resolved
@@ -62,12 +62,6 @@ func validateBlock(evidencePool EvidencePool, stateDB dbm.DB, state State, block
block.ConsensusHash,
)
}
if !bytes.Equal(block.LastResultsHash, state.LastResultsHash) {

Choose a reason for hiding this comment

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

thinking about this change, I suggest we do another dedicate PR to remove those checks and make this PR only about CheckBlock

consensus/common_test.go Outdated Show resolved Hide resolved
proto/tendermint/abcix/types.proto Outdated Show resolved Hide resolved
state/execution.go Outdated Show resolved Hide resolved
state/execution.go Outdated Show resolved Hide resolved
},
},
}
events := []types.Event{}
return types.ResponseCreateBlock{Txs: txs, InvalidTxs: invalidTxs, Hash: appHash, Events: events}

Choose a reason for hiding this comment

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

just assign events to nil?

txResp := types.ResponseDeliverTx{GasUsed: gasUsed}
ret.DeliverTxs = append(ret.DeliverTxs, &txResp)
}
ret.AppHash = lastState.AppHash

Choose a reason for hiding this comment

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

should add a nil check. if no txs, lastState will be nil pointer

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale label Sep 11, 2020
@ninjaahhh ninjaahhh merged commit ca49119 into master Sep 12, 2020
@ninjaahhh ninjaahhh deleted the check-block-consensus branch September 12, 2020 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants