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

Start incrementing jsonrpc message id from random number #5371

Merged
merged 4 commits into from
Aug 25, 2022
Merged

Start incrementing jsonrpc message id from random number #5371

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eae2c1a
start incrementing jsonrpc.id from random number
Muhammad-Altabba Aug 22, 2022
e766254
modify the starging value of Jsonrpc message id
Muhammad-Altabba Aug 22, 2022
4a8ae6d
modify a comment at jsonrpc.js
Muhammad-Altabba Aug 22, 2022
7d83e6b
enhance calculating Jsonrpc message id
Muhammad-Altabba Aug 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,3 +592,4 @@ Released with 1.0.0-beta.37 code base.

### Fixed
- Browser builds support polyfills (#5031) (#5053) (#4659) (#4767)
- Start incrementing jsonrpc.id from random number (#5327)
4 changes: 3 additions & 1 deletion packages/web3-core-requestmanager/src/jsonrpc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@

// Initialize Jsonrpc as a simple object with utility functions.
var Jsonrpc = {
messageId: 0
// This is the starting counter for the Jsonrpc.id. And, to insure staying in the safe range while incrementing later,
// pick a random number between 0 and (the maximum safe integer minus the max unsigned 32 bit integer)
messageId: Math.floor(Math.random() * (Number.MAX_SAFE_INTEGER - Math.pow( 2, 32 )))
Copy link
Contributor

Choose a reason for hiding this comment

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

I had a hunch that this will not fix the actual issue.

Responses get mixed up due to conflicting payload IDs

The response will still be mixed up if the payload Ids are conflicting, starting point does not matter and randomizing starting point would minimize but not fixed the problem.

For once instance of the web3 there will be no conflicting payload IDs in any case.

Copy link
Contributor

Choose a reason for hiding this comment

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

As per here: https://www.jsonrpc.org/specification

An identifier established by the Client that MUST contain a String, Number, or NULL value if included.

uuid is already used in 1.x, maybe we could use it, also, for request id`s. We won't need a starting point and other libraries use numbers for ids.

Copy link
Contributor Author

@Muhammad-Altabba Muhammad-Altabba Aug 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, there will be no issue if one instance is running and one library is used. And, yes, this solution will minimize the possibility of a conflict to some grade that is neglected but the possibility is still there.
But using a uuid (that would be handled as a string) instead of a number would be a breaking change. Even the tests for 1.x would need to be modified accordingly.

However, I proposed to handle this in a different way in v4.x as there we can do a breaking change by making the message id as a string that contains a UUID or another format. The new issue is here: #5373

And if you have a better suggestion to solve this in v1.x, please advise.

Thanks,

Copy link
Contributor

Choose a reason for hiding this comment

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

We can proceed with this approach but considering it's not a permanent fix.

Also it does not matter if there is more than one library using the same id for the same endpoint. For the HTTP, the RPC server will be responding synchronously. For WS based RPC server it will be responding per connection. So if there are two libraries, both would be having different connections and that will not conflict the ids.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @nazarhussain ,
As you see this as "not a permanent fix", do you have a better suggestion for 1.x that does not cause a breaking change?

Copy link

@abrindam abrindam Aug 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

So if there are two libraries, both would be having different connections and that will not conflict the ids.

@nazarhussain I just wanted to note in my original report #5327, I saw a situation where two libraries were using the same connection, which resulted in an ID conflict. I'm not certain how this happened because I'm completely unfamilar with your codebase, but it's somehow possible.

In my case, it was eth-block-tracker that was sending a message that somehow bypassed jsonrpc.js but still was being processed by the same websocket connection that jsonrpc.js was using, which resulted in a conflict.

Maybe someone more experienced than me could figure out how that happened? There might be another lurking bug there.

};

/**
Expand Down