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

types: Cap evidence in block validation #2560

Merged
merged 5 commits into from
Oct 9, 2018
Merged

Conversation

ebuchman
Copy link
Contributor

@ebuchman ebuchman commented Oct 6, 2018

Closes #1637

Still need to write tests

EDIT: wrote tests!

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

@codecov-io
Copy link

codecov-io commented Oct 6, 2018

Codecov Report

Merging #2560 into develop will increase coverage by 0.16%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           develop   #2560      +/-   ##
==========================================
+ Coverage    61.23%   61.4%   +0.16%     
==========================================
  Files          202     202              
  Lines        16733   16747      +14     
==========================================
+ Hits         10246   10283      +37     
+ Misses        5617    5593      -24     
- Partials       870     871       +1
Impacted Files Coverage Δ
state/validation.go 61.4% <80%> (+12.31%) ⬆️
evidence/reactor.go 65.93% <0%> (+0.5%) ⬆️
consensus/reactor.go 72.35% <0%> (+1.8%) ⬆️

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

🍰 🌮 🍉

@xla
Copy link
Contributor

xla commented Oct 8, 2018

Still need to write tests

@ebuchman Does this mean this PR is WIP?

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, just needs a test case!

@ebuchman ebuchman changed the title cap evidence in block validation WIP: cap evidence in block validation Oct 9, 2018
@ebuchman ebuchman changed the title WIP: cap evidence in block validation [WIP]: cap evidence in block validation Oct 9, 2018
@xla xla changed the title [WIP]: cap evidence in block validation [WIP] cap evidence in block validation Oct 9, 2018
@xla xla changed the title [WIP] cap evidence in block validation [WIP] types: Cap evidence in block validation Oct 9, 2018
@ebuchman ebuchman changed the title [WIP] types: Cap evidence in block validation types: Cap evidence in block validation Oct 9, 2018
@ebuchman
Copy link
Contributor Author

ebuchman commented Oct 9, 2018

Updated the block validation tests - made them table-driven, opened an issue to write more of them (#2589), and added a test for the evidence cap :)

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

🥗 🔥 🌯

require.Error(t, err)
{"EvidenceHash", func(block *types.Block) { block.EvidenceHash = wrongHash }}, // wrong evidence hash
{"Proposer wrong", func(block *types.Block) { block.ProposerAddress = ed25519.GenPrivKey().PubKey().Address() }}, // wrong proposer
{"Proposer invalid", func(block *types.Block) { block.ProposerAddress = []byte("wrong size") }}, // invalid proposer
Copy link
Contributor

Choose a reason for hiding this comment

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

no need in // invalid proposer comment if we already have "Proposer invalid" as a test name

// Manipulation of any header field causes failure.
testCases := []struct {
name string
f func(block *types.Block)
Copy link
Contributor

Choose a reason for hiding this comment

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

malleateBlock func(*Block)

@ebuchman ebuchman merged commit 6ec52a9 into develop Oct 9, 2018
@ebuchman ebuchman deleted the bucky/evidence-cap branch October 9, 2018 17:31
melekes added a commit that referenced this pull request Oct 10, 2018
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.
```
ebuchman pushed a commit that referenced this pull request Oct 12, 2018
* require block.Time of the fist block to be genesis time

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.
```

* do not accept blocks with negative height

* update changelog and spec

* nanos precision for test genesis time

* Fix failing test (#2607)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants