fix: fix not append logs when tx was reverted

[graykode]

#1746.

What does this PR do?

Skip event logs on failed tx.

Reviewers

Main reviewers:

Codeowner reviewers:

[cla-bot]

We require contributors @graykode to read our Contributor License Agreement, please check the CLA document

[tclemos]

Hey @graykode thanks for your feedback on this.

I'll take a look into that to make sure we don't return the logs from reverted transactions when calling eth_getLogs.

The solution you propose fixes the problem, but solving it in the RPC layer will add unnecessary overhead to the State layer.

I'll be performing an investigation around this topic to make sure the behavior works as is in Ethereum and also want to try a different solution to reduce the overhead in the State layer.

[graykode]

@tclemos Cool I'll check it out too in Ethereum

[graykode]

@tclemos I have a question, https://github.com/0xPolygonHermez/zkevm-node/blob/develop/jsonrpc/endpoints_eth.go#L343 already uses context related to State. is this okay?

[tclemos]

@tclemos I have a question, https://github.com/0xPolygonHermez/zkevm-node/blob/develop/jsonrpc/endpoints_eth.go#L343 already uses context related to State. is this okay?

yep, the State is where everything is stored and the context is shared from the top layer until the bottom, so we can control the whole request.

[graykode]

@tclemos Then, isn't this also the overhead caused by using State?

[graykode]

Also I find bug that failed txs(reverted) are not mined. It was denied by rpc node.

[tclemos]

Also I find bug that failed txs(reverted) are not mined. It was denied by rpc node.

it was fixed here: #1744

[tclemos]

@tclemos Then, isn't this also the overhead caused by using State?

No, the overhead is calling the state for every log, we can filter it directly when reading the logs from the db for example, but we could also avoid storing these logs if they are not needed, I still need to investigate if they should affect the state somehow.

[arnaubennassar]

@cla-bot check

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!

[graykode]

@tclemos

I checked the code in go-ethereum and found that event logs are not recorded in a block when transactions fail. This causes issues when trying to debug failed transactions using tools like etherscan, which execute traceTransaction when displaying event logs.

Here are the reasons I found:

1. The makeLog function (in instructions.go) creates a log for each event. It records the log using Addlog (in instructions.go) if the transaction was not failed.
2. An opcode is run by the evm.interpreter.Run function (in evm.go).
3. However, the stateDB is rolled back to the snapshot before execution (in evm.go). This means that the logs created by makeLog are not recorded in the block if the transaction fails.

Therefore, I reverted before commit and added a filter where reciept.Status = 1 (txSucceeded) in pgstatestorage: c060e9b

[tclemos]

Yes, nice investigation.

The approach to filter logs in the query DB is correct when trying to avoid the overhead by calling the DB multiple times, but the change to the DB query is wrong.

Inner join is meant to join tables, so when adding the receipt table to the query, we should be able to join it by matching the tx hash, as we do when we join the table state.transaction to the state.log table, and then, we must add the receipt status condition to the where clause.

So, instead of having this:

INNER JOIN state.receipt r ON r.status = 1

we should have something like this:

INNER JOIN state.receipt r ON r.tx_hash = t.hash
...
WHERE ...
AND r.status = 1

[arnaubennassar]

@graykode This will be fixed in the executor (the actual EVM implementation), so this logs are not created in the first place.

We expect this changes to reach testnet in ~24 hours. Thanks for the feedback and the effort 💜

[graykode]

@tclemos Then, can I open new pr about fixing postgresql query. @arnaubennassar

[tclemos]

@graykode It's not needed, the problem is going to be solved in a step before, the current query will work and the data will be filtered when the tx is executed, not generating logs for reverted TXs.