make ConsensusParams.EvidenceParams.MaxAge time.Duration#2606
make ConsensusParams.EvidenceParams.MaxAge time.Duration#2606
ConsensusParams.EvidenceParams.MaxAge time.Duration#2606Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2606 +/- ##
===========================================
- Coverage 61.33% 61.26% -0.07%
===========================================
Files 203 204 +1
Lines 16781 16792 +11
===========================================
- Hits 10292 10287 -5
- Misses 5621 5634 +13
- Partials 868 871 +3
|
ConsensusParams.EvidenceParams.MaxAge time.DurationConsensusParams.EvidenceParams.MaxAge time.Duration
consensus/reactor.go
Outdated
|
|
||
| // GetLastBlockTime returns an atomic snapshot of the PeerRoundState's last | ||
| // block time used by the evidence reactor when sending evidence. | ||
| func (ps *PeerState) GetLastBlockTime() time.Time { |
There was a problem hiding this comment.
Its not clear to me why they need the last block time? Why doesn't knowledge of the previous block hash suffice, if you already have the block?
There was a problem hiding this comment.
if you already have the block?
what if we don't?
There was a problem hiding this comment.
But if they don't have the latest block, shouldn't I be tracking by block height which blocks to send to them?
consensus/reactor.go
Outdated
| if rs.Step == cstypes.RoundStepCommit { | ||
| csMsg = &CommitStepMessage{ | ||
| Height: rs.Height, | ||
| Time: rs.CommitTime, |
There was a problem hiding this comment.
Why did we need to add this?
There was a problem hiding this comment.
I was confused by this as well
evidence/pool.go
Outdated
| evpool.removeEvidence(height, maxAge, blockEvidenceMap) | ||
|
|
||
| evpool.removeEvidence( | ||
| height, |
There was a problem hiding this comment.
I think if we pass in the Header to MarkEvidenceAsCommitted instead of just the height (or maybe the timestamp, along with the height, without the full header), we can pass it in here too and avoid the need to get the LastBlockTime from the state ... then we don't need the Time field in those consensus msgs.
There was a problem hiding this comment.
True, but what about checkSendEvidenceMessage in evidenceReactor? where we decide whenever we should send an evidence to a peer based on its state.
There was a problem hiding this comment.
ok, what if we remove the check 3ab2107 ?
There was a problem hiding this comment.
another alternative is to pass blockstore and load block header?
maxAge := evR.evpool.State().ConsensusParams.EvidenceParams.MaxAge
// peerLastBlockTime := blockStore.LoadBlockMeta(peerHeight).Header.Time
if peerLastBlockTime.Sub(ev.Time()) > maxAge {
// evidence is too old, skip
// NOTE: if evidence is too old for an honest peer,
// then we're behind and either it already got committed or it never will!
evR.Logger.Info("Not sending peer old evidence", "peerHeight", peerHeight, "evHeight", evHeight, "maxAge", maxAge, "peer", peer)
return nil, false
}220d525 to
3ab2107
Compare
when deciding if we should send evidence to it
3ab2107 to
e9f30e1
Compare
|
Maybe we should hold off merging this until #2653 is resolved. |
* evidence: introduce time.Duration to evidence params - add time.duration to evidence - this pr is taking pr #2606 and updating it to use both time and height - closes #2565 Signed-off-by: Marko Baricevic <marbar3778@yahoo.com> * fix testing and genesis cfg in signer harness * remove debugging fmt * change maxageheight to maxagenumblocks, rename other things to block instead of height * further check of duration * check duration to not send peers outdated evidence * change some lines, onward and upward * refactor evidence package * add a changelog pending entry * make mockbadevidence have time and use it * add what could possibly be called a test case * remove mockbadevidence and mockgoodevidence in favor of mockevidence * add a comment for err that is returned * add a changelog for removal of good & bad evidence * add a test for adding evidence * fix test * add ev to types in testcase * Update evidence/pool_test.go Co-Authored-By: Anton Kaliaev <anton.kalyaev@gmail.com> * Update evidence/pool_test.go Co-Authored-By: Anton Kaliaev <anton.kalyaev@gmail.com> * fix tests * fix linting Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
Refs #2565
TODO
nonnullable