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

rpc: implement websockets with github.com/gorilla/websocket #19866

Merged
merged 8 commits into from Jul 22, 2019

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Jul 19, 2019

This change makes package rpc use the github.com/gorilla/websocket package
for WebSockets instead of golang.org/x/net/websocket. The new library is more robust
and supports all WebSocket features including continuation frames.

There are new tests for two issues with the previously-used library:

  • TestWebsocketClientPing checks handling of Ping frames.
  • TestWebsocketLargeCall checks whether the request size limit is
    applied correctly.

DialWebsocket's handling of the default Origin header has changed. It used to set the
local machine's hostname as the origin, but that's weird and won't work with any
Internet-facing service. The new behavior is creating connections without an Origin
header unless one is provided explicitly.

This change also raises the request size limit for HTTP and WebSocket connections
to 5 MB. Quite a few people have reported issues where geth fails to deploy contracts
when they're too large, and retesteth needs a larger limit to upload the pre-state of large
tests.

Fixes #19798
Fixes #16846
Updates #19640
Updates #19001

fjl added 3 commits July 19, 2019 15:54
This change makes package rpc use the github.com/gorilla/websocket
package for WebSockets instead of golang.org/x/net/websocket. The new
library is more robust and supports all WebSocket features including
continuation frames.

There are new tests for two issues with the previously-used library:

  - TestWebsocketClientPing checks handling of Ping frames.
  - TestWebsocketLargeCall checks whether the request size limit is
    applied correctly.
The client used to put the local hostname into the Origin header because
the server wanted an origin to accept the connection, but that's silly:
Origin is for browsers/websites. The nobody would whitelist a particular
hostname.

Now that the server doesn't need Origin anymore, don't bother setting
one for clients. Users who need an origin can use DialWebsocket to
create a client with arbitrary origin if needed.
@fjl fjl requested a review from holiman as a code owner July 19, 2019 14:03
@fjl fjl requested a review from karalabe July 19, 2019 14:03
@fjl fjl added this to the 1.9.1 milestone Jul 19, 2019
log.Debug(fmt.Sprintf("Allowed origin(s) for WS RPC interface %v", origins.ToSlice()))

f := func(cfg *websocket.Config, req *http.Request) error {
f := func(req *http.Request) bool {
// Skip origin verification if no Origin header is present. The origin check
// is supposed to protect against browser based attacks. Browsers always set
// Origin. Non-browser software can put anything in origin and checking it doesn't
// provide additional security.
if _, ok := req.Header["Origin"]; !ok {
Copy link
Member

Choose a reason for hiding this comment

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

We need to use req.Header.Get here, otherwise the code won't ignore the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's actually not true. net/http canonicalizes header keys, i.e. the keys in the map are assumed to be in the form returned by http.CanonicalHeaderKey.

Copy link
Contributor Author

@fjl fjl Jul 22, 2019

Choose a reason for hiding this comment

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

Using req.Header.Get is not great because it doesn't distinguish between absent and empty headers.

Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

SGTM, can't say I'm confident nothing's borked, but I guess we'd find out soon enough if anything obvious went haywire.

@karalabe karalabe merged commit 04e175b into ethereum:master Jul 22, 2019
@Deviad
Copy link

Deviad commented Jul 25, 2019

I have connection issues via websocket with web3j. Is it related to these changes?

@karalabe
Copy link
Member

Possibly. Please provide more information and a repro.

@Deviad
Copy link

Deviad commented Jul 25, 2019

I started having problems with Infura (same websocket connection issue) and for this reason I gave the go-ethereum client a shot (in order to have more debugging info possibly).
Here is where I establish the connection:

I know I had to use Websocket in the controller along with stomp probably, but before complicating things and get into an unexplored territory I wanted to be sure to be able to establish a connection and get some kind of result.

@Deviad
Copy link

Deviad commented Jul 26, 2019

EDIT
It finally works like this:

@Configuration
@Slf4j
public class InfuraSocketConfiguration {

    @Autowired
    private Web3jService web3jService;

    @SneakyThrows
    @Bean("webSocketSubscription")
    Flux<String> wsConnectNetty() {
//        Map<String, Object> config = Stream.of(
//                new AbstractMap.SimpleEntry<>("includeTransactions", true)
//        ).collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));

        Map<String, Object> map = Stream.of(
                new AbstractMap.SimpleEntry<>("id", 1),
                new AbstractMap.SimpleEntry<>("method", "eth_subscribe"),
                new AbstractMap.SimpleEntry<>("params", new Object[]{"newHeads"})
        ).collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));


        ObjectMapper objectMapper = new ObjectMapper();

        String json = objectMapper.writeValueAsString(map);

        Flux<String> input = Flux.generate(sink -> sink.next(json));

        ReactorNettyWebSocketClient client = new ReactorNettyWebSocketClient();
        EmitterProcessor<String> output = EmitterProcessor.create();

        Mono<Void> sessionMono = client.execute(URI.create(web3jService.getInfuraWsEndpoint()), session -> session.send(input.map(session::textMessage))
                .thenMany(session.receive().map(WebSocketMessage::getPayloadAsText).subscribeWith(output).then()).then());

        return output.doOnSubscribe(s -> {
            sessionMono.subscribe();
        }).doOnError(x -> log.info(x.getMessage()));
    }

    @Bean
    public ApplicationRunner applicationRunner(final @Qualifier("webSocketSubscription") Flux<String> ws) {
        return applicationArguments -> ws.subscribe(log::info);
    }

}

I would like to know why it doesn't work with Web3j though.
Basically now I have to create a class that is a wrapper over this lower level implementation with the operations that I need to perform.

ez-max pushed a commit to ez-max/go-ethereum that referenced this pull request Jul 29, 2019
…#19866)

* rpc: implement websockets with github.com/gorilla/websocket

This change makes package rpc use the github.com/gorilla/websocket
package for WebSockets instead of golang.org/x/net/websocket. The new
library is more robust and supports all WebSocket features including
continuation frames.

There are new tests for two issues with the previously-used library:

  - TestWebsocketClientPing checks handling of Ping frames.
  - TestWebsocketLargeCall checks whether the request size limit is
    applied correctly.

* rpc: raise HTTP/WebSocket request size limit to 5MB

* rpc: remove default origin for client connections

The client used to put the local hostname into the Origin header because
the server wanted an origin to accept the connection, but that's silly:
Origin is for browsers/websites. The nobody would whitelist a particular
hostname.

Now that the server doesn't need Origin anymore, don't bother setting
one for clients. Users who need an origin can use DialWebsocket to
create a client with arbitrary origin if needed.

* vendor: put golang.org/x/net/websocket back

* rpc: don't set Origin header for empty (default) origin

* rpc: add HTTP status code to handshake error

This makes it easier to debug failing connections.

* ethstats: use github.com/gorilla/websocket

* rpc: fix lint
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.

rpc: WebSocket client treats pings as errors Geth >= 1.8.3 can't deploy large contracts over websockets
3 participants