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

int64 height (Refs #911) #914

Merged
merged 10 commits into from Dec 3, 2017

Conversation

Projects
None yet
5 participants
@melekes
Contributor

melekes commented Nov 30, 2017

No description provided.

@melekes melekes requested a review from ebuchman as a code owner Nov 30, 2017

@melekes

This comment has been minimized.

Contributor

melekes commented Nov 30, 2017

TODO: ensure there is no underflow here

consensus/replay.go
115:    gr, found, err = cs.wal.SearchForEndHeight(csHeight - 1)

consensus/state.go
705:    lastBlockMeta := cs.blockStore.LoadBlockMeta(height - 1)

state/execution.go
168:                    signed.SetIndex(i, true) // val_.LastCommitHeight = block.Height - 1

blockchain/reactor.go
58:             store.height-- // XXX HACK, make this better

rpc/core/blocks.go
79:     for height := maxHeight; height >= minHeight; height-- {
@melekes

This comment has been minimized.

Contributor

melekes commented Nov 30, 2017

no longer needed

consensus/wal_test.go
52:     gr, found, err := wal.SearchForEndHeight(uint64(h))

types/protobuf.go
16:             Height:         uint64(header.Height),

mempool/mempool.go
436:    return uint64(atomic.LoadUint64(&memTx.height))
@zramsay

This comment has been minimized.

Contributor

zramsay commented Dec 1, 2017

this would close #731

@codecov-io

This comment has been minimized.

codecov-io commented Dec 1, 2017

Codecov Report

Merging #914 into develop will decrease coverage by 0.25%.
The diff coverage is 80.67%.

@@             Coverage Diff             @@
##           develop     #914      +/-   ##
===========================================
- Coverage    58.76%   58.51%   -0.26%     
===========================================
  Files           97       97              
  Lines         9691     9694       +3     
===========================================
- Hits          5695     5672      -23     
- Misses        3458     3473      +15     
- Partials       538      549      +11
@ebuchman

This comment has been minimized.

Contributor

ebuchman commented Dec 1, 2017

Needs to be rebased on develop (#835 was merged). Also needs uint64 -> int64

melekes and others added some commits Nov 30, 2017

int64 height
uint64 is considered dangerous. the details will follow in a blog post.

@melekes melekes changed the title from uint64 height (Refs #911) to int64 height (Refs #911) Dec 2, 2017

@melekes

This comment has been minimized.

Contributor

melekes commented Dec 2, 2017

WARNING: DATA RACE
Read at 0x00c4200bc758 by goroutine 42:
  github.com/tendermint/tendermint/p2p/trust.(*TrustMetric).calcTrustValue()
      github.com/tendermint/tendermint/p2p/trust/_test/_obj_test/trustmetric.go:584 +0xa7
  github.com/tendermint/tendermint/p2p/trust.(*TrustMetric).NextTimeInterval()
      github.com/tendermint/tendermint/p2p/trust/_test/_obj_test/trustmetric.go:348 +0xef
  github.com/tendermint/tendermint/p2p/trust.(*TrustMetric).processRequests()
      github.com/tendermint/tendermint/p2p/trust/_test/_obj_test/trustmetric.go:607 +0xdb

Previous write at 0x00c4200bc758 by goroutine 8:
  [failed to restore the stack]

Goroutine 42 (running) created at:
  github.com/tendermint/tendermint/p2p/trust.NewMetricWithConfig()
      github.com/tendermint/tendermint/p2p/trust/_test/_obj_test/trustmetric.go:448 +0x281
  github.com/tendermint/tendermint/p2p/trust.(*TrustMetricStore).loadFromDB()
      github.com/tendermint/tendermint/p2p/trust/_test/_obj_test/trustmetric.go:147 +0x34a
  github.com/tendermint/tendermint/p2p/trust.TestTrustMetricStoreSaveLoad()
      /go/src/github.com/tendermint/tendermint/p2p/trust/trustmetric_test.go:66 +0x685
  fmt.Fscanf()
      /usr/local/go/src/fmt/scan.go:143 +0xf5
  fmt.Sscanf()
      /usr/local/go/src/fmt/scan.go:114 +0xbe
  github.com/tendermint/tendermint/vendor/github.com/syndtr/goleveldb/leveldb/storage.fsParseName()
      /go/src/github.com/tendermint/tendermint/vendor/github.com/syndtr/goleveldb/leveldb/storage/file_storage.go:571 +0x18c
  github.com/tendermint/tendermint/vendor/github.com/syndtr/goleveldb/leveldb/storage.(*fileStorage).List()
      /go/src/github.com/tendermint/tendermint/vendor/github.com/syndtr/goleveldb/leveldb/storage/file_storage.go:386 +0x2dd

I thought we'd fixed that.
test_integrations (1).log.zip

@ebuchman

This comment has been minimized.

Contributor

ebuchman commented Dec 2, 2017

I thought we'd fixed that.

Looks like a different race :(

@@ -90,15 +90,16 @@ func (cs *ConsensusState) readReplayMessage(msg *TimedWALMessage, newStepCh chan
// replay only those messages since the last block.
// timeoutRoutine should run concurrently to read off tickChan
func (cs *ConsensusState) catchupReplay(csHeight int) error {
// CONTRACT: csHeight > 0

This comment has been minimized.

@ebuchman

ebuchman Dec 2, 2017

Contributor

so, the contract with this whole package is that height > 0.

of course it only really "matters" on functions that have the - 1. So I'm not sure what's best:

  1. put these CONTRACT statements on funcs that subtract.
  2. actually check defensively in the code and return an error or otherwise appropriate response and possibly log the message
  3. check defensively and panic, but I suppose then we need to comment on the func that it might panic
  4. don't do anything and assert at package level that height > 0 and enforce it as early as possible

This comment has been minimized.

@melekes

melekes Dec 2, 2017

Contributor

this was for uint. i'll remove

@@ -697,7 +697,8 @@ func (cs *ConsensusState) enterNewRound(height int, round int) {
// needProofBlock returns true on the first height (so the genesis app hash is signed right away)
// and where the last block (height-1) caused the app hash to change
func (cs *ConsensusState) needProofBlock(height int) bool {
// CONTRACT: height > 0

This comment has been minimized.

@ebuchman

ebuchman Dec 2, 2017

Contributor

see above

func (memTx *mempoolTx) Height() int {
return int(atomic.LoadInt64(&memTx.height))
func (memTx *mempoolTx) Height() int64 {
return atomic.LoadInt64(&memTx.height)

This comment has been minimized.

@ebuchman

ebuchman Dec 2, 2017

Contributor

heh!

@@ -42,7 +42,7 @@ func WaitForHeight(c StatusClient, h int, waiter Waiter) error {
if err != nil {
return err
}
delta = h - s.LatestBlockHeight
delta = int(h - s.LatestBlockHeight)

This comment has been minimized.

@ebuchman

ebuchman Dec 2, 2017

Contributor

?

This comment has been minimized.

@melekes

melekes Dec 2, 2017

Contributor

latestblockheight is int64. delta is int

This comment has been minimized.

@ebuchman

ebuchman Dec 2, 2017

Contributor

shouldnt we change delta to int64?

This comment has been minimized.

@melekes

melekes Dec 2, 2017

Contributor

yes!

logger.Debug("BlockchainInfoHandler", "maxHeight", maxHeight, "minHeight", minHeight)
if minHeight > maxHeight {

This comment has been minimized.

@ebuchman

ebuchman Dec 2, 2017

Contributor

nice thanks

@@ -84,18 +84,18 @@ func Tx(hash []byte, prove bool) (*ctypes.ResultTx, error) {
}
height := r.Height
index := r.Index
index := int(r.Index) // XXX:overflow

This comment has been minimized.

@ebuchman

ebuchman Dec 2, 2017

Contributor

cool should be fixed with new abci

// TODO: handle overflow
block := blockStore.LoadBlock(int(height))
proof = block.Data.Txs.Proof(int(index))

This comment has been minimized.

@ebuchman

ebuchman Dec 2, 2017

Contributor

can remove this int. but lets just wait till abci update

This comment has been minimized.

@melekes

melekes Dec 2, 2017

Contributor

should linter catch such things?

This comment has been minimized.

@ebuchman

ebuchman Dec 2, 2017

Contributor

not sure @zramsay . we dont have all linters active yet

@@ -53,7 +53,7 @@ func (valSet *ValidatorSet) IncrementAccum(times int) {
// Add VotingPower * times to each validator and order into heap.
validatorsHeap := cmn.NewHeap()
for _, val := range valSet.Validators {
val.Accum += int64(val.VotingPower) * int64(times) // TODO: mind overflow
val.Accum += val.VotingPower * int64(times) // TODO: mind overflow

This comment has been minimized.

@ebuchman

ebuchman Dec 2, 2017

Contributor

this one's actually a little scary.

This comment has been minimized.

@melekes

melekes Dec 2, 2017

Contributor

very scary

This comment has been minimized.

@ebuchman

ebuchman Dec 2, 2017

Contributor

opened an issue #919

melekes added some commits Dec 2, 2017

@ebuchman ebuchman merged commit 72e3890 into develop Dec 3, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 80.67% of diff hit (target 58.76%)
Details
codecov/project Absolute coverage decreased by -0.25% but relative coverage increased by +21.9% compared to b2489b4
Details

@zramsay zramsay deleted the 11-response-info-with-invalid-height-crashes-tm branch Dec 3, 2017

@silasdavis

This comment has been minimized.

Contributor

silasdavis commented Dec 21, 2017

@melekes missed this when it was going through. Any reason why we are not using uint64 since block height cannot be negative. I have been using uint64 on my side and was hoping that you would adopt that too...

@ebuchman

This comment has been minimized.

Contributor

ebuchman commented Dec 21, 2017

uints are bad for arithmetic. After extensive research, the general consensus seems to be that uints are only good for things that will never be involved in arithmetic - eg. bitarrays/masks.

The risk of messing up the arithmetic is much greater than the type enforcement that it will not be negative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment