Responses get mixed up due to conflicting payload IDs

[abrindam]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

When using this library in conjunction with Truffle's HDWalletProvider, when requesting the current network ID via web3.eth.net.getId(), rarely (1 out of 20 or so) you will get back the block height instead of the network ID.

Expected Behavior

If you request the network ID, you should get back the network ID and not another random piece of information.

Steps to Reproduce

The following node script reproduces the issue at a frequency of approximately 1/20 times against a local blockchain. The root cause is a network race condition so using a remote blockchain might help reproduce more easily.

const Web3 = require('web3')
const HDWalletProvider = require("@truffle/hdwallet-provider");

async function main() {

    const nodeAddress = "ws://127.0.0.1:8545"
    const accountKey = "<valid private key here>"
                    
    provider = new HDWalletProvider({
        privateKeys: [accountKey],
        providerOrUrl: nodeAddress
    });

    const web3 = new Web3(provider)
    console.log(await web3.eth.net.getId())

}

main().catch(console.log).then(() => process.exit(0))

Web3.js Version

1.7.4

Environment

• Operating System: MacOS 12.4
• Browser: N/A
• Node.js Version: v16.15.1
• NPM Version: 8.11.0
• @truffle/hdwallet-provider: 2.0.13
• Ganache: v7.4.0

Anything Else?

I did a very deep dive into this issue to understand what is going on. It looks like the WebsocketProvider uses transaction IDs to correlate request messages with response messages, but there is no guarantee that transaction messages are unique. In particular, in my case, there was a conflict between the ID generation mechanism of jsonrpc.js (https://github.com/ChainSafe/web3.js/blob/1.x/packages/web3-core-requestmanager/src/jsonrpc.js#L40) in this package, and the eth-block-tracker package which is brought in by HDWalletProvider.

The issue with eth-block-tracker issue was much worse as they were always using id=1. They have already fixed this in a newer version (MetaMask/eth-block-tracker#42).

The library is using sequential ID generation, which is better, but if two different libraries were to use that strategy, there would be a high likelihood of collision.

In my opinion, the best fix here would be to not use an externally provided transaction ID to correlate messages, but to have the WebsocketProvider itself generate private correlation IDs that it knows to be unique. Failing that, I would suggest adjusting jsonrpc.js to use random numbers for IDs instead of sequential numbers, as this will greatly reduce the chance of collision.

Note that basically the same issue was reported years ago here: #1510. The proposed solution was the same, but it looked like the attempted fix went in a different direction.

[jdevcs]

@abrindam Thanks for investigation and reporting this our team will look this.

[Muhammad-Altabba]

Thanks @abrindam ,
The changes are done now for 1.x and it will be available in the next release. And this issue has been addressed and in discussion for version 4.x.