Responses get mixed up due to conflicting payload IDs

[abrindam]

Current Behavior

When using this library via HDWalletProvider, and then 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))

Environment

• web3-provider-engine: 16.0.3
• web3.js: 1.7.4
• @truffle/hdwallet-provider: 2.0.13
• ganache: v7.4.0
• Node.js Version: v16.15.1
• NPM Version: 8.11.0

Analysis

After many hours of debugging, I root caused this to the fact that the Websocket sub-provider correlates request and response messages using payload.id, which may be generated by one or more other libraries, and who therefore may generate conflicting ids. In the case of a conflicting ID, the wrong response may be sent to the callback. In my case, two requests, one for eth_blockNumber and one for net_version, both would have having a generated ID of 1, which resulted in the block number being reported as the network ID.

This is the code in question: https://github.com/MetaMask/web3-provider-engine/blob/main/subproviders/websocket.js#L69

My primary suggestion would be to adjust the Websocket sub-provider so that it generates its own correlation IDs, rather than relying on an externally populated value. By doing this, it is possible guarantee uniqueness, thus bypassing the potential for this problem entirely.

For the record, bugs exist on the two libraries that generated conflicting IDs:
web3/web3.js#5327 (just filed by me)
MetaMask/eth-block-tracker#42 (fixed, but in a newer version than this library depends on)

At minimum, it would be great to upgrade eth-block-tracker to a version that doesn't have this issue.