Fix faulty corpus transaction detection #1184
Conversation
WalkthroughThe recent updates across multiple files in the Echidna library refine transaction handling by distinguishing and treating Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- lib/Echidna/Campaign.hs (1 hunks)
Additional comments: 2
lib/Echidna/Campaign.hs (2)
- 71-73: The modification to filter out
NoCalltransactions before proceeding with fault detection based on destination addresses is a targeted and effective solution to the issue described in the PR objectives. This change ensures that transactions inherently lacking a relevant destination are not improperly flagged as faulty, aligning with the intended behavior forNoCalltransactions.- 71-73: While the current filter effectively excludes
NoCalltransactions from fault detection based on destination addresses, it's important to ensure that this change does not inadvertently exclude transactions that should be subject to other forms of fault detection. Confirm that all other relevant checks and balances within thereplayCorpusfunction and related transaction handling logic remain intact and effective.
| let maybeFaultyTx = | ||
| List.find (\tx -> LitAddr tx.dst `notElem` Map.keys vm.env.contracts) $ | ||
| List.filter (\case Tx { call = NoCall } -> False; _ -> True) txSeq |
There was a problem hiding this comment.
Given the PR's mention of a future, more comprehensive fix to the Tx type to eliminate unnecessary fields for NoCall transactions, it's crucial to ensure that this temporary solution is revisited and potentially revised to align with those future changes. This will help maintain the integrity and efficiency of the transaction handling process in the long term.
Would you like to create a follow-up task or issue to revisit this logic once the comprehensive Tx type changes are implemented?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- lib/Echidna/Exec.hs (1 hunks)
- lib/Echidna/Transaction.hs (1 hunks)
Additional comments: 4
lib/Echidna/Transaction.hs (2)
- 161-166: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [90-90]
The modification to exclude
loadContractforNoCalltransactions insetupTxaligns with the PR's objective to refine transaction handling by not setting up unnecessary contract loads forNoCalltransactions. This change is logically sound and improves performance by avoiding redundant operations.
- 161-166: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [90-90]
Ensure that the exclusion of
loadContractforNoCalltransactions does not inadvertently affect the VM's state in a way that could impact the execution of subsequent transactions. This is particularly relevant in scenarios where the state modified byresetStateand other operations insetupTxis expected to be further altered byloadContract.lib/Echidna/Exec.hs (2)
- 90-97: The addition of a conditional case to handle
NoCalltransactions by returning a default value inexecTxWithis a logical extension of the PR's objective to refine transaction handling. This ensures thatNoCalltransactions are processed efficiently without unnecessary execution steps, aligning with the goal of optimizing the transaction execution flow.- 90-97: While the handling of
NoCalltransactions with a default return value is appropriate, it's important to confirm that this approach does not bypass any critical gas accounting or event tracing mechanisms that might be expected to run even forNoCalltransactions. This is to ensure that the system's integrity and observability are maintained across all transaction types.
NoCalls form corpus shouldn't be checked for destination. Long term, fix theTxtype soNoCalldoesn't have unnecessary fields.Summary by CodeRabbit
execTxWithfunction to handleNoCallcases separately, affecting the return value based on the transaction's call type.setupTxfunction in transactions to exclude theloadContractcall for transactions withNoCall, impacting the setup logic for transactions.