feat: add param arguments for initializing websocket #992

KolbyML · 2023-10-14T19:31:05Z

What was wrong?

When I run bridge with multiple nodes I get an error on the lines of WS socker address already bound When bridge makes X connections it WS binds to 8456 everytime.

Closes #757

How was it fixed?

I am just removing WS from starting with HTTP as it isn't high priority enough to properly support WS

njgheorghita · 2023-10-16T17:17:10Z

#764

KolbyML · 2023-10-16T18:44:59Z

#764

Ok I am just removing WS from starting with the HTTP rpc in that case, as properly supporting WS isn't high priority right now

njgheorghita · 2023-10-16T20:23:27Z

@KolbyML If I remember correctly, @ogenev and I discussed this option before and decided that it was better to run both ws & http simultaneously... I'll see if I can dig up that conversation

njgheorghita · 2023-10-16T20:24:52Z

Looks like it was probably to follow geth's behavior ... #757 (comment) unless I'm missing some other part of the conversation

njgheorghita · 2023-10-16T20:26:01Z

I think the optimal solution in this case is to add a cli flag that allows for custom ws ports, which we could use when running bridge with > 1 node to avoid collisions

KolbyML · 2023-10-16T21:00:32Z

I'd suggest we add a --ws flag which will...

enable the ws server if the flag is provided

take a port, --ws 8546

taken from #757 (comment)

Sounds like the solution was to optionally run WS simulatneously if a port was provided.

Which is different then

I think the optimal solution in this case is to add a cli flag that allows for custom ws ports, which we could use when running bridge with > 1 node to avoid collisions

^ which requires we run WS regardless of if we pass a port param or not

I am confused cause the comment linked says running WS is optional. I think WS running optionally is the better solution here. As then users don't have to worry or wonder why there 2nd trin instance isn't running. Also why run WS if the user isn't using it.

njgheorghita · 2023-10-17T14:59:27Z

Also why run WS if the user isn't using it.

Yup, I agree. And if I remember correctly, I also pushed for this argument. Unfortunately, I can't remember the justification, all I remember is @ogenev pushed for running WS by default. Maybe, I'm misremembering, but would like a final confirmation from Ognyan here that we're good to go ahead with making WS optional (and will only be enabled via an cli flag)

KolbyML · 2023-10-17T15:28:31Z

Also why run WS if the user isn't using it.

Yup, I agree. And if I remember correctly, I also pushed for this argument. Unfortunately, I can't remember the justification, all I remember is @ogenev pushed for running WS by default. Maybe, I'm misremembering, but would like a final confirmation from Ognyan here that we're good to go ahead with making WS optional (and will only be enabled via an cli flag)

Okie dokie sounds like a plan

ogenev · 2023-10-23T10:07:00Z

I'm ok with removing the WS by default but IMO this is not solving the real issue mentioned in the PR description and I don't think it justifies disabling WS.

If a bridge runs multiple nodes and all of those nodes bind to the same WS address, it is either a bridge configuration issue or Trin issue that doesn't allow using a custom WS address, or both :)

KolbyML · 2023-10-23T15:04:19Z

I'm ok with removing the WS by default but IMO this is not solving the real issue mentioned in the PR description and I don't think it justifies disabling WS.

If a bridge runs multiple nodes and all of those nodes bind to the same WS address, it is either a bridge configuration issue or Trin issue that doesn't allow using a custom WS address, or both :)

So are you ok with us only enabling ws if somebody specifies the port on running trin so trin -- --ws-port=1234.
I.e the same way geth handles this situation?

njgheorghita · 2023-10-24T18:46:30Z

In this situation, I feel like following what geth does is the easiest way to proceed, unless somebody can produce a strong use case for another setup...

geth --ws --ws.port 3334

Using just the --ws flag, will enable WS connections using the default port, and users can otherwise specify their own custom port. Without the --ws flag, no ws is enabled. And without the --ws flag, providing a --ws-port flag should cause an error. This configuration should solve our bridge conflict.

We could do something like .... --ws-port default / --ws-port 3334 (aka remove the --ws flag) ... to help remove some redundancy, but imo I'd prefer to simply follow geth's behavior in this scenario.

KolbyML · 2023-10-25T03:17:21Z

Ok PR is ready to review with the geth styled ws pattern nick posted above

njgheorghita

Looks good! My only question is w/ regards to this comment by @ogenev

HTTP and WS must run on a separate port. Current defaults are 8545 for HTTP and 8546 for WS.

Why do we require separate ports? From what I can tell, it seems technically possible to run both ws & http processes on the same port. Now, it's a different question about whether this is something we want to support or not?

Right now, it looks like we support running ws & http on the same port, but I'm leaning towards requiring separate ports...

morph-dev · 2023-10-25T19:16:17Z

I have few comments / suggestions

might be worth documenting these changes in book/src/developers/contributing/build_instructions/linux.md (or somewhere else in the book)
If I'm not mistaken, there is now difference in how we configure http and websocket (http using web3_http_address (address and port) and websocket just configuring port)
there is also implied assumption that web3_transport needs to be set to http in order for websocket to work, we don't have any checks whether things are properly set (e.g. someone might set websocket without changing web3_transport)

If this PR is blocking some other work, you can merge it like this and I can look into making these changes.

njgheorghita · 2023-10-26T17:09:06Z

Following up from the discussion in today's sync....

Let's shelve the HTTP and WS must run on a separate port. question for now. We currently support opening HTTP & WS connections on the same port.

Regarding @morph-dev 's suggestions...

Agreed that we should make sure all relevant book updates are made to reflect the changes in this pr
there is now difference in how we configure http and websocket.. personally, I'm ok with this. However, if you want to propose an updated cli arg design, I'm open to exploring how we can consolidate formats. Though this is out of scope for this pr
This pr should handle the case where a user tries to use ws alongside ipc. Imo, simply throwing an error is sufficient for the meantime

KolbyML · 2023-10-31T21:02:23Z

Agreed that we should make sure all relevant book updates are made to reflect the changes in this pr

This pr should handle the case where a user tries to use ws alongside ipc. Imo, simply throwing an error is sufficient for the meantime

@njgheorghita I resolved these 2 concerns. PR is ready for another review

morph-dev · 2023-11-01T06:07:29Z

njgheorghita

🚢

njgheorghita · 2023-11-01T14:47:53Z

ethportal-api/src/types/cli.rs

+    #[arg(
+        long = "ws-port", 
+        help = "The WebSocket port to listen on.", 
+        default_value_t = DEFAULT_WEB3_WS_PORT


I think we should add requires the ws argument here

njgheorghita · 2023-11-01T14:49:13Z

ethportal-api/src/types/cli.rs

+            Web3TransportType::IPC => {
+                match config.web3_http_address.as_str() {
+                    DEFAULT_WEB3_HTTP_ADDRESS => {}
+                    p => return Err(Error::raw(ErrorKind::ArgumentConflict,format!("Must not supply an http address when using ipc protocol for json-rpc (received: {p})"))),


I know it's not your change, but can you update this single letter variable to something more descriptive

KolbyML self-assigned this Oct 14, 2023

KolbyML changed the title ~~fix: prevent WS from using same port causing bridge failing to start on multiple nodes~~ fix: prevent WS from using same port causing bridge to fail on node count > than 1 Oct 14, 2023

KolbyML requested review from ogenev, carver and njgheorghita October 14, 2023 19:36

KolbyML marked this pull request as ready for review October 14, 2023 19:36

KolbyML changed the title ~~fix: prevent WS from using same port causing bridge to fail on node count > than 1~~ fix: remove WS from starting with HTTP to prevent bridge from failing on node count > than 1 Oct 16, 2023

KolbyML force-pushed the remove-ws-from branch 3 times, most recently from c77158b to 49c71c3 Compare October 25, 2023 03:09

KolbyML changed the title ~~fix: remove WS from starting with HTTP to prevent bridge from failing on node count > than 1~~ feat: add param arguments for initializing websocket Oct 25, 2023

njgheorghita reviewed Oct 25, 2023

View reviewed changes

njgheorghita approved these changes Nov 1, 2023

View reviewed changes

feat: add param arguments for initializing websocket

36503d2

KolbyML force-pushed the remove-ws-from branch from b9c647a to 36503d2 Compare November 1, 2023 15:57

KolbyML merged commit d6ba50b into ethereum:master Nov 1, 2023

morph-dev mentioned this pull request Dec 2, 2024

Websocket port should be configurable via cli arguments #764

Closed

KolbyML deleted the remove-ws-from branch January 22, 2025 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add param arguments for initializing websocket #992

feat: add param arguments for initializing websocket #992

KolbyML commented Oct 14, 2023 •

edited by njgheorghita

Loading

njgheorghita commented Oct 16, 2023

KolbyML commented Oct 16, 2023 •

edited

Loading

njgheorghita commented Oct 16, 2023

njgheorghita commented Oct 16, 2023

njgheorghita commented Oct 16, 2023 •

edited

Loading

KolbyML commented Oct 16, 2023 •

edited

Loading

njgheorghita commented Oct 17, 2023

KolbyML commented Oct 17, 2023

ogenev commented Oct 23, 2023 •

edited

Loading

KolbyML commented Oct 23, 2023

njgheorghita commented Oct 24, 2023

KolbyML commented Oct 25, 2023

njgheorghita left a comment

morph-dev commented Oct 25, 2023

njgheorghita commented Oct 26, 2023

KolbyML commented Oct 31, 2023

morph-dev commented Nov 1, 2023

njgheorghita left a comment

njgheorghita Nov 1, 2023

njgheorghita Nov 1, 2023

feat: add param arguments for initializing websocket #992

feat: add param arguments for initializing websocket #992

Conversation

KolbyML commented Oct 14, 2023 • edited by njgheorghita Loading

What was wrong?

How was it fixed?

njgheorghita commented Oct 16, 2023

KolbyML commented Oct 16, 2023 • edited Loading

njgheorghita commented Oct 16, 2023

njgheorghita commented Oct 16, 2023

njgheorghita commented Oct 16, 2023 • edited Loading

KolbyML commented Oct 16, 2023 • edited Loading

njgheorghita commented Oct 17, 2023

KolbyML commented Oct 17, 2023

ogenev commented Oct 23, 2023 • edited Loading

KolbyML commented Oct 23, 2023

njgheorghita commented Oct 24, 2023

KolbyML commented Oct 25, 2023

njgheorghita left a comment

Choose a reason for hiding this comment

morph-dev commented Oct 25, 2023

njgheorghita commented Oct 26, 2023

KolbyML commented Oct 31, 2023

morph-dev commented Nov 1, 2023

njgheorghita left a comment

Choose a reason for hiding this comment

njgheorghita Nov 1, 2023

Choose a reason for hiding this comment

njgheorghita Nov 1, 2023

Choose a reason for hiding this comment

KolbyML commented Oct 14, 2023 •

edited by njgheorghita

Loading

KolbyML commented Oct 16, 2023 •

edited

Loading

njgheorghita commented Oct 16, 2023 •

edited

Loading

KolbyML commented Oct 16, 2023 •

edited

Loading

ogenev commented Oct 23, 2023 •

edited

Loading