Skip to content

rpc: set scheme for ws and ipc conns to the server #23615

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
Merged
holiman merged 4 commits into from
Sep 21, 2021
Merged

rpc: set scheme for ws and ipc conns to the server #23615

holiman merged 4 commits into from
Sep 21, 2021

Conversation

@s1na
Copy link
Contributor

@s1na s1na commented Sep 21, 2021

Fixes #23593

Clef is reading out the request scheme from key "scheme" of the context. Scheme is being set for http requests in

go-ethereum/rpc/http.go

Line 244 in b61ef24

ctx = context.WithValue(ctx, "scheme", r.Proto)
but its not set for websocket or ipc connections.

@s1na s1na requested review from fjl and holiman as code owners September 21, 2021 12:01
holiman
holiman reviewed Sep 21, 2021
View reviewed changes
rpc/client.go Outdated
// Close closes the client, aborting any in-flight requests.
func (c *Client) Close() {
if c.isHTTP {
if c.scheme == HTTPConn {
Copy link
Contributor

@holiman holiman Sep 21, 2021

Choose a reason for hiding this comment

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

An alternative change would have been to hide this internaly, so instead of c.isHTTP {, you would have c.isHTTP() {

holiman
holiman reviewed Sep 21, 2021
View reviewed changes
rpc/client.go Outdated
Comment on lines 63 to 65
HTTPConn = "http"
WSConn = "ws"
IPCConn = "ipc"
Copy link
Contributor

@holiman holiman Sep 21, 2021

Choose a reason for hiding this comment

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

Afaict, this is only used in the rpc package

Suggested change
HTTPConn = "http"
WSConn = "ws"
IPCConn = "ipc"
httpConn = "http"
wsConn = "ws"
ipcConn = "ipc"

holiman
holiman approved these changes Sep 21, 2021
View reviewed changes
Copy link
Contributor

@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.

Seems to work!

@holiman
Copy link
Contributor

holiman commented Sep 21, 2021

Tested this a bit, seems to work as intended

t=2021-09-21T17:58:33+0200 lvl=info msg=Configured api=signer audit log=audit.log
t=2021-09-21T17:59:05+0200 lvl=info msg=Version    api=signer type=request metadata="{\"remote\":\"NA\",\"local\":\"NA\",\"scheme\":\"ipc\",\"User-Agent\":\"\",\"Origin\":\"\"}"
t=2021-09-21T17:59:05+0200 lvl=info msg=Version    api=signer type=response data=6.1.0 error=nil
t=2021-09-21T17:59:15+0200 lvl=info msg=Version    api=signer type=request  metadata="{\"remote\":\"NA\",\"local\":\"NA\",\"scheme\":\"ipc\",\"User-Agent\":\"\",\"Origin\":\"\"}"
t=2021-09-21T17:59:15+0200 lvl=info msg=Version    api=signer type=response data=6.1.0 error=nil
t=2021-09-21T17:59:19+0200 lvl=info msg=Version    api=signer type=request  metadata="{\"remote\":\"NA\",\"local\":\"NA\",\"scheme\":\"ipc\",\"User-Agent\":\"\",\"Origin\":\"\"}"
t=2021-09-21T17:59:19+0200 lvl=info msg=Version    api=signer type=response data=6.1.0 error=nil
t=2021-09-21T17:59:29+0200 lvl=info msg=Version    api=signer type=request  metadata="{\"remote\":\"NA\",\"local\":\"NA\",\"scheme\":\"ipc\",\"User-Agent\":\"\",\"Origin\":\"\"}"
t=2021-09-21T17:59:29+0200 lvl=info msg=Version    api=signer type=response data=6.1.0 error=nil
t=2021-09-21T18:00:11+0200 lvl=info msg=List       api=signer type=request  metadata="{\"remote\":\"NA\",\"local\":\"NA\",\"scheme\":\"ipc\",\"User-Agent\":\"\",\"Origin\":\"\"}"
t=2021-09-21T18:01:02+0200 lvl=info msg=List       api=signer type=response data=[]
t=2021-09-21T18:01:33+0200 lvl=info msg=List       api=signer type=request  metadata="{\"remote\":\"NA\",\"local\":\"NA\",\"scheme\":\"ipc\",\"User-Agent\":\"\",\"Origin\":\"\"}"

on master

t=2021-09-21T18:01:57+0200 lvl=info msg=List       api=signer type=response data=[]
t=2021-09-21T18:02:40+0200 lvl=info msg=Configured api=signer audit log=audit.log
t=2021-09-21T18:02:43+0200 lvl=info msg=Version    api=signer type=request metadata="{\"remote\":\"NA\",\"local\":\"NA\",\"scheme\":\"NA\",\"User-Agent\":\"\",\"Origin\":\"\"}"
t=2021-09-21T18:02:43+0200 lvl=info msg=Version    api=signer type=response data=6.1.0 error=nil
t=2021-09-21T18:02:47+0200 lvl=info msg=List       api=signer type=request  metadata="{\"remote\":\"NA\",\"local\":\"NA\",\"scheme\":\"NA\",\"User-Agent\":\"\",\"Origin\":\"\"}"
t=2021-09-21T18:02:56+0200 lvl=info msg=List       api=signer type=response data=[]

@holiman holiman added this to the 1.10.9 milestone Sep 21, 2021
@holiman holiman merged commit ca9bce9 into ethereum:master Sep 21, 2021
@s1na s1na deleted the rpc-conn-scheme branch September 23, 2021 07:40
yongjun925 pushed a commit to DODOEX/go-ethereum that referenced this pull request Dec 3, 2022
@s1na @zzyalbert
rpc: set scheme for ws and ipc conns to the server (ethereum#23615)
24069fc 
* rpc: set scheme for ws and ipc conns to the server

* rpc: review fixes

* rpc: bugfix

* rpc: empty default scheme
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fjl fjl Awaiting requested review from fjl fjl is a code owner

1 more reviewer

@holiman holiman holiman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.10.9

Development

Successfully merging this pull request may close these issues.

Clef req.meta.scheme is NA even request from clef.ipc

2 participants

@s1na @holiman