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

(BIDS-2550) fixed contract invocation detection by adding metadata column #2618

Merged
merged 5 commits into from
Feb 6, 2024

Conversation

remoterami
Copy link
Collaborator

@remoterami remoterami commented Oct 18, 2023

🤖 Generated by Copilot at 4ba935d

This pull request adds a feature to detect and display contract invocation status for eth1 transactions. It adds a new field invokes_contract to the Transaction message and the transaction data in the bigtable. It also updates the handlers and the rpc code to use this field and show the contract status in the execution block and transaction pages.

rpc/erigon.go Outdated
@@ -283,6 +283,11 @@ func (client *ErigonClient) GetBlock(number int64, traceMode string) (*types.Eth
}

c.Transactions[trace.TransactionPosition].Itx = append(c.Transactions[trace.TransactionPosition].Itx, tracePb)
if len(trace.TraceAddress) == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rare comment by me on code: I don't know if we want to use Erigon specific API. If we ever consider changing the EL this might break stuff. If I interpret this incorrectly, just ignore me :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not touched, just using another field of the reply. We are already using an Erigon specific endpoint here, but there's a fallback to geth in place here. Both cases should have the same result.

Copy link
Collaborator

@Eisei24 Eisei24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@remoterami remoterami force-pushed the BIDS-2550/add_invokesContract_field branch 4 times, most recently from 569ed0e to b18aef3 Compare November 2, 2023 16:23
@remoterami remoterami changed the title (BIDS-2550) fixed contract invocation detection by using tracer info (BIDS-2550) fixed contract invocation detection by adding metadata column Nov 2, 2023
@remoterami remoterami force-pushed the BIDS-2550/add_invokesContract_field branch 3 times, most recently from ba72047 to 6978df0 Compare November 7, 2023 10:59
@remoterami remoterami requested a review from Eisei24 November 8, 2023 11:51
Copy link
Contributor

@D13ce D13ce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@remoterami , as discussed in DMs, I have also reviewed this a bit and left some questions and minor suggestions. ptal

types/eth1.pb.go Outdated Show resolved Hide resolved
types/eth1.pb.go Show resolved Hide resolved
types/eth1.proto Outdated Show resolved Hide resolved
db/bigtable_eth1.go Outdated Show resolved Hide resolved
db/bigtable_eth1.go Outdated Show resolved Hide resolved
db/bigtable_eth1.go Outdated Show resolved Hide resolved
db/bigtable_eth1.go Outdated Show resolved Hide resolved
db/bigtable_eth1.go Outdated Show resolved Hide resolved
handlers/eth1Block.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@Eisei24 Eisei24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look at the comments from me and @D13ce.

db/bigtable_eth1.go Outdated Show resolved Hide resolved
db/bigtable_eth1.go Outdated Show resolved Hide resolved
db/bigtable_eth1.go Show resolved Hide resolved
db/bigtable_eth1.go Outdated Show resolved Hide resolved
@remoterami remoterami force-pushed the BIDS-2550/add_invokesContract_field branch 2 times, most recently from a277d8f to a076907 Compare December 6, 2023 12:30
db/bigtable_eth1.go Show resolved Hide resolved
db/bigtable_eth1.go Outdated Show resolved Hide resolved
db/bigtable_eth1.go Outdated Show resolved Hide resolved
handlers/eth1Block.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@Eisei24 Eisei24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look at my comments.
The behaviour in DeleteBlock is still the most important point of discussion.

db/bigtable_eth1.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@Eisei24 Eisei24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check my block keys comment.

@remoterami remoterami force-pushed the BIDS-2550/add_invokesContract_field branch from aa3c0ff to 8fd878e Compare December 18, 2023 11:35
Copy link
Collaborator

@Eisei24 Eisei24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

A small nitpick can still be adapted.

db/bigtable_eth1.go Outdated Show resolved Hide resolved
Eisei24
Eisei24 previously approved these changes Jan 4, 2024
Copy link
Collaborator

@Eisei24 Eisei24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, tested for tx and itx of address 0x70f5d9a2716aa99ad24b7461167e57906c7a6789 on Prater.
utACK for various edge cases and contracts that suicided.

@remoterami remoterami force-pushed the BIDS-2550/add_invokesContract_field branch 2 times, most recently from 227e58d to a4c537a Compare January 8, 2024 12:45
Copy link
Collaborator

@Eisei24 Eisei24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the new unique bigtable key for the contract state query times look fast enough.

There is just one frontend issue remaining.

db/bigtable_eth1.go Outdated Show resolved Hide resolved
db/bigtable_eth1.go Show resolved Hide resolved
@remoterami remoterami force-pushed the BIDS-2550/add_invokesContract_field branch from 2f721cd to 3fc4081 Compare January 17, 2024 11:42
Eisei24
Eisei24 previously approved these changes Jan 17, 2024
Copy link
Collaborator

@Eisei24 Eisei24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

(BIDS-2550) use field for blob transactions

(BIDS-2550) save contract updates to bigtable

(BIDS-2550) removed metadataUpdates indirection; added destruction detection

(BIDS-2550) support for internal transactions
(BIDS-2550) handle contract destruction, show correct data

(BIDS-2550) detect failed transactions
(BIDS-2550) applied (most) suggestions

(BIDS-2550) remove contract state updates on forks

this approach might be a bit naive though, as bigtable probably can't filter by time efficiently
(BIDS-2550) applied feedback

(BIDS-2550) fixed invalid bt blockKey timestamps

(BIDS-2550) added bigtable transform
(BIDS-2550) correct variable naming

(BIDS-2550) added new transform to list of all

(BIDS-2550) fixed ts search

(BIDS-2550) fixed failed itx detection, fixed update sorting, variable naming

(BIDS-2550) fixed bigtable rowrange

(BIDS-2550) using distinct contract update bt key

(BIDS-2550) add column filter to ClearByPrefix misc command
(BIDS-2550) delete keys output clarified

(BIDS-2550) ui issue fixed, bit assumptions adjusted
(BIDS-2550) fixed columns filter

(BIDS-2550) disabled column filter
@remoterami remoterami force-pushed the BIDS-2550/add_invokesContract_field branch from 09c74c9 to b797b01 Compare February 5, 2024 16:21
Copy link
Collaborator

@Eisei24 Eisei24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a minor issue for this BIDS itself.

However as discussed we have an issue with the "itx for a tx" page, e.g.
https://prater.beaconcha.in/tx/0xebc8edfcd300271d458da454b3d63e212031e8d860c73322fceffffb988525f4#internal-txns
https://goerli.etherscan.io/tx/0xebc8edfcd300271d458da454b3d63e212031e8d860c73322fceffffb988525f4#internal
where the "Type Trace Address" column is empty and the Advanced mode does nothing.

Please fix this here or leave a comment on how you want to handle this bug.

rpc/erigon.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@Eisei24 Eisei24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the issue, please fix.

db/bigtable_eth1.go Show resolved Hide resolved
Copy link
Collaborator

@Eisei24 Eisei24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@remoterami remoterami merged commit ed188e4 into master Feb 6, 2024
3 checks passed
@remoterami remoterami deleted the BIDS-2550/add_invokesContract_field branch February 6, 2024 11:20
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.

5 participants