track evidence, include in block by ebuchman · Pull Request #592 · tendermint/tendermint

ebuchman · 2017-07-25T15:58:14Z

odeke-em · 2017-09-14T22:03:48Z

consensus/state.go

-			// TODO: track evidence for inclusion in a block
+			cs.Logger.Error("Found conflicting vote. Recording evidence in the RoundState", "height", vote.Height, "round", vote.Round, "type", vote.Type, "valAddr", vote.ValidatorAddress, "valIndex", vote.ValidatorIndex)

+			// TODO: ensure we haven't seen this evidence already !


Wouldn't that be the case already since we've already reported a conflicting vote and below are adding voteErr.DuplicateVoteEvidence? Or are you saying that before adding new evidence, we should check for the presence of DuplicateVoteEvidence?

It's possible we already saw this particular evidence (ie. from the same validator) and added it to the cs.Evidence.

melekes · 2017-09-19T19:34:37Z

types/evidence.go

+	if !bytes.Equal(dve.VoteA.ValidatorAddress, dve.VoteB.ValidatorAddress) {
+		return fmt.Errorf("DuplicateVoteEvidence Error: Validator addresses do not match. Got %X and %X", dve.VoteA.ValidatorAddress, dve.VoteB.ValidatorAddress)
+	}
+	// XXX: Should we enforce index is the same ?


should we? checking the address should be enough. although, validator set is sorted, so it does not matter

I think you're right, and we don't need the index.

melekes · 2017-09-19T19:35:00Z

Looking good so far )

ebuchman · 2017-11-08T01:49:46Z

types/evidence.go

+	valIdx, val := valset.GetByAddress(addr)
+	if val == nil {
+		return fmt.Errorf("Address %X was not a validator at height %d", addr, height)
+	} else if idx != valIdx {


maybe we still want to punish even if indexes are wrong?!

ebuchman · 2017-11-20T04:36:06Z

Only thing left to do is add the Evidence to BeginBlock and add some simple tests in the state package.

But this is ready for more serious review.

adrianbrink · 2017-11-20T11:11:31Z

evidence/pool.go

+}
+
+// EvidenceChan returns an unbuffered channel on which new evidence can be received.
+func (evpool *EvidencePool) EvidenceChan() chan types.Evidence {


How about encoding the direction of the channel here.
func (evpool *EvidencePool) EvidenceChan() <-chan types.Evidence

https://gobyexample.com/channel-directions

adrianbrink · 2017-11-20T11:16:14Z

evidence/store.go

+	// reverse the order so highest priority is first
+	l := store.ListEvidence(baseKeyOutqueue)
+	l2 := make([]types.Evidence, len(l))
+	for i, _ := range l {


The underscore can be omitted here.

adrianbrink · 2017-11-20T11:18:25Z

evidence/store.go

+	return ei
+}
+
+// AddNewEvidence adds the given evidence to the database.


Extra doc comment: Returns false if the evidence is already in the database.

petabytestorage · 2017-11-20T17:24:32Z

evidence/store.go

+	if len(val) == 0 {
+		return nil
+	}
+	ei := new(EvidenceInfo)


there are at least two cases of var ei EvidenceInfo and &ei elsewhere however this is the only case of ei := new(EvidenceInfo) in this PR. since the variable is named the same maybe it is easier reading to convert this last one to the var form also for parallel consistency.

The difference here is we return the pointer. But sure I'll be consistent and just return &ei

melekes · 2017-11-20T19:45:21Z

evidence/store.go

+
+	// add it to the store
+	key := keyOutqueue(evidence, priority)
+	store.db.Set(key, eiBytes)


I wonder if the same problem applies here If we crash after committed but before removing/updating (i.e. if one of the set op crashes)

These operations seem idempotent... see my other comment on idempotency

ebuchman · 2017-11-22T20:27:39Z

Also needs a reactor_test.go which I forgot to check in and just accidentally deleted. Awesome :). It's mostly just a copy of mempool/reactor_test.go though so not that much work

codecov-io · 2017-11-30T07:16:16Z

Codecov Report

Merging #592 into develop will decrease coverage by 0.07%.
The diff coverage is 62.21%.

@@             Coverage Diff             @@
##           develop     #592      +/-   ##
===========================================
- Coverage    60.12%   60.05%   -0.08%     
===========================================
  Files          110      114       +4     
  Lines        10273    10596     +323     
===========================================
+ Hits          6177     6363     +186     
- Misses        3535     3658     +123     
- Partials       561      575      +14

jaekwon · 2017-11-30T14:31:01Z

evidence/pool.go

+	// XXX: is this thread safe ?
+	priority, err := evpool.state.VerifyEvidence(evidence)
+	if err != nil {
+		return err


// TODO if err is that we can't check it before we pruned, then ignore.
// TODO if err is that evidence is bad, return (and upstream mark peer as bad)

jaekwon · 2017-11-30T14:32:30Z

evidence/pool.go

+// Blocks on the EvidenceChan.
+func (evpool *EvidencePool) AddEvidence(evidence types.Evidence) (err error) {
+
+	// XXX: is this thread safe ?


State is not thread safe... it's actually a bit of a mess.
(e.g. we should probably move state.LoadValidators out into a global function and keep state a singleton snapshot). Is AddEvidence thread safe?

Ok. This is actually broken because this state never gets updated. The goal with evidence pool was to be thread safe thanks to the threadsafety of the underlying db. But with needing to update the state, we probably need something more

jaekwon · 2017-11-30T14:38:24Z

evidence/pool.go

+
+	evpool.logger.Info("Verified new evidence of byzantine behaviour", "evidence", evidence)
+
+	evpool.evidenceChan <- evidence


NOTE: how do you know that evidenceChan isn't closed? (since it would panic)...
answer: because we don't close it. But that should be OK because channels are garbage collected as long as there are no hanging goroutines waiting on them.

jaekwon · 2017-11-30T14:39:41Z

evidence/reactor.go

+func (evR *EvidenceReactor) AddPeer(peer p2p.Peer) {
+	// send the peer our high-priority evidence.
+	// the rest will be sent by the broadcastRoutine
+	evidence := evR.evpool.PriorityEvidence()


Why a single high one?

oh it's a list. that wasn't clear.

jaekwon · 2017-11-30T14:40:57Z

evidence/reactor.go

+}
+
+// broadcast new evidence to all peers.
+// broadcasts must be non-blocking so routine is always available to read off EvidenceChan.


capitalization please :)

jaekwon · 2017-11-30T14:45:01Z

evidence/store.go

+	- Once committed, atomically remove from pending and update lookup.
+		- TODO: If we crash after committed but before removing/updating,
+			we'll be stuck broadcasting evidence we never know we committed.
+			so either share the state db and atomically MarkCommitted


Doesn't have to be atomic, but if we make updating this evidence store happen first before (or during) state db commit, and make sure the operation on the evidence store is idempotent, then that works too.

(would have to comment CONTRACT: needs to be idempotent in that case)

jaekwon · 2017-11-30T14:45:47Z

evidence/store.go

+}
+
+// big endian padded hex
+func be(h int) string {


jaekwon · 2017-11-30T14:48:13Z

evidence/store.go

+// It is wrapped by PriorityEvidence and PendingEvidence for convenience.
+func (store *EvidenceStore) ListEvidence(prefixKey string) (evidence []types.Evidence) {
+	iter := store.db.IteratorPrefix([]byte(prefixKey))
+	for iter.Next() {


I think you're skipping the first item by calling Next().

See documentation for tmlibs/db/Iterator:

Usage: for itr.Seek(mykey); itr.Valid(); itr.Next() { k, v := itr.Key(); itr.Value() .... }

In this case for ;itr.Valid(); itr.Next() {

but how itr.Valid() changes this skipping the first item by calling Next()? if calling Next() progresses itr, then we must do something like

iter := store.db.IteratorPrefix([]byte(prefixKey)) // handle first item iter.Key() iter.Value() for iter.Next()

I am confused

and there is no docs for iterator in tmlibs https://github.com/tendermint/tmlibs/tree/develop/db. none that I can find

jaekwon · 2017-11-30T14:51:16Z

evidence/store.go

+// It returns false if the evidence is already stored.
+func (store *EvidenceStore) AddNewEvidence(evidence types.Evidence, priority int) bool {
+	// check if we already have seen it
+	ei_ := store.GetEvidence(evidence.Height(), evidence.Hash())


What if a byzantine validator signed a million votes for the same height/round?

I think for simple vote evidence, we could search for whether we have evidence for a validator at a given height, and not worry about rounds or blockhashes etc. So if we already have evidence to slash a validator at a height, no need to keep collecting evidence. What do you think?

That sounds good. So we should change the lookup scheme to include the validator-address in the key so we can quickly check if theres already evidence for the validator at the height?

jaekwon · 2017-11-30T14:51:43Z

evidence/store.go

+	}
+	eiBytes := wire.BinaryBytes(ei)
+
+	// add it to the store


Seems like this function could be refactored out right underneath.

What do you mean?

jaekwon · 2017-11-30T14:55:17Z

evidence/store.go

+	ei.Committed = true
+
+	// TODO: we should use the state db and db.Sync in state.Save instead.
+	// Else, if we call this before state.Save, we may never mark committed evidence as committed.


We should just mark it as committed before state.Save. It'll get committed again if we restart.

There's a case during replay if the app is multiple blocks behind that we don't construct historical state and call state.ApplyBlock, we use sm.ExecCommitBlock instead, so eg. MarkEvidenceAsCommitted would never get called.

But now with historical validators etc we should be able to construct the historical state to use ApplyBlock after all.

jaekwon · 2017-11-30T14:56:53Z

state/state.go

 	return s.LastValidators, s.Validators
 }

+// VerifyEvidence verifies the evidence fully by checking it is internally


See my other comment about State... I think we should move these kinds of functions out of State and keep State a simple snapshot. LMK what you think.

Ok - I think that's a nice idea. All of the LoadXxx just need the db, but nothing else from the state.

This VerifyEvidence does require a bit more. Do we just pass in a state as an arg here?

Let's deal with it in a separate PR: #1010

ebuchman · 2017-12-27T20:22:11Z

Further issues moved to #1012

ebuchman force-pushed the evidence branch from 8337ebd to 47d99c3 Compare August 21, 2017 18:18

odeke-em reviewed Sep 14, 2017

View reviewed changes

melekes reviewed Sep 19, 2017

View reviewed changes

ebuchman force-pushed the evidence branch from 096866e to f0ea64f Compare November 2, 2017 05:11

ebuchman commented Nov 8, 2017

View reviewed changes

ebuchman force-pushed the evidence branch from df532ec to 9d44186 Compare November 18, 2017 23:58

ebuchman changed the title ~~WIP: track evidence, include in block~~ track evidence, include in block Nov 20, 2017

adrianbrink reviewed Nov 20, 2017

View reviewed changes

petabytestorage reviewed Nov 20, 2017

View reviewed changes

melekes reviewed Nov 20, 2017

View reviewed changes

ebuchman force-pushed the evidence branch from 02fa1ce to 582775a Compare November 30, 2017 06:43

jaekwon reviewed Nov 30, 2017

View reviewed changes

evidence/store.go Outdated

}

// big endian padded hex

func be(h int) string {

Copy link

Contributor

jaekwon Nov 30, 2017

Choose a reason for hiding this comment

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

bE()?

ebuchman reacted with thumbs up emoji

jaekwon reviewed Nov 30, 2017

View reviewed changes

ebuchman mentioned this pull request Dec 11, 2017

Summary of Header Changes #952

Closed

12 tasks

ebuchman added 20 commits December 26, 2017 20:25

introduce evidence store

10c43c9

check evidence is from validator; some cleanup

df3f4de

some comments and cleanup

f7731d3

cleanup evidence pkg. state.VerifyEvidence

6c4a0f9

evidence linked with consensus/node. compiles

7a18fa8

evidence store comments and cleanup

4854c23

types: evidence cleanup

3271634

state.ApplyBlock takes evpool and calls MarkEvidenceAsCommitted

869d873

evidence: more funcs in store.go

c7acdfa

state.VerifyEvidence enforces EvidenceParams.MaxAge

cc418e5

update consensus/test_data/many_blocks.cswal

1d021c2

evidence_pool.go -> pool.go. remove old test files

c2585b5

evidence: store tests and fixes

c13e93d

evidence: pool test

666ae24

evidence: reactor test

cfbedec

remove some TODOs

0f293bf

minor fixes from review

5904f6d

evidence: reactor test

014b0b9

remove unused var

b01b1e4

fixes from rebase

6a4fd46

ebuchman force-pushed the evidence branch from 5768b95 to 6a4fd46 Compare December 27, 2017 01:52

ebuchman added 2 commits December 27, 2017 01:27

address some comments from review

7d81a3f

types: better error messages for votes

d0e0ac5

ebuchman mentioned this pull request Dec 27, 2017

Evidence Issues #1012

Closed

6 tasks

ebuchman merged commit 53eb9ac into develop Dec 27, 2017

ebuchman deleted the evidence branch December 27, 2017 20:22

zramsay mentioned this pull request Dec 28, 2017

Tracking Evidence and Including in Blocks #569

Closed

17 tasks

This was referenced Oct 6, 2018

state: Use BlockExecutor everywhere #1029

Closed

Check evidence by validator address #2561

Closed


		evpool.logger.Info("Verified new evidence of byzantine behaviour", "evidence", evidence)

		evpool.evidenceChan <- evidence

Conversation

ebuchman commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebuchman commented Nov 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrianbrink Nov 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebuchman commented Nov 22, 2017

Uh oh!

codecov-io commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebuchman commented Jul 25, 2017 •

edited

Loading

adrianbrink Nov 20, 2017 •

edited

Loading

codecov-io commented Nov 30, 2017 •

edited

Loading

melekes Nov 30, 2017 •

edited

Loading