evidence: dont send evidence to unsynced peers#1628
Merged
Conversation
melekes
reviewed
May 25, 2018
evidence/reactor.go
Outdated
| // TODO: Broadcast runs asynchronously, so this should wait on the successChan | ||
| // in another routine before marking to be proper. | ||
| // TODO: the broadcast here is just doing TrySend. | ||
| // We should make sure the send succeeds before marking broadcasted. |
Contributor
There was a problem hiding this comment.
maybe we should use Send instead of TrySend then?
evidence/reactor.go
Outdated
| // broadcast all pending evidence | ||
| msg := &EvidenceListMessage{evR.evpool.PendingEvidence()} | ||
| evR.Switch.Broadcast(EvidenceChannel, cdc.MustMarshalBinaryBare(msg)) | ||
| evR.broadcastEvidenceListMsg(msg) |
Contributor
There was a problem hiding this comment.
should we mark evidence as broadcasted here too?
ebuchman
commented
May 29, 2018
evidence/reactor.go
Outdated
| } | ||
|
|
||
| for _, peer := range evR.Switch.Peers().List() { | ||
| ps := peer.Get(types.PeerStateKey).(PeerState) |
Contributor
Author
There was a problem hiding this comment.
E[05-26|15:40:21.860] Stopping peer for error module=p2p peer="Peer{MConn{13.124.200.97:37036} 0d04322b0637d8bf81c94e263ae7ea056c922d0f in}" err="Error{recovered panic in MConnection (cause: interface conversion: interface {} is nil, not *consensus.PeerState)}"
8e1e2bd to
fcab7a4
Compare
This was referenced Jun 4, 2018
Closed
ebuchman
commented
Jun 5, 2018
evidence/pool.go
Outdated
| blockEvidenceMap := make(map[string]struct{}) | ||
| for _, ev := range evidence { | ||
| evpool.evidenceStore.MarkEvidenceAsCommitted(ev) | ||
| blockEvidenceMap[ev.String()] = struct{}{} |
Contributor
Author
There was a problem hiding this comment.
use ev.Hash instead
evidence/reactor.go
Outdated
| peerHeight := peerState.GetHeight() | ||
| if peerHeight < height || | ||
| peerHeight > height+maxAge { | ||
| time.Sleep(peerCatchupSleepIntervalMS * time.Millisecond) |
Contributor
Author
There was a problem hiding this comment.
only sleep if the peerHeight < height, not based on the maxAge
| return ps.height | ||
| } | ||
|
|
||
| func TestReactorSelectiveBroadcast(t *testing.T) { |
* only send evidence to peers that are synced enough to validate it all * closes #1624
d2c74fd to
e5f4bed
Compare
e5f4bed to
1b2e347
Compare
🍡
Codecov Report
@@ Coverage Diff @@
## develop #1628 +/- ##
==========================================
- Coverage 62.37% 62.28% -0.1%
==========================================
Files 122 123 +1
Lines 11073 11128 +55
==========================================
+ Hits 6907 6931 +24
- Misses 3551 3581 +30
- Partials 615 616 +1
|
firelizzard18
pushed a commit
to AccumulateNetwork/tendermint
that referenced
this pull request
Feb 1, 2024
…rmint#1628) * Introduces check for FIFO ordering during gossip limitation, which fails * Including some more tests. The `TestMempoolParallelCheckTx` one shows that receiving the same sequence of transactions from multiple sources could cause inversion in the mempool. * remove debug line * fix lint error * check the right reactor * some extra debugging * Rename test function and disable during CI * satisfy linter * Reverting the original test so it does not fail if FIFO is violated * Update mempool/reactor_test.go Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com> * ignoring specific lint error * Improve skip message --------- Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.