Conversation
22d70ec to
341893a
Compare
| // down or removed peers, it will check if an evidence broadcasting goroutine | ||
| // exists and signal that it should exit. | ||
| // | ||
| // FIXME: The peer may be behind in which case it would simply ignore the |
| @@ -49,10 +49,9 @@ type Pool struct { | |||
| // NewPool creates an evidence pool. If using an existing evidence store, | |||
There was a problem hiding this comment.
This file diff contains linting updates only.
There was a problem hiding this comment.
Thanks for the heads-up! 😅
|
I've attempted to leave the existing reactor's semantics unchanged as much as possible, however, since we no longer have access to peers directly, only their IDs, we cannot know of their height or if they're caught up or not. Thus, the main change in semantics this PR introduces currently is that evidence will always be sent to a peer regardless if the recipient peer is able to accept/process that evidence or not. Whereas, the old semantics in this context would just simply not gossip the evidence at all. This results in the fact that we now send more messages over the wire. This yields the need for discussion around shared state, particularly around peers between reactors. However, this is a topic that should not block this PR and we can revisit it later. |
Codecov Report
@@ Coverage Diff @@
## master #5747 +/- ##
==========================================
+ Coverage 59.98% 60.20% +0.21%
==========================================
Files 263 263
Lines 23858 23938 +80
==========================================
+ Hits 14312 14411 +99
+ Misses 8021 8006 -15
+ Partials 1525 1521 -4
|
Co-authored-by: Callum Waters <cmwaters19@gmail.com>
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
bump @erikgrinaker |
p2p/key.go
Outdated
| type NodeID string | ||
|
|
||
| // NewNodeID returns a lowercased (normalized) NodeID. | ||
| func NewNodeID(nodeID string) NodeID { |
There was a problem hiding this comment.
This should call Validate() as well, to make sure it's actually valid, and Validate() should check that it's lowercased (since callers may not be using NewNodeID, e.g. if it's received via Protobuf).
We should possibly have a Normalize() method as well, which can be called on already-typed strings received from external sources.
There was a problem hiding this comment.
Why would this call Validate if it is doing the normalizing. If anything, we should just have NewNodeID and Normalize, where the former calls the latter. Also, we could have a Validate function too.
There was a problem hiding this comment.
Because it needs to e.g. check that it's actually a valid hex string, not just some random garbage.
NodeID can be embedded in structs, and these structs can be received via the network (e.g. from Protobuf handshakes), so we may want to have Validate() and/or Normalize() to handle this. I believe we also have some existing P2P code from the old stack that relies on Validate().
Ideally we should be able to enforce this as an invariant, but the Go type system doesn't allow this.
There was a problem hiding this comment.
Ok, yeah I think having NewNodeID ensure input is a valid hex-string and then lower-case should suffice. We'll have a validate function as well.
Description
Prepares the evidence reactor for the newly designed p2p changes per ADR 062.
ref: #5670