-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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: Rework revert_reason #9212
Conversation
@@ -585,22 +586,54 @@ defmodule Explorer.Chain.Transaction do | |||
process_hex_revert_reason(hex, transaction, options) | |||
end | |||
|
|||
@default_error_abi [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we already have any specific UI views for these standard solidity errors, or we can render them just in the same way as custom ones.
For panics, the following error code description should be displayed somewhere I guess - https://docs.soliditylang.org/en/v0.8.23/control-structures.html#panic-via-assert-and-error-via-require
"0x" <> gas_hex_without_prefix | ||
else | ||
"0x0" | ||
response = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's save a first trace here, as done in
InternalTransactions.run_insert_only(first_trace_params, %{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't put db insert here intentionally to not slow down API view by an extra db operation.
I also not a fan of current separated logic of saving first trace and remaining traces in different place. It needs some refactoring and internal tx fetcher should not drop the first trace if it's useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree with you. But what I see now, is that we added one more quite heavy RPC call and its result is not reused, and then it will most probably be duplicated. I don't think that db insertion will be slower than RPC call.
One more question. Shouldn't we check if first trace is already fetched and stored in DB by first_trace_on_demand
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's reused, since we save it in the revert_reason
column, we won't re-fetch traces if the revert_reason is already non-empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that Indexer.Fetcher.FirstTraceOnDemand
will request first trace again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but Indexer.Fetcher.FirstTraceOnDemand
is only triggered for raw-trace
API endpoints, which are not called very often I guess.
If my understanding is correct, then with proper future refactoring in internal tx import logic Indexer.Fetcher.FirstTraceOnDemand
and all of its associated function won't be needed at all. This code will also work as a fallback, since internal tx import will contain the main logic extracting revert reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right but...
b354729
to
2f64816
Compare
285fd97
to
7f5a8b6
Compare
7f5a8b6
to
fbddb0a
Compare
* master: (65 commits) fix: Add healthcheck endpoints for indexer-only setup (#10076) 6.6.0 fix: Rework revert_reason (#9212) fix: Eliminate from_address_hash == #{address_hash} clause for transactions query in case of smart-contracts (#9469) feat: Add optional retry of NFT metadata fetch in Indexer.Fetcher.Tok… (#10036) fix: Separate indexer setup (#10032) feat: Blueprint contracts support (#10058) chore: Update hackney pool size: add new fetchers accounting (#9941) fix: Disallow batched queries in GraphQL endpoint (#10050) feat: Clone with immutable arguments proxy pattern (#10039) fix: vyper contracts re-verificaiton (#10053) refactor: Refactor get_additional_sources/4 -> get_additional_sources/3 (#10046) chore: Bump credo from 1.7.5 to 1.7.6 (#10060) chore: Bump redix from 1.5.0 to 1.5.1 (#10059) chore: Bump ex_doc from 0.32.1 to 0.32.2 (#10061) fix: Fix Unknown UID bug at smart-contract verification (#9986) refactor: test database config (#9662) chore: remove `has_methods` from `/addresses` (#10051) feat: Improve retry NFT fetcher (#10027) chore: Add support of Blast-specific L1 OP withdrawal events (#10049) ...
Motivation
Multiple problems in the current logic handling
revert_reason
have been discovered:transactions.revert_reason
column type istext
, although it should bebytea
, this bloats the storage unnecessary, as hex values are stored as text.transactions.revert_reason
contain irrelevant error messages, that should not be stored in the database. Here are the results of theSELECT revert_reason, count(*) FROM transactions WHERE status = 0 AND revert_reason IS NOT NULL GROUP BY revert_reason
query on Goerli: revert_reasons_stat.csv. Values like these are clearly not valid revert reasons and were saved by mistake:err: insufficient funds for gas * price + value: ...
execution reverted
insufficient funds for gas * price + value: ...
we can't execute this request
eth_call
for obtaining revert reasons is not 100% robust, as it can't simulate the call in the middle of the block using the intermediary state.trace_*
methods should be used instead. This is also likely the reason how messages likeinsufficient funds for gas * price + value: ...
appeared in therevert_reason
column.Error(string)
andPanic(uint256)
) decoding does not properly work:revert_reason
fields in the database, due tocall_has_error_or_result
constraint. Fillingrevert_reasons
in internal transactions will potentially reduce number of on-demand rpc calls totrace_*
oreth_call
methods used for obtaining revert reasons.Changelog
Attempt to fix problems 3&4 from the above list. 1&2&5 might be fixed as well if deemed feasible.
Checklist for your Pull Request (PR)
CHANGELOG.md
with this PRmaster
in the Version column. Changes will be reflected in this table: https://docs.blockscout.com/for-developers/information-and-settings/env-variables.