Upgrade abci and tm versions #76

ruseinov · 2018-06-20T10:28:57Z

Get chainID from initChain again
Update tests
Update pubKey indexing according to the new message format

Get chainID from initChain again Update tests Update pubKey indexing according to the new message format

codecov · 2018-06-20T10:31:07Z

Codecov Report

Merging #76 into master will decrease coverage by 0.4%.
The diff coverage is 50%.

@@           Coverage Diff            @@
##           master     #76     +/-   ##
========================================
- Coverage    77.9%   77.4%   -0.5%     
========================================
  Files          56      56             
  Lines        2399    2443     +44     
========================================
+ Hits         1869    1893     +24     
- Misses        425     445     +20     
  Partials      105     105

Impacted Files	Coverage Δ
app/store.go	`58.5% <50%> (-0.4%)`	⬇️
x/persistent.go	`83.3% <0%> (-8%)`	⬇️
x/cash/handler.go	`80% <0%> (-1.3%)`	⬇️
conditions.go	`100% <0%> (ø)`	⬆️
x/cash/decorator.go	`100% <0%> (ø)`	⬆️
app/genesis.go	`0% <0%> (ø)`	⬆️
examples/mycoind/app/examples.go	`0% <0%> (ø)`	⬆️
x/sigs/decorator.go	`94.1% <0%> (+0.7%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10b9ce2...833f5d7. Read the comment docs.

ruseinov · 2018-06-20T10:32:11Z

@ethanfrey So we've got the following changes:

BREAKING CHANGES:

[example/dummy] Remove. See example/kvstore
[types] Upgrade many messages:
RequestInitChain takes all fields from a Genesis file - we don't really need anything, but the chainID retrieval here, which I have already implemented
RequestBeginBlock provides a list of all validators and whether or not they signed - do we need to store these?
Header: remove some fields, add proposer - probably does not matter for weave implementation
BlockID, PartSetHeader: remove - same as above
Validator: includes address - same as above
PubKey: new message with type and data - I have fixed the comparison
Evidence: add type and more fields - Does not seem to be used in weave
FEATURES:

[types] Add some fields - Anything here?
ResponseInitChain includes ConsensusParams and Validators
ResponseBeginBlock includes tags
ResponseEndBlock includes tags

ethanfrey · 2018-06-20T11:52:03Z

What we care about:

RequestInitChain takes all fields from a Genesis file - we don't really need anything, but the chainID retrieval here, which I have already implemented

Yup. we will want to store the set of initial validators later, or pass down to the apps to store it. Not needed now, but I will create an issue.

RequestBeginBlock provides a list of all validators and whether or not they signed - do we need to store these?

We don't need to store them at all, but we may want to place them in the Context, so an application could take action. This is only really useful when we have actions that run every block in BeginBlock, so I will create a future issue for this as well.

Header: remove some fields, add proposer - probably does not matter for weave implementation
BlockID, PartSetHeader: remove - same as above

No one uses these fields now, they are all passed down the line. Good to go.

Validator: includes address - same as above

Validator in init genesis? I didn't think there was a validator in BeginBlock? But maybe I am wrong.
We want the validator format used in the EndBlock Diff to be totally compatible with tendermint. If we need to change something here, please do it.

PubKey: new message with type and data - I have fixed the comparison

👍 this is a key change we need, so the validator diff works with the new version.

Evidence: add type and more fields - Does not seem to be used in weave
[types] Add some fields - Anything here?

ResponseInitChain includes ConsensusParams and Validators

This one is interesting. What happens if we don't provide Validators??? It just uses the validators from the genesis file, right? Or do we have to explicitly return the same set as we get in from the input to InitChain? It would be good to investigate and add a comment on InitChain how this works. If we provide values, do they override the genesis value, or are the combined together (like the EndBlock Diff)?

ResponseBeginBlock includes tags
ResponseEndBlock includes tags

We do not use these now, but maybe add a // TODO or // NOTE: to these methods in app/store.go so we remember they are there... we will want to add a BeginBlock handler at some point to deal with repeating or delayed tasks and it is nice to know this can add tags.

Many of the features are added for the complex staking model used in cosmos, and they are not necessary for us now, but it is good to know they are there, and when we add an interface to handle BeginBlock, we should make sure to include that it can return Tags.

ethanfrey

Looks good. One suggestion for an enhancement

ethanfrey · 2018-06-20T12:49:23Z

app/store.go

@@ -111,6 +111,11 @@ func (s *StoreApp) parseAppState(data []byte, init weave.Initializer) error {
 		return errors.WithCode(err, errors.CodeTxParseError)
 	}

+	err = s.storeChainID(chainID)


ethanfrey · 2018-06-20T12:51:37Z

app/store.go

@@ -345,7 +342,7 @@ func (s *StoreApp) AddValChange(diffs []abci.Validator) {
 // return index of list with validator of same PubKey, or -1 if no match
 func pubKeyIndex(val abci.Validator, list []abci.Validator) int {
 	for i, v := range list {
-		if bytes.Equal(val.PubKey, v.PubKey) {
+		if val.PubKey.Type == v.PubKey.Type && bytes.Equal(val.PubKey.Data, v.PubKey.Data) {


ethanfrey · 2018-06-20T12:53:15Z

examples/mycoind/app/app_test.go

 	// Commit first block, make sure non-nil hash
-	header := abci.Header{Height: h, ChainID: chainId}
+	header := abci.Header{Height: h}


Better this way.
Just wondering, maybe we verify chainID of every header on BeginBlock? And return an error if it is present and different from the one stored in InitCHain

Well, I was not sure it's necessary, but we can indeed panic with an error if stuff does not match. Kind of provides "extra security", but on the other hand this should not be happening if the init was correct, otherwise we can't trust tendermint at all. Unless I'm missing something.

What do you think?

I guess what I'm trying to say is: double checking is good, but it leads to more code and more things that could potentially break/need to be supported while sometimes providing us with no benefit. I used to do that a lot on every level of an application, but then learned to limit myself to what's necessary.

I might be wrong about this one, so I leave this call up to you.

Makes me look ridiculous, fighting for one line of code here 🗡

Please leave the tests as is.

I just got contacted from some people, where they ended up replaying blocks from the wrong chain in some dev setup. I thought a second check would not be bad.

But on reflection, the Info method on handshake will show a different AppHash than the chain, so tendermint should prohibit that anyway.

Ignore the above comment.

@ethanfrey cool, not changing anything here then.

ruseinov · 2018-06-20T14:49:34Z

@ethanfrey I will investigate the evidence stuff and put TODO's about other stuff as well these coming days.

ruseinov · 2018-06-21T07:39:17Z

@ethanfrey added some TODO's, ready for merge. Feel free to add tickets for whatever I might have missed.

* Replace confio with iov packages * Fix utils imports * Fix test imports * More imports

Upgrade abci and tm versions

5ea7105

Get chainID from initChain again Update tests Update pubKey indexing according to the new message format

ruseinov assigned ethanfrey Jun 20, 2018

ruseinov requested a review from ethanfrey June 20, 2018 10:28

ruseinov changed the title ~~Upgrade abci and tm versions~~ WIP: Upgrade abci and tm versions Jun 20, 2018

faster comparison

851e684

ethanfrey mentioned this pull request Jun 20, 2018

Expose more genesis fields to extensions #77

Closed

This was referenced Jun 20, 2018

Enable registering handler/callback for begin block #78

Closed

Add validators that signed the block to context #79

Closed

ethanfrey reviewed Jun 20, 2018

View reviewed changes

ruseinov changed the title ~~WIP: Upgrade abci and tm versions~~ Upgrade abci and tm versions Jun 21, 2018

add todo

833f5d7

ethanfrey merged commit 9476ac7 into master Jun 21, 2018

ethanfrey deleted the feature/upgrade-tm-0.20.0 branch June 21, 2018 14:51

alpe added a commit that referenced this pull request Oct 29, 2018

Replace confio with iov packages (#76)

7de1fda

* Replace confio with iov packages * Fix utils imports * Fix test imports * More imports

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade abci and tm versions #76

Upgrade abci and tm versions #76

ruseinov commented Jun 20, 2018

codecov bot commented Jun 20, 2018 •

edited

Loading

ruseinov commented Jun 20, 2018

ethanfrey commented Jun 20, 2018

ethanfrey left a comment

ethanfrey Jun 20, 2018

ethanfrey Jun 20, 2018

ethanfrey Jun 20, 2018

ruseinov Jun 20, 2018

ruseinov Jun 20, 2018

ethanfrey Jun 20, 2018 •

edited

Loading

ruseinov Jun 20, 2018

ruseinov commented Jun 20, 2018

ruseinov commented Jun 21, 2018

Upgrade abci and tm versions #76

Upgrade abci and tm versions #76

Conversation

ruseinov commented Jun 20, 2018

codecov bot commented Jun 20, 2018 • edited Loading

Codecov Report

ruseinov commented Jun 20, 2018

ethanfrey commented Jun 20, 2018

ethanfrey left a comment

Choose a reason for hiding this comment

ethanfrey Jun 20, 2018

Choose a reason for hiding this comment

ethanfrey Jun 20, 2018

Choose a reason for hiding this comment

ethanfrey Jun 20, 2018

Choose a reason for hiding this comment

ruseinov Jun 20, 2018

Choose a reason for hiding this comment

ruseinov Jun 20, 2018

Choose a reason for hiding this comment

ethanfrey Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

ruseinov Jun 20, 2018

Choose a reason for hiding this comment

ruseinov commented Jun 20, 2018

ruseinov commented Jun 21, 2018

codecov bot commented Jun 20, 2018 •

edited

Loading

ethanfrey Jun 20, 2018 •

edited

Loading