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

Fix deposit receipt nonce #149

Merged

Conversation

jyellick
Copy link

The receipts stored in the DB do not contain the nonce for the deposit transactions. This change triggers the receipt retrieval to go through the more full path which includes the deposit nonce.

Additionally, it fixes the pending RPC block number to correspond to the latest block number as is the behavior in op-geth.

@ImTei ImTei self-requested a review March 14, 2024 07:13
@ImTei
Copy link
Member

ImTei commented Mar 14, 2024

@jyellick Can you provide me more detailed context about the change, with an example of RPC response if possible?

@jyellick
Copy link
Author

For the RPC please see https://github.com/ethereum-optimism/optimism/blob/abfc1e1f37a89405bacd08a3bb6363250d3f68f5/op-e2e/system_test.go#L1518-L1558

For a bit more history, we forked Erigon to use as the sequencer for our network, implementing the Bedrock protocol. As part of this, we worked through getting the op-e2e tests passing. Eventually, we found this fork being maintained as well and we're now syncing some of the changes here to our repo, and trying to push back some the changes from our repo here.

So please expect quite a few more of these in the near future. All of them are backed by the op-e2e tests.

@ImTei
Copy link
Member

ImTei commented Mar 19, 2024

@jyellick, the change for pending blocks looks to make sense. but I wanna ask you the example of RPC response for deposit nonce issue, because I tested with op-erigon and it shows the correct nonce for deposit txs.

Fixes the pending RPC block number to correspond to the latest block
number as is the behavior in op-geth.

See:

https://github.com/ethereum-optimism/optimism/blob/abfc1e1f37a89405bacd08a3bb6363250d3f68f5/op-e2e/system_test.go#L1518-L1558
@jyellick jyellick force-pushed the jyellick/fix-deposit-receipt-nonce branch from 54958de to 6f85a6f Compare March 22, 2024 17:50
@jyellick
Copy link
Author

Sorry for the long delay -- I ended up needing to write a test in the e2e to convince myself that the receipt changes were not needed. Looks like they are indeed superfluous and a holdover from an earlier version that is no longer needed. I've removed them from this PR.

@ImTei
Copy link
Member

ImTei commented Mar 27, 2024

@jyellick Thank you for confirming!

@ImTei ImTei merged commit ab1411d into testinprod-io:op-erigon Mar 27, 2024
4 of 5 checks passed
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.

None yet

2 participants