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

feat: support "safe" param for eth_getBlockByNumber for 30 blocks #12110

Merged
merged 6 commits into from
Jun 21, 2024

Conversation

snissn
Copy link
Contributor

@snissn snissn commented Jun 18, 2024

Related Issues

#12083

$ curl  -s -X POST localhost:1234/rpc/v1   -H "Content-Type: application/json"   -d '{
    "jsonrpc":"2.0",
    "method":"eth_getBlockByNumber",
    "params":["safe", true],
    "id":1
}'  | jq -r '.result.number' | xargs printf "%d\n"
1714033

$ curl  -s -X POST localhost:1234/rpc/v1   -H "Content-Type: application/json"   -d '{
    "jsonrpc":"2.0",
    "method":"eth_getBlockByNumber",
    "params":["latest", true],
    "id":1
}'  | jq -r '.result.number' | xargs printf "%d\n"
1714063

Proposed Changes

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

Copy link

github-actions bot commented Jun 18, 2024

All checks have completed

❌ Failed Test / Test (itest-eth_legacy_transactions) (pull_request)

@snissn snissn marked this pull request as ready for review June 18, 2024 23:34
gateway/proxy_eth.go Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Jun 19, 2024

Would be worth doing finalized as well in this and then we'd have the complete set, no?

@aarshkshah1992
Copy link
Contributor

@snissn "finalised" should be very similar to the "safe" implementation no ? Best to just add it to this PR.

@rvagg
Copy link
Member

rvagg commented Jun 19, 2024

there's a build.Finality for that btw

@snissn
Copy link
Contributor Author

snissn commented Jun 20, 2024

Thanks @rvagg @aarshkshah1992 !!

I will move the safeEpochDelay to a const in eth_types and will use build.Finality for the additional case and request a review again when that is ready

@snissn
Copy link
Contributor Author

snissn commented Jun 20, 2024

I've added support for 'finalized' and created a const for the safe delay with the suggested comment

$ curl  -s -X POST localhost:1234/rpc/v1   -H "Content-Type: application/json"   -d '{
    "jsonrpc":"2.0",
    "method":"eth_getBlockByNumber",
    "params":["safe", true],
    "id":1
}'  | jq -r '.result.number' | xargs printf "%d\n"
1717680

$ curl  -s -X POST localhost:1234/rpc/v1   -H "Content-Type: application/json"   -d '{
    "jsonrpc":"2.0",
    "method":"eth_getBlockByNumber",
    "params":["latest", true],
    "id":1
}'  | jq -r '.result.number' | xargs printf "%d\n"
1717710

$ curl  -s -X POST localhost:1234/rpc/v1   -H "Content-Type: application/json"   -d '{
    "jsonrpc":"2.0",
    "method":"eth_getBlockByNumber",
    "params":["finalized", true],
    "id":1
}'  | jq -r '.result.number' | xargs printf "%d\n"
1716810

@rvagg
Copy link
Member

rvagg commented Jun 20, 2024

Lack of tests is a bit of a bummer on this and there's not an easy slot for a new unit test - but if you could come up with an easy way to do it locally there then that would be great.

Here's an integration test that would exercise it, which is something at least, stick this in itests/eth_api_test.go under TestEthGetGenesis which has a EthGetBlockByNumber call so it's in a similar ballpark:

func TestEthBlockNumberAliases(t *testing.T) {
	blockTime := 2 * time.Millisecond
	kit.QuietMiningLogs()
	client, _, ens := kit.EnsembleMinimal(t, kit.MockProofs(), kit.ThroughRPC())
	ens.InterconnectAll().BeginMining(blockTime)
	ens.Start()

	build.Clock.Sleep(time.Second)

	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
	defer cancel()

	head := client.WaitTillChain(ctx, kit.HeightAtLeast(build.Finality+100))

	// latest should be head-1 (parents)
	latestEthBlk, err := client.EVM().EthGetBlockByNumber(ctx, "latest", true)
	require.NoError(t, err)
	diff := int64(latestEthBlk.Number) - int64(head.Height()-1)
	require.GreaterOrEqual(t, diff, int64(0))
	require.LessOrEqual(t, diff, int64(2))

	// safe should be latest-30
	safeEthBlk, err := client.EVM().EthGetBlockByNumber(ctx, "safe", true)
	require.NoError(t, err)
	diff = int64(latestEthBlk.Number-30) - int64(safeEthBlk.Number)
	require.GreaterOrEqual(t, diff, int64(0))
	require.LessOrEqual(t, diff, int64(2))

	// finalized should be Finality blocks behind latest
	finalityEthBlk, err := client.EVM().EthGetBlockByNumber(ctx, "finalized", true)
	require.NoError(t, err)
	diff = int64(latestEthBlk.Number) - int64(build.Finality) - int64(finalityEthBlk.Number)
	require.GreaterOrEqual(t, diff, int64(0))
	require.LessOrEqual(t, diff, int64(2))
}

maybe that could be made more concise; the awkwardness is in wanting to allow a fast moving chain to move on between calls, so we can't guarantee we'll hit it precisely, hence the 2 epochs

@snissn
Copy link
Contributor Author

snissn commented Jun 21, 2024

Thanks that test looks great and it feels a lot better that we have this test coverage with this PR! 84199c2

@rvagg rvagg enabled auto-merge (squash) June 21, 2024 01:38
@rvagg rvagg merged commit 6408709 into master Jun 21, 2024
76 of 77 checks passed
@rvagg rvagg deleted the mikers/safeEthGetBlockByNumber branch June 21, 2024 01:38
@snissn snissn self-assigned this Jun 21, 2024
@snissn snissn linked an issue Jun 21, 2024 that may be closed by this pull request
9 tasks
@rvagg rvagg mentioned this pull request Jul 3, 2024
rvagg added a commit that referenced this pull request Jul 3, 2024
…12110)

* add support for eth_getBlockByNumber to accept the term safe which we are using as 30 blocks

* fix lint catch of unnecessary cast

* add finalized to get block by number

* Update chain/types/ethtypes/eth_types.go

Co-authored-by: Rod Vagg <[email protected]>

* add test for eth get block by number to accept latest and safe and finalized as arguments

---------

Co-authored-by: Rod Vagg <[email protected]>
rvagg added a commit that referenced this pull request Jul 10, 2024
…12110)

* add support for eth_getBlockByNumber to accept the term safe which we are using as 30 blocks

* fix lint catch of unnecessary cast

* add finalized to get block by number

* Update chain/types/ethtypes/eth_types.go

Co-authored-by: Rod Vagg <[email protected]>

* add test for eth get block by number to accept latest and safe and finalized as arguments

---------

Co-authored-by: Rod Vagg <[email protected]>
rvagg added a commit that referenced this pull request Jul 10, 2024
…12110)

* add support for eth_getBlockByNumber to accept the term safe which we are using as 30 blocks

* fix lint catch of unnecessary cast

* add finalized to get block by number

* Update chain/types/ethtypes/eth_types.go

Co-authored-by: Rod Vagg <[email protected]>

* add test for eth get block by number to accept latest and safe and finalized as arguments

---------

Co-authored-by: Rod Vagg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add finality-related params for eth_getBlockByNumber
3 participants