Skip to content

state: require block.Time of the fist block to be genesis time#2594

Merged
ebuchman merged 5 commits intodevelopfrom
2587-validate-block-time-height-1
Oct 12, 2018
Merged

state: require block.Time of the fist block to be genesis time#2594
ebuchman merged 5 commits intodevelopfrom
2587-validate-block-time-height-1

Conversation

@melekes
Copy link
Contributor

@melekes melekes commented Oct 10, 2018

Refs #2587

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

@melekes
require block.Time of the fist block to be genesis time
2d05d51 
Refs #2587:

```
We only start validating block.Time when Height > 1, because there is no
commit to compute the median timestamp from for the first block. This
means a faulty proposer could make the first block with whatever time
they want.

Instead, we should require the timestamp of block 1 to match the genesis
time.

I discovered this while refactoring the ValidateBlock tests to be
table-driven while working on tests for #2560.
```
@melekes
do not accept blocks with negative height
e4c6398
@melekes
update changelog and spec
75ec9b7
@melekes
nanos precision for test genesis time
8aedcd3
@melekes
Copy link
Contributor Author

melekes commented Oct 10, 2018 •
edited
Loading 

I[10-10|08:52:05.220] Updating ValidBlock because of POL.          module=consensus validRound=0 POLRound=3
D[10-10|08:52:05.220] enterNewRound(1/3): Invalid args. Current step: 1/3/RoundStepPrevote module=consensus height=1 round=3
I[10-10|08:52:05.220] enterPrecommit(1/3). Current: 1/3/RoundStepPrevote module=consensus height=1 round=3
I[10-10|08:52:05.221] enterPrecommit: +2/3 prevoted locked block. Relocking module=consensus height=1 round=3
I[10-10|08:52:05.221] Signed and pushed vote                       module=consensus height=1 round=3 vote="Vote{0:2CEF12552905 1/03/2(Precommit) 0D20984CDF24 56BE7AEA68D5 @ 2018-10-10T08:52:05.2211871Z}" err=null
D[10-10|08:52:05.221] addVote                                      module=consensus voteHeight=1 voteType=2 valIndex=0 csHeight=1
I[10-10|08:52:05.222] Added to precommit                           module=consensus vote="Vote{0:2CEF12552905 1/03/2(Precommit) 0D20984CDF24 56BE7AEA68D5 @ 2018-10-10T08:52:05.2211871Z}" precommits="VoteSet{H:1 R:3 T:2 +2/3:<nil>(0.5) BA{2:x_} map[]}"
--- FAIL: TestStateLockNoPOL (0.29s)
	state_test.go:381: #### ONTO ROUND 1
	state_test.go:426: #### ONTO ROUND 2
	state_test.go:467: #### ONTO ROUND 3
panic: Timeout expired while waiting for NewTimeout event [recovered]
	panic: Timeout expired while waiting for NewTimeout event

goroutine 37416 [running]:
testing.tRunner.func1(0xc424828870)
	/usr/local/go/src/testing/testing.go:742 +0x567
panic(0xef3b80, 0xc42896f0e0)
	/usr/local/go/src/runtime/panic.go:502 +0x24a
github.com/tendermint/tendermint/consensus.ensureNewEvent(0xc4272e4720, 0x2faf080, 0x1047f16, 0x32)
	/go/src/github.com/tendermint/tendermint/consensus/common_test.go:334 +0x159
github.com/tendermint/tendermint/consensus.ensureNewTimeout(0xc4272e4720, 0x989680)
	/go/src/github.com/tendermint/tendermint/consensus/common_test.go:352 +0x5c
github.com/tendermint/tendermint/consensus.TestStateLockNoPOL(0xc424828870)
	/go/src/github.com/tendermint/tendermint/consensus/state_test.go:487 +0x19ec
testing.tRunner(0xc424828870, 0x117d4d8)
	/usr/local/go/src/testing/testing.go:777 +0x16e
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:824 +0x565
FAIL	github.com/tendermint/tendermint/consensus	26.267s
Exited with code 1

milosevic
milosevic reviewed Oct 10, 2018
View reviewed changes
@melekes melekes changed the title state: require block.Time of the fist block to be genesis time [WIP] state: require block.Time of the fist block to be genesis time Oct 11, 2018
@melekes melekes changed the title [WIP] state: require block.Time of the fist block to be genesis time state: require block.Time of the fist block to be genesis time Oct 11, 2018
@codecov-io
Copy link

codecov-io commented Oct 11, 2018

Codecov Report

Merging #2594 into develop will decrease coverage by 0.04%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #2594      +/-   ##
===========================================
- Coverage    61.47%   61.42%   -0.05%     
===========================================
  Files          202      202              
  Lines        16703    16708       +5     
===========================================
- Hits         10268    10263       -5     
- Misses        5568     5577       +9     
- Partials       867      868       +1
Impacted Files Coverage Δ
p2p/netaddress.go 72.39% <ø> (ø) ⬆️
config/toml.go 53.19% <ø> (ø) ⬆️
state/validation.go 63.93% <100%> (+2.53%) ⬆️
state/state.go 90.58% <100%> (+2.08%) ⬆️
cmd/tendermint/commands/lite.go 17.5% <100%> (ø) ⬆️
crypto/encoding/amino/amino.go 82.75% <0%> (-6.14%) ⬇️
consensus/reactor.go 72.09% <0%> (-1.04%) ⬇️
node/node.go 65.41% <0%> (-0.25%) ⬇️
consensus/state.go 76.98% <0%> (-0.24%) ⬇️

ValarDragon
ValarDragon approved these changes Oct 11, 2018
View reviewed changes
Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

Lgtm!

ebuchman
ebuchman approved these changes Oct 12, 2018
View reviewed changes
types/block.go
b.mtx.Lock()
defer b.mtx.Unlock()

if b.Height < 0 {
Copy link
Contributor

@ebuchman ebuchman Oct 12, 2018

Choose a reason for hiding this comment

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

Nice. Though technically we don't accept b.Height == 0 either

@ebuchman ebuchman merged commit e1538bf into develop Oct 12, 2018
@ebuchman ebuchman deleted the 2587-validate-block-time-height-1 branch October 12, 2018 05:04
@ebuchman ebuchman mentioned this pull request Oct 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xla xla Awaiting requested review from xla

@zramsay zramsay Awaiting requested review from zramsay

3 more reviewers

@milosevic milosevic milosevic approved these changes

@ebuchman ebuchman ebuchman approved these changes

@ValarDragon ValarDragon ValarDragon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@melekes @codecov-io @milosevic @ebuchman @ValarDragon