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.

Sign up for GitHub

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

Jump to bottom

Enable superfluous ws.port flag to fix some Hive RPC tests #8909

Merged
merged 4 commits into from
Dec 7, 2023
Merged

Enable superfluous ws.port flag to fix some Hive RPC tests #8909

merged 4 commits into from
Dec 7, 2023

Conversation

somnathb1
Copy link
Contributor

@somnathb1 somnathb1 commented Dec 5, 2023 •
edited
Loading

Context

Websocket port flag
Hive tests for RPC suite depend on the (geth) default 8546 port. So, opening one more listener for this additional port if ws.port was specified. This flag isn't used in Erigon, as it shares port with http listener. Normally, one may not specify and it offers no other benefit.

@yperbasis
Copy link
Member

What's the test case fixed by the TTD reshuffle? Could you elaborate on what's happening there?

@somnathb1
Copy link
Contributor Author

What's the test case fixed by the TTD reshuffle? Could you elaborate on what's happening there?

A test uses clique with fakepow and is identified as PoS because of TotalTerminalDifficultyPassed=true

@yperbasis
Copy link
Member

What's the test case fixed by the TTD reshuffle? Could you elaborate on what's happening there?

A test uses clique with fakepow and is identified as PoS because of TotalTerminalDifficultyPassed=true

Hmm. Where is that code it Hive? It sounds to me that we should fix it there rather than on our side.

@somnathb1
Copy link
Contributor Author

What's the test case fixed by the TTD reshuffle? Could you elaborate on what's happening there?

A test uses clique with fakepow and is identified as PoS because of TotalTerminalDifficultyPassed=true

Hmm. Where is that code it Hive? It sounds to me that we should fix it there rather than on our side.

Now, I did find an erigon-specific bug on Hive's side, will PR a fix, and link it here.
Having said that, the question here is with TTD = nil (not "0"), do we want to consider the chain POS still?

@somnathb1
Copy link
Contributor Author

ethereum/hive#953 depends on this (for ws.port)

@yperbasis
Copy link
Member

Now, I did find an erigon-specific bug on Hive's side, will PR a fix, and link it here. Having said that, the question here is with TTD = nil (not "0"), do we want to consider the chain POS still?

To my mind yes, if we say TerminalTotalDifficultyPassed is true, that means the chain is PoS. I'd rather fix the Hive side to not set TerminalTotalDifficultyPassed to true for non-PoS chains if that's not super complicated.

@somnathb1
Copy link
Contributor Author

Now, I did find an erigon-specific bug on Hive's side, will PR a fix, and link it here. Having said that, the question here is with TTD = nil (not "0"), do we want to consider the chain POS still?

To my mind yes, if we say TerminalTotalDifficultyPassed is true, that means the chain is PoS. I'd rather fix the Hive side to not set TerminalTotalDifficultyPassed to true for non-PoS chains if that's not super complicated.

Reverted the TTD condition order shuffle in favour of a change into Hive

@yperbasis yperbasis changed the title Fix some Hive 2 RPC tests Enable superfluous ws.port flag to fix some Hive RPC tests Dec 7, 2023
@somnathb1 somnathb1 merged commit 5987d4e into devel Dec 7, 2023
6 checks passed
@somnathb1 somnathb1 deleted the som/hive2-fix branch December 7, 2023 10:59
@taratorio taratorio mentioned this pull request Jan 3, 2024
Merged
mh0lt pushed a commit that referenced this pull request Jan 3, 2024
@taratorio
devnet: fix ws port clash for state-sync scenario (#9125)
b94ca6d 
Getting an error in one of the bor nodes in devnet when trying to run
the "state-sync" scenario:
```
[EROR] [01-03|16:55:44.179] cli.StartRpcServer error                 err="could not start separate Websocket RPC api at port 8546: listen tcp 127.0.0.1:8546: bind: address already in use"
```

This happens for scenarios with more than 1 node.

Digging further this regressions has happened due to this change:
#8909

This PR fixes this by updating the devnet `NodeArgs` struct to set the
corresponding `--ws.port` `arg` tag which now exists.
@BrewTestBot BrewTestBot mentioned this pull request Jan 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yperbasis yperbasis yperbasis approved these changes

@battlmonstr battlmonstr Awaiting requested review from battlmonstr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@somnathb1 @yperbasis