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

simulators: integrate les light client into the RPC test suite #432

Closed
wants to merge 16 commits into from

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Feb 2, 2021

It's a following PR based on #424

Copy link
Collaborator

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman
Copy link
Collaborator

holiman commented Mar 11, 2021

It's a following PR based on #424

Does this require that other PR or can this be merged separately?

@rjl493456442
Copy link
Member Author

rjl493456442 commented Mar 12, 2021

@holiman The problem of the previous PR(#424) is that:

LES Server has this property that it will reject all client connections by default if it's not synced yet. While the indicator of "node synced" is setup by (1) inserting a new block (2) start mining.

For (1) it's impossible/ugly in the hive test suite, we insert the pre-configured blocks by offline command and no more block involved. So the indicator won't be marked forever.

For (2) it's feasible to start mining with thread=0. It won't waste CPU resource but can mark the node as "synced". But it's not elegant enough?

So I add a new feature in Geth, so that LES server can start serving even the les server is not synced. It can work perfectly with new version Geth, but it won't work for the legacy Geths. So it blocks the PR.

We can either filter out the legacy Geth version for LES testing or use approach (2) so that it can work for all Geths. What's your option about it?

And for this RPC test suite for LES, it's based on the previous one. If the previous one can be merged, we can just merge this one.

cc @fjl

@fjl
Copy link
Collaborator

fjl commented Mar 16, 2021

I think we need to wait a couple more days before merging this. I'm working on integrating the client roles from PR #443 at the moment. When this is done, we can remove the client name logic in this PR and just define roles for les-capable clients.

@fjl
Copy link
Collaborator

fjl commented Feb 2, 2022

Finally added this in 1f7f6b4 and 9e744e7.

@fjl fjl closed this Feb 2, 2022
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.

3 participants