identities: refactor is_malicious check to read minimal amount of data #6346
Conversation
dshulyak
force-pushed
the
simplify-is-malicious
branch
from
811741f
to
0bcec1d
Compare
dshulyak
requested review from
fasmat,
poszu,
ivan4th and
acud
as code owners
|
bors try |
hare3/hare.go
Outdated
| err := h.db.WithTx(context.Background(), func(tx sql.Transaction) error { | ||
| return identities.SetMalicious( | ||
| h.db, equivocation.Messages[0].SmesherID, codec.MustEncode(proof), time.Now()) | ||
| }) | ||
| if err != nil { |
There was a problem hiding this comment.
Could you please explain why a transaction is used? Btw, the tx isn't used as h.db is passed to identities.SetMalicious() - is it on purpose?
There was a problem hiding this comment.
SetMalicious runs more than one statement, if they are executed without transaction then on failure the state will be inconsistent, e.g only one of the atxs will be marked as malicious
There was a problem hiding this comment.
OK makes sense, but in this case it would be great if SetMalicious enforced being run in a transaction by taking a sql.Transaction type instead of sql.Executor, wdyt? Such API would prevent mistakes like in this snippet.
activation/handler_v2.go
Outdated
| if atx.MarriageATX != nil { | ||
| receivedProof, err := identities.IsMarriageMalicious(tx, *atx.MarriageATX) | ||
| if err != nil { | ||
| return false, fmt.Errorf("checking if marriage ATX is malicious: %w", err) | ||
| } | ||
| if receivedProof != nil { | ||
| err = identities.SetMaliciousBecauseOfMarriage(tx, atx.SmesherID, *receivedProof) | ||
| if err != nil { | ||
| return false, fmt.Errorf("setting node as malicious because of marriage: %w", err) | ||
| } | ||
| return true, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this might not be sufficient. The marriage ATX (the one that marries IDs - it includes the marriage certificates) must also be marked as malicious when it is processed if any of the married IDs is already malicious. This was previously covered by matching by marriage ATX in the IsMalicious() func.
There was a problem hiding this comment.
so there are 2 things now:
- all identities with same marriage_atx that are present in db at the moment of SetMalicious are marked malicious
- if some identity is learned after SetMalicious and it has same marriage_atx we update it here as malicious
it doesn't cover the case you described?
There was a problem hiding this comment.
so the case is MarriageAtx is nil, so that it doesn't point to itself?
but instead it has some certificates, does it work if pick id of such atx as marriage_atx and run everything the same way?
although i don't see that covered in this query
SELECT 1 FROM identities
WHERE (
marriage_atx = (SELECT marriage_atx FROM identities WHERE pubkey = ?1 AND marriage_atx IS NOT NULL)
AND proof IS NOT NULL
)
OR (pubkey = ?1 AND marriage_atx IS NULL AND proof IS NOT NULL);
There was a problem hiding this comment.
i feel stupid there is even a failing test and i can't figure what is going on
There was a problem hiding this comment.
The problem is that an ATX that contains a marriage and is syntactically valid marks all identities in the marriage set as malicious if at least one identity in the set is already malicious.
I do have a fix for the problem but it requires calling IsMalicious on all identities in a marriage ATX before updating the marriage set (which I think is OK, it should be much faster then inserting the knowledge about the set anyway)
tryBuild failed: |
dshulyak
force-pushed
the
simplify-is-malicious
branch
from
20ea60c
to
d3f0825
Compare
There was a problem hiding this comment.
For now I believe this is good enough to merge. For the new malfeasance syncer we probably need to revisit the DB logic without making the performance of the ATX handler worse:
- When a peer asks for the set of identities that are malicious we should reconciliate on the full set of identities (not just one identity per marriage set), because nodes might not agree on who's part of a set and who is not
- Per marriage set we only send one proof to a peer asking together with a list of identities we consider to also be part of the marriage set the identity we proved malicious belongs to. This list might be incomplete in the view of the requesting node (it might now that additional identities belong to any marriage set).
- All proofs served must cover the full set the nodes reconciliated on.
Example: two nodes compare their local list of malicious identities and node A realizies it is missing proofs for identities 100 - 120 while node B realizes that it doesn't consider identities 200 - 210 malfeasant yet
Node A requests proof for identity 100 from node B, node b sends proof together with proof that identities 101 - 110 also belong to that set. Node A now has proof for 100 - 110 and continues with 111 until it has proofs for all identities that B claims to know to be malfeasant and A doesn't.
Node B then requests proof for identity 200, Node A serves a proof for identity 220 (which it already knows to be malfeasant) and proof that 200 belongs to the same marriage set, it just updates it local set and continues from there.
|
Superseded by #6378 |
part of #6326
in the current implementation IsMalicious has to read whole identities table twice whenever new atx is received.
even if it table relatively small (not GB), it has to scan whole table on every query, best case it is served from memory and then only wastes cpu, in worst case it goes to disk if total amount of memory is low.
changes: