fix(halo/attest): delete empty attestations on double-sign detection

raehat commented 5 days ago

Issue: Attestations were created even if double signing was detected, allowing malicious validators to spam the database with invalid data.
Cause: Attestation creation was not rolled back if no valid signature was stored due to double sign detection.
Solution: Added logic to delete the attestation if it was created but no signatures were added, preventing the spam of invalid data in the DB as suggested by @corverroos in https://github.com/omni-network/omni/issues/2488#:~:text=Proposed%20Solution-,In%20addOne%2C%20delete%20the%20attestation%20if%20it%20was%20created%20but%20signatures%20were%20inserted.,-corverroos%20added%20the

issue: fixes #2488

corverroos commented 4 days ago

Hey @raehat, thanks for your PR, this is great!

Sorry, I didn't see this until now. I actually also pushed a PR for this earlier today. I actually prefer my implementation, as that avoid doing additional DB queries, which can be significant if the validator set if large. You are welcome to follow my example and then we can merge your PR and I'll abandon mine.

Let me know what you think

raehat commented 3 days ago

Hey @corverroos, I chose the db query approach because I wasn't sure of race conditions. I'll follow your example and push a commit then 👍

corverroos commented 3 days ago

Great work @arajasek approved 🚀

omni-network / omni

fix(halo/attest): delete empty attestations on double-sign detection #2524