Fraud reporting improvements #1375
Conversation
dwasse
requested review from
trajan0x,
ChiTimesChi and
CryptoMaxPlanck
as code owners
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1375 +/- ##
===================================================
- Coverage 50.88921% 50.58048% -0.30873%
===================================================
Files 353 343 -10
Lines 24966 24118 -848
Branches 277 277
===================================================
- Hits 12705 12199 -506
+ Misses 10981 10716 -265
+ Partials 1280 1203 -77
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
github-actions
bot
added
M-contracts
trajan0x
force-pushed
the
feat/fraud-report-enhancement
branch
from
fe830b9 to
806df4e
Compare
github-actions
bot
removed
M-contracts
dwasse
force-pushed
the
feat/fraud-report-enhancement
branch
from
fed0213 to
13c6cff
Compare
…ttestationWithMetadata
WalkthroughThis pull request introduces significant changes to the agent management and state validation logic. It refactors existing functions, introduces new data structures, and enhances modularity. The changes also include improved documentation and testing instructions. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Files selected for processing (8)
- agents/agents/executor/executor_utils.go (1 hunks)
- agents/agents/guard/calls.go (1 hunks)
- agents/agents/guard/fraud.go (9 hunks)
- agents/agents/guard/fraud_test.go (1 hunks)
- agents/agents/guard/guard.go (1 hunks)
- agents/contracts/inbox/parser.go (2 hunks)
- agents/contracts/lightinbox/parser.go (3 hunks)
- agents/types/state_validation_data.go (1 hunks)
Files skipped from review due to trivial changes (1)
- agents/agents/guard/guard.go
Additional comments (Suppressed): 19
agents/agents/guard/fraud_test.go (2)
330-342: The variable
fraudAttestationhas been renamed toattestationData. Ensure that all references to this variable in the codebase have been updated accordingly.343-343:
agents/agents/executor/executor_utils.go (1)
- 68-70: The
AgentDomainmethod is now being called instead of accessing theAgentDomainfield directly. Ensure that all references to this field throughout the codebase have been updated to use the new method.- if snapshotMetadata.Snapshot == nil || snapshotMetadata.AgentDomain == 0 { + if snapshotMetadata.Snapshot == nil || snapshotMetadata.AgentDomain() == 0 {agents/contracts/lightinbox/parser.go (1)
- 13-16: The return type of
ParseAttestationAcceptedmethod has been changed from*types.FraudAttestationto*types.AttestationWithMetadata. Ensure that all calls to this function throughout the codebase have been updated to handle the new return type.agents/contracts/inbox/parser.go (1)
- 16-18: The return type of
ParseSnapshotAcceptedhas been changed from*types.FraudSnapshotto*types.SnapshotWithMetadata. Ensure that all calls to this function throughout the codebase have been updated to handle the new return type.agents/agents/guard/calls.go (3)
44-83: This function verifies a state on a given chain. However, there is no check for whether the state actually exists on the chain or not before verifying it. It would be better to add a check for the existence of the state on the chain before proceeding with the verification.
93-146: Similar to the previous comment, this function submits a state report to a given chain. But there is no check for whether the state actually exists on the chain or not before submitting the report. It would be better to add a check for the existence of the state on the chain before proceeding with the submission.
192-210: This function relays an Active agent status from Summit to a remote chain where the agent is unknown. However, there is no validation that the agent is indeed active on the Summit before relaying the status. It would be better to add a check to confirm the agent's status on the Summit before relaying it.
agents/types/state_validation_data.go (4)
1-24: The
StateValidationDatainterface is well defined and provides a clear contract for any type that needs to provide state validation data. This will make it easier to add new types of state validation data in the future.26-29: The
HasAttestationfunction is a good utility function that makes it easy to check if a givenStateValidationDatainstance corresponds to an attestation or not. However, it's important to ensure that this function is used correctly throughout the codebase to avoid potential issues.31-85: The
SnapshotWithMetadatastruct and its methods are well implemented. The use of a constructor function (NewSnapshotWithMetadata) ensures that all necessary fields are set before the struct can be used, which helps prevent potential bugs due to uninitialized fields. The error handling in the constructor function is also well done.87-147: Similar to
SnapshotWithMetadata, theAttestationWithMetadatastruct and its methods are well implemented. The constructor function (NewAttestationWithMetadata) ensures that all necessary fields are set before the struct can be used. The addition of theSetSnapshotPayloadmethod allows for the snapshot payload to be set separately, which could be useful in certain scenarios. However, it's important to ensure that this method is used correctly to avoid potential issues.agents/agents/guard/fraud.go (7)
19-30: The function
handleSnapshotAcceptedhas been refactored to call another functionhandleSnapshot. Ensure that the logic inhandleSnapshotis equivalent to the old code inhandleSnapshotAccepted.76-89: The function
handleAttestationAcceptedhas been refactored to call other functionshandleValidAttestationandhandleInvalidAttestation. Ensure that the logic in these new functions is equivalent to the old code inhandleAttestationAccepted.109-117: The function
handleValidAttestationhas been refactored to call another functionhandleSnapshot. Ensure that the logic inhandleSnapshotis equivalent to the old code inhandleValidAttestation.141-178: The function
handleSnapshotseems to have been significantly refactored. Ensure that the new logic correctly implements the intended functionality and does not introduce any regressions.198-217: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [182-217]
The function
handleInvalidAttestationseems to have been significantly refactored. Ensure that the new logic correctly implements the intended functionality and does not introduce any regressions.
232-235: The function
handleReceiptAcceptedhas been renamed fromhandleReceipt. Ensure that all calls to this function throughout the codebase have been updated to match the new name.396-398: The condition
if chainID == g.summitDomainIDhas been added. Ensure that this condition is correct and does not alter the intended functionality.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- agents/contracts/lightinbox/parser.go (2 hunks)
Additional comments (Suppressed): 1
agents/contracts/lightinbox/parser.go (1)
- 13-16: The return type of
ParseAttestationAcceptedmethod has been changed from*types.FraudAttestationto*types.AttestationWithMetadata. Ensure that all calls to this function throughout the codebase have been updated to handle the new return type.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (1)
- agents/agents/guard/calls.go (1 hunks)
Additional comments (Suppressed): 6
agents/agents/guard/calls.go (6)
16-19: The
agentStatusContractinterface is introduced to abstract the retrieval of agent status. This is a good practice as it allows for easier testing and decouples the implementation details from the usage.43-84: The
verifyStatefunction verifies a state on a given chain. It checks whether the agent that provided the snapshot is active on origin before submitting the transaction. This is a good practice as it ensures only valid agents can submit transactions.86-91: The
stateReportContractinterface is introduced to abstract the submission of state reports. This is a good practice as it allows for easier testing and decouples the implementation details from the usage.93-146: The
submitStateReportfunction submits a state report to a given chain. It also ensures that the agent that provided the snapshot is active on the agent's respective domain before submitting the transaction. This is a good practice as it ensures only valid agents can submit transactions.165-198: The
ensureAgentActivefunction checks if the given agent is in a slashable status (Active or Unstaking), and relays the agent status from Summit to the given chain if necessary. This is a good practice as it ensures only valid agents can submit transactions.200-218: The
relayAgentStatusfunction relays an Active agent status from Summit to a remote chain where the agent is unknown. It's a good practice to handle such cases, ensuring that the agent status is updated across chains.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (1)
- agents/agents/guard/fraud.go (9 hunks)
Additional comments (Suppressed): 7
agents/agents/guard/fraud.go (7)
15-30: The function
handleSnapshotAcceptedhas been refactored to separate the parsing of the snapshot and handling it into two different steps. This improves readability and maintainability of the code.32-49: The function
getStateReportChainsis introduced to determine which chains need to receive the state report. This is a good practice as it separates concerns and makes the code more modular.76-89: The function
handleAttestationAcceptedhas been refactored similar tohandleSnapshotAccepted. The changes improve readability and maintainability of the code.123-191: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [105-138]
The function
handleValidAttestationhas been refactored to separate the fetching of the attested snapshot and handling it into two different steps. This improves readability and maintainability of the code.
141-178: The function
handleSnapshothas been introduced to handle a snapshot by validating each state in the snapshot. If an invalid state is found, it initiates slashing and submits state reports on eligible chains. This is a good practice as it separates concerns and makes the code more modular.198-217: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [180-217]
The function
handleInvalidAttestationhas been refactored to separate the initiation of slashing for invalid attestation and submitting a fraud report into two different steps. This improves readability and maintainability of the code.
- 229-235: The function
handleReceiptAcceptedhas been renamed fromhandleReceipt. This change improves the clarity of the function's purpose.
| # Guard | ||
| The Guard is an agent responsible for verifying actions from other Agents in the optimistic messaging model. This includes polling for invalid states and attestations as well as submitting fraud reports. | ||
|
|
||
| ## Components | ||
| The Guard operates with four main components: | ||
| ### Run | ||
| `streamLogs` is the data-getter for the Guard. It works by instantiating a gRPC connection to Scribe, and puts logs in a channel for the origin and destination contracts on each chain in the config. From here, it verifies the logs' order since the order of logs are very important for merkle tree construction. | ||
| <br /> Additionally, if the Guard unexpectedly stops in the middle of streaming logs, it will use the current database state to reconstruct the tree up to where the last log was, then continue to use gRPC. | ||
| <br /> <br > `receiveLogs` is the data-processor for the Guard. It works by taking the logs streamed from `streamLogs` and parsing the logs into either a `Message` on the `Origin.sol` contract, or a `Attestation` on the `Destination.sol` contract. It then stores the data into the Guard's database and builds the tree accordingly. | ||
| <br /> <br > `loadOriginLatestStates` polls Origin states and caches them in order to make the latest data available. | ||
| <br /> <br > `submitLatestSnapshot` fetches the latest snapshot from Origin and submits it on Summit. | ||
| <br /> <br > `updateAgentStatuses` polls the database for `RelayableAgentStatus` entries and calls `updateAgentStatus()` once a sufficient agent root is passed to the given remote chain. | ||
|
|
||
| ### Fraud Reporting | ||
| The fraud reporting logic can be found in `fraud.go`, which consists mostly of handlers for various logs. The two major handlers are `handleSnapshotAccepted` and `handleAttestationAccepted`, both of which verify states corresponding to the incoming snapshot/attestation, initiate slashing if applicable, and submit state reports to eligible chains. | ||
|
|
||
| ## Usage | ||
|
|
||
| Navigate to `sanguine/agents/agents/guard/main` and run the following command to start the Guard: | ||
|
|
||
| ```bash | ||
| $ go run main.go | ||
| ``` | ||
| Then the Guard command line will be exposed. The Guard requires a gRPC connection to a Scribe instance to stream logs. This can be done with either a remote Scribe or an embedded Scribe. | ||
|
|
||
| For more information on how to interact with Scribe see the Executor README. | ||
|
|
||
| ## Directory Structure | ||
|
|
||
| <pre> | ||
| Guard | ||
| ├── <a href="./api">main</a>: API server | ||
| ├── <a href="./cmd">cmd</a>: CLI commands | ||
| ├── <a href="./db">db</a>: Database interface | ||
| │ └── <a href="db/sql">sql</a>: Database writer, reader, and migrations | ||
| ├── <a href="./main">main</a>: CLI entrypoint | ||
| </pre> |
There was a problem hiding this comment.
The README is well written and provides a clear overview of the Guard component, its main functions, usage instructions, and directory structure. It would be helpful to include a brief explanation of the new data structures SnapshotWithMetadata and AttestationWithMetadata that have been introduced in this PR.
| ### Run | ||
| `streamLogs` is the data-getter for the Guard. It works by instantiating a gRPC connection to Scribe, and puts logs in a channel for the origin and destination contracts on each chain in the config. From here, it verifies the logs' order since the order of logs are very important for merkle tree construction. | ||
| <br /> Additionally, if the Guard unexpectedly stops in the middle of streaming logs, it will use the current database state to reconstruct the tree up to where the last log was, then continue to use gRPC. | ||
| <br /> <br > `receiveLogs` is the data-processor for the Guard. It works by taking the logs streamed from `streamLogs` and parsing the logs into either a `Message` on the `Origin.sol` contract, or a `Attestation` on the `Destination.sol` contract. It then stores the data into the Guard's database and builds the tree accordingly. |
There was a problem hiding this comment.
Builds the tree accordingly is a bit confusing here, since Executor builds a real Merkle tree for submitting proofs, but the Guard does more of gets for tree states (at least now)
There was a problem hiding this comment.
ooh i like this file haha
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Description
Various improvements to the Guard's fraud reporting:
calls.goStateValidationDatainterface for providing args for functions that require snapshots or attestationsguardagentsREADMEMetadata
Summary by CodeRabbit
Guardstruct to improve modularity and readability. The changes include renaminghandleSnapshot,handleAttestation, andhandleReceipttohandleSnapshotAccepted,handleAttestationAccepted, andhandleReceiptAcceptedrespectively.SnapshotWithMetadataandAttestationWithMetadatatypes to provide additional metadata for snapshots and attestations.