do not mark self malicious #4981
Conversation
countvonzero
requested review from
dshulyak,
fasmat and
poszu
as code owners
Codecov Report
@@ Coverage Diff @@
## develop #4981 +/- ##
=========================================
- Coverage 77.1% 77.1% -0.1%
=========================================
Files 254 254
Lines 30141 30173 +32
=========================================
+ Hits 23253 23266 +13
- Misses 5384 5400 +16
- Partials 1504 1507 +3
... and 4 files with indirect coverage changes 📢 Have feedback on the report? Share it here. |
countvonzero
force-pushed
the
dont-mark-self-malicious
branch
from
93b9179 to
5c90fa8
Compare
| @@ -699,6 +699,61 @@ func TestHandler(t *testing.T) { | |||
| }) | |||
| } | |||
|
|
|||
| func TestHandlerOwnEquivocation(t *testing.T) { | |||
There was a problem hiding this comment.
sorry. i couldn't find a way to add it in TestHandler.
i didn't know how to advance to a round that the node is eligible again
There was a problem hiding this comment.
i think the way to go to introduce such test is to make initialization reusable, instead of copying it
| @@ -9,6 +9,18 @@ import ( | |||
| "github.com/spacemeshos/go-spacemesh/log" | |||
| ) | |||
|
|
|||
| var ownNodeID NodeID | |||
There was a problem hiding this comment.
i don't think that adding global public key is acceptable even as a mitigation.
beside that this code already can be removed next week
| @@ -286,7 +286,7 @@ func (h *Hare) Handler(ctx context.Context, peer p2p.Peer, buf []byte) error { | |||
| gossip, equivocation := session.OnInput(input) | |||
| h.log.Debug("after on message", log.ZShortStringer("hash", input.msgHash), zap.Bool("gossip", gossip)) | |||
| submitLatency.Observe(time.Since(start).Seconds()) | |||
| if equivocation != nil && !malicious { | |||
| if equivocation != nil && !malicious && msg.Sender != types.MinerNodeID() { | |||
There was a problem hiding this comment.
*Hare has reference to signer
|
i don't understand what it fixes? if node sent an equivocating message thats enough, it doesn't have to generate a proof for itself. the latter is certainly should not be done, but by itself it doesn't fix much. this code also doesn't exit with fatal. the solution in #4857 prevents some configuration mistakes, but not all |
| msg2.Sender = n.signer.NodeID() | ||
| msg2.Signature = n.signer.Sign(signing.HARE, msg2.ToMetadata().ToBytes()) | ||
|
|
||
| types.SetMinerNodeID(n.hare.signer.NodeID()) |
There was a problem hiding this comment.
this global state can't work with t.Parallel, but hare already has access to signer
|
but anyway, i think this is not the approach. it doesn't solve any actual problems, introduces global state and code that needs to be removed "tomorrow". the solutions that addresses some problems are:
|
Motivation
to ease adoption of testnet
Changes