Feat/block validation #2899

frrist · 2019-06-06T18:48:52Z

What

This PR implements block validation as defined in spec.
This PR must merge after #2897

Follow On:

Discussion around Should the SystemClock go on the Porcelain API? #2961 is required, once it is addressed the TODO's added in this PR will be removed and the code will be updated
When EC Upgrade 1: ticket arrays #2222 lands the TODO relating to it will be replaced with the correct code for block validation involving tickets.

ZenGround0 · 2019-06-06T21:51:32Z

consensus/block_validation.go

+	// check that child is appropriately delayed from its parents including
+	// null blocks.
+	// TODO replace check on height when #2222 lands
+	limit := uint64(pmin) + uint64(dv.BlockTime().Seconds())*uint64(uint64(child.Height)-ph)


The outer cast to uint64 appears unnecessary.

ZenGround0 · 2019-06-06T21:54:17Z

plumbing/clock/clock.go

@@ -40,3 +41,7 @@ func NewConfiguredBlockClock(blockTime time.Duration) *DefaultBlockClock {
 func (bc *DefaultBlockClock) BlockTime() time.Duration {
 	return bc.blockTime
 }
+
+func (bc DefaultBlockClock) EpochSeconds() int64 {


nit: this name seems a little unclear (could just be me). Why not Now?

Missing function comment

Called it EpochSeconds due to this comment: #2882 (comment)

will add a comment as well

oh I see. One reason I don't like EpochSeconds is that epoch is a term with a precise meaning for the consensus protocol and it appears to be used in a different way here. This might be something @anorth didn't think of.

ZenGround0 · 2019-06-06T21:56:14Z

consensus/block_validation.go

+	if uint64(blk.Height) < 0 {
+		return fmt.Errorf("block has negative height")
+	}
+	// TODO vlidate block signature: 1054


super nit: "validate"

types/tipset.go

codecov-io · 2019-06-07T23:38:09Z

Codecov Report

Merging #2899 into master will decrease coverage by <1%.
The diff coverage is 74%.

@@          Coverage Diff           @@
##           master   #2899   +/-   ##
======================================
- Coverage      43%     43%   -1%     
======================================
  Files         213     213           
  Lines       12680   12703   +23     
======================================
- Hits         5560    5528   -32     
- Misses       6261    6320   +59     
+ Partials      859     855    -4

Impacted Files	Coverage Δ
mining/block_generate.go	`71% <100%> (ø)`	⬆️
node/node.go	`59% <100%> (ø)`	⬆️
types/tipset.go	`80% <50%> (-4%)`	⬇️
consensus/block_validation.go	`86% <80%> (+86%)`	⬆️
proofs/sectorbuilder/testing/builder.go	`0% <0%> (-88%)`	⬇️
proofs/sectorbuilder/testing/harness.go	`0% <0%> (-58%)`	⬇️
protocol/hello/hello.go	`77% <0%> (-7%)`	⬇️

anorth · 2019-06-12T01:45:47Z

FYI I'm not looking at this yet because it's marked draft

ingar · 2019-06-14T00:11:12Z

clock/clock.go

+
+// MockClock returns a mocked clock implementation that may be manually
+// set for testing things related to time.
+type MockClock struct {


Should this belong in its own file, and perhaps in its own package that has to do with testing/test helpers?

ingar · 2019-06-14T00:11:51Z

clock/clock.go

+
+// Now returns the current value of the MockClock.
+func (mc *MockClock) Now() time.Time {
+	mc.nowMu.Lock()


I am not quite sure what race condition this mutex is guarding against. Is there one?

Yup this was a mistake, removed.

ingar · 2019-06-14T00:15:50Z

mining/block_generate.go

@@ -86,6 +86,7 @@ func (w *DefaultWorker) Generate(ctx context.Context,
 		Proof:           proof,
 		StateRoot:       newStateTreeCid,
 		Ticket:          ticket,
+		Timestamp:       types.Uint64(time.Now().Unix()),


Isn't the point of the clock module to encapsulate access to time.Now()? Coming later?

Oh, I see your "What's next" in the description. 👍

ingar · 2019-06-14T00:17:13Z

types/tipset.go

+		return 0, errUndefTipSet
+	}
+	min := ts.blocks[0].Timestamp
+	for i := 0; i < len(ts.blocks[0:]); i++ {


Don't quite understand the slice here..

Was this intended to be i := 1; i < len(ts.blocks); i++?
or for block := range ts.blocks[1:]?

Whoops, yes thanks!

anorth

Minor blocking comments, but since we're planning to roll a new release for this change, I think we should put the effort into solid testing, so I'd really like those comments addressed too. Let's chat briefly to decide a plan.

clock/clock.go

consensus/block_validation.go

consensus/block_validation_test.go

testhelpers/clock.go

types/tipset_test.go

anorth · 2019-06-19T01:51:43Z

consensus/block_validation.go

+	// TODO replace check on height when #2222 lands
+	limit := uint64(pmin) + uint64(dv.BlockTime().Seconds())*(uint64(child.Height)-ph)
+	if uint64(child.Timestamp) < limit {
+		return fmt.Errorf("block %s with timestamp %d generated too far in future", child.Cid().String(), child.Timestamp)


"... too far past parent, expected timestamp < %s", ... limit)

* add MinTimestamp to tipset and test * add clock package and block validation

frrist requested review from anorth and ZenGround0 June 6, 2019 18:49

frrist force-pushed the feat/block-validation branch from db9a4b2 to 0b2bedc Compare June 6, 2019 18:56

ZenGround0 approved these changes Jun 6, 2019

View reviewed changes

frrist self-assigned this Jun 11, 2019

frrist mentioned this pull request Jun 11, 2019

move BlockTime to plumbing add BlockClock interface #2894

Closed

frrist force-pushed the feat/timestamp-block branch 3 times, most recently from b2caa77 to 3a5fe58 Compare June 12, 2019 17:25

frrist force-pushed the feat/block-validation branch from 4bff05a to f55889f Compare June 12, 2019 19:02

frrist requested a review from ingar June 12, 2019 19:05

frrist marked this pull request as ready for review June 12, 2019 19:05

frrist force-pushed the feat/timestamp-block branch 2 times, most recently from ac0d78c to acbcc58 Compare June 12, 2019 20:55

frrist force-pushed the feat/block-validation branch from f55889f to 867a70f Compare June 12, 2019 20:58

frrist force-pushed the feat/timestamp-block branch from acbcc58 to da4344d Compare June 12, 2019 21:13

frrist force-pushed the feat/block-validation branch from 867a70f to 7444af3 Compare June 12, 2019 22:48

frrist changed the base branch from feat/timestamp-block to master June 12, 2019 22:50

frrist force-pushed the feat/block-validation branch from 7444af3 to 3f39519 Compare June 12, 2019 22:57

ingar reviewed Jun 14, 2019

View reviewed changes

frrist mentioned this pull request Jun 18, 2019

Patch/block validation 0.2.3 #2958

Merged

frrist force-pushed the feat/block-validation branch from 106e692 to 7fe4e0f Compare June 18, 2019 19:02

anorth reviewed Jun 18, 2019

View reviewed changes

frrist mentioned this pull request Jun 19, 2019

Should the SystemClock go on the Porcelain API? #2961

Closed

frrist added 4 commits June 18, 2019 18:30

add MinTimestamp to tipset and test

af7908d

add clock package and block validation

7d4fea5

address feedback

c3b7b68

move mockclock to testhelpers

928fa43

frrist added 6 commits June 18, 2019 18:30

rename blockclock to systemclock

57d2e13

block validation errors have block cid and time

6f273ab

fix block validation tests

63543a3

rename mockclock to fakesystemclock

aca48a7

fix tipset tests

a699842

add TODO about Clock interface

0817d95

frrist force-pushed the feat/block-validation branch from 6a716c3 to 0817d95 Compare June 19, 2019 01:31

anorth approved these changes Jun 19, 2019

View reviewed changes

better error message for timestamp

b0b5019

frrist merged commit 989cb18 into master Jun 19, 2019

frrist added a commit that referenced this pull request Jun 19, 2019

Feat/block validation (#2899)

5399815

* add MinTimestamp to tipset and test * add clock package and block validation

frrist mentioned this pull request Jun 25, 2019

[bug] implement block delay validation (quick fix against the attack on 0.2.2) #2834

Closed

ZenGround0 deleted the feat/block-validation branch October 29, 2019 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/block validation #2899

Feat/block validation #2899

frrist commented Jun 6, 2019 •

edited by mishmosh

Loading

ZenGround0 Jun 6, 2019

ZenGround0 Jun 6, 2019

ZenGround0 Jun 6, 2019

frrist Jun 6, 2019

frrist Jun 6, 2019

ZenGround0 Jun 10, 2019

ZenGround0 Jun 6, 2019

codecov-io commented Jun 7, 2019 •

edited

Loading

anorth commented Jun 12, 2019

ingar Jun 14, 2019

ingar Jun 14, 2019

frrist Jun 18, 2019

ingar Jun 14, 2019

ingar Jun 14, 2019

ingar Jun 14, 2019 •

edited

Loading

frrist Jun 18, 2019

anorth left a comment

anorth Jun 19, 2019

Feat/block validation #2899

Feat/block validation #2899

Conversation

frrist commented Jun 6, 2019 • edited by mishmosh Loading

What

Follow On:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 7, 2019 • edited Loading

Codecov Report

anorth commented Jun 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingar Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frrist commented Jun 6, 2019 •

edited by mishmosh

Loading

codecov-io commented Jun 7, 2019 •

edited

Loading

ingar Jun 14, 2019 •

edited

Loading