Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix malicious identities getting rewards in ATXv2 #6732

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

fasmat
Copy link
Member

@fasmat fasmat commented Feb 14, 2025

Motivation

In tests malicious identities were still given rewards by other nodes. This PR fixes the malfeasance checks for ballots and in the process optimises various queries to be more efficient.

Description

ballots.Get queried not only the ballot from the ballots table, but also joined the identities table to set the ballot as malicious if the smesher that created it is a known malfeasant. Not only is this inefficient to do, especially on a database with millions of entries. It is also easy to overlook when identities is refactored (like for malfeasance 2 which stores malfeasance information in a new table). To address this I updated the queries for ballots to avoid joins:

  • ballots.Get: instead of joining with identities the function now first fetches the ballot from the DB and then queries both identities and malfeasance for an entry and sets the ballot to malicious if it finds one in at least one of the two tables. This now requires a transaction to be passed to ballots.Get instead of a sql.Executor. I added a TODO there to clean that up in the future: ideally we separate domain ballot types from DTO types and have a service that queries all tables necessary when fetching a ballot and malfeasance information is needed.
  • ballots.Layer and ballots.LayerNoMalicious: I combined both into a single function that does not fetch information about malfeasance when getting all ballots for a layer. The reason is that ballots.LayerNoMalicious didn't do this anyway before and all users of ballots.Layer (only GRPC v1 API) didn't even care about the malfeasance information.
  • ballots.FirstInEpoch and ballots.LastInEpoch: again I removed fetching malfeasance information for the ballot that is returned by these functions since all callers (only the proposal builder) don't care about the malfeasance information from the ballot.
  • atxs.GetIDWithMaxHeight: I updated the query to select the maximum epoch only once instead of once per row (since we do not have an index on the epoch column in the ATXs table). Additionally I replaced the JOIN with a WHERE NOT EXISTS clause that should be faster than checking for null after joining and applied this to both identities and malfeasance.
  • atxs.IterateForGrading: TODO
  • atxs.IterateAtxsWithMalfeasance: TODO
  • atxs.IterateAtxIdsWithMalfeasance: TODO

Test Plan

all existing tests pass, new tests were added to check that malfeasance information from malfeasance2 is now returned as well when fetching data from the DB.

TODO

  • Explain motivation or link existing issue(s)
  • Test changes and document test plan
  • Update documentation as needed
  • Update changelog as needed

@fasmat fasmat self-assigned this Feb 14, 2025
Copy link

codecov bot commented Feb 14, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
12444 1 12443 5
View the top 1 failed test(s) by shortest run time
github.com/spacemeshos/go-spacemesh/p2p/pubsub TestGossip
Stack Traces | 10s run time
=== RUN   TestGossip
    pubsub_test.go:43: 
        	Error Trace:	.../p2p/pubsub/pubsub_test.go:43
        	Error:      	Condition never satisfied
        	Test:       	TestGossip
--- FAIL: TestGossip (10.00s)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@fasmat fasmat force-pushed the fix-malicious-identities-getting-rewards branch from 2d7b249 to 157536f Compare February 14, 2025 19:02
@fasmat fasmat changed the title WiP: Fix malicious identities getting rewards in ATXv2 Fix malicious identities getting rewards in ATXv2 Feb 17, 2025
@fasmat fasmat marked this pull request as ready for review February 17, 2025 17:24
@fasmat fasmat force-pushed the fix-malicious-identities-getting-rewards branch from ffeb40d to bb9eb4f Compare February 18, 2025 09:03
@fasmat fasmat force-pushed the fix-malicious-identities-getting-rewards branch from bb9eb4f to 9a77e2d Compare February 22, 2025 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant