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: Add eth_syncing RPC method #10719

Merged
merged 1 commit into from
May 10, 2023
Merged

feat: Add eth_syncing RPC method #10719

merged 1 commit into from
May 10, 2023

Conversation

fridrik01
Copy link
Contributor

@fridrik01 fridrik01 commented Apr 21, 2023

Fixes: #10622

This PR adds eth_syncing as a new RPC method to Lotus. This method returns an object with data about the sync status or false.

NOTE: I added SyncApi as dependency to the EthModule in order to implement this functionality, not sure if there was any way around that...

NOTE: If calling eth_syncing as soon as lotus daemon starts (within a <5 seconds) then it will result in an error (see test plan) as the syncing hasn't started properly, but I am to making it instead wait.

Test plan

Started my lotus daemon on mainnet from snapshot and calling this method produces the following output depending on the sync status:

Before starting sync:

curl http://localhost:1234/rpc/v1 -X POST -H "Content-Type: application/json" --data '{"method":"eth_syncing","params":[],"id":1,"jsonrpc":"2.0"}'
{"jsonrpc":"2.0","id":1,"error":{"code":1,"message":"no active syncs, try again"}}

During sync:

curl http://localhost:1234/rpc/v1 -X POST -H "Content-Type: application/json" --data '{"method":"eth_syncing","params":[],"id":1,"jsonrpc":"2.0"}'
{"jsonrpc":"2.0","result":{"startingblock":"0x2a9e8e","currentblock":"0x2a9e95","highestblock":"0x2a9ea8"},"id":1}

After sync is done (according to spec "result" should be set to "false" in this case):

url http://localhost:1234/rpc/v1 -X POST -H "Content-Type: application/json" --data '{"method":"eth_syncing","params":[],"id":1,"jsonrpc":"2.0"}'
{"jsonrpc":"2.0","result":"false","id":1}

@fridrik01 fridrik01 force-pushed the 10622-add-eth-syncing branch 3 times, most recently from 507158f to 5c93adf Compare April 21, 2023 18:39
@fridrik01 fridrik01 force-pushed the 10622-add-eth-syncing branch from 5c93adf to 42270a0 Compare April 21, 2023 19:23
@fridrik01 fridrik01 marked this pull request as ready for review April 21, 2023 19:51
@fridrik01 fridrik01 requested a review from a team as a code owner April 21, 2023 19:51
@fridrik01 fridrik01 enabled auto-merge April 21, 2023 20:36
if ss.Stage == api.StageIdle {
continue
}
working = i
Copy link
Contributor

@maciejwitowski maciejwitowski Apr 25, 2023

Choose a reason for hiding this comment

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

Can you explain this? Why are looking for the index of the last non-idle one from ActiveSyncs and eg not the 1st one? (I'm not sure what ActiveSyncs represent tbh)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to have the same logic as when calling lotus sync wait which is done like this (see https://github.com/filecoin-project/lotus/blob/master/cli/sync.go#L286)

There is also this commit which picks the last worker in case all are idle, but it does not go in details as to why

@fridrik01 fridrik01 disabled auto-merge May 4, 2023 14:34
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM with one bug.

chain/types/ethtypes/eth_types.go Outdated Show resolved Hide resolved
@fridrik01 fridrik01 force-pushed the 10622-add-eth-syncing branch 2 times, most recently from 22cf8da to 2bfa6b0 Compare May 10, 2023 20:02
This commit adds eth_syncing RPC method which returns an object
with data about the sync status or false.
@fridrik01 fridrik01 force-pushed the 10622-add-eth-syncing branch from 2bfa6b0 to 2bc205e Compare May 10, 2023 20:08
@fridrik01 fridrik01 merged commit b4ea0db into master May 10, 2023
@fridrik01 fridrik01 deleted the 10622-add-eth-syncing branch May 10, 2023 20:21
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.

Lotus to support eth_syncing RPC method
3 participants