Redundant newHeads Subscriptions when waiting on multiple outstanding requests.

[benjamincburns]

Expected behavior

When I'm using a websocket or other subscription-capable provider, I'd assume that a single newHeads subscription would be all that's necessary for web3 to be notified when my transaction is confirmed.

Actual behavior

When I'm using a websocket or other subscription-capable provider, web3.js sets up one newHeads subscription for every outstanding transaction.

Steps to reproduce the behavior

1. Send a transaction.
2. Quickly send another one before the previous one is mined.
3. Count the number of newHeads subscriptions active on the client (it'll have increased by two).

Environment

I'm running into this primarily in a NodeJS environment while performance testing Hyperledger Besu.

Admittedly this likely isn't a major issue for public Ethereum, but it becomes quite substantial when submitting a high volume of transactions to a private network. For example, on a network with a 2s block time (typical for IBFT 2 networks), if the user is sending 100 transactions per second, there will be up to 200 newHeads subscriptions active on the client, all from a single user.

[benjamincburns]

I can work around this by using the usual provider-wrapping tricks, but I think it'd be better if this was fixed in web3 directly.

I may be wrong, but it looks like there's a single RequestManager shared across all transactions submitted from a single Web3 instance? If so, perhaps we should route the subscription request through there, and use a reference counter to determine when the subscription should be removed?

[GregTheGreek]

Do you mind providing a script for this? Happy to also take a look at it with hyperledger for better understanding :)

[benjamincburns]

Do you mind providing a script for this? Happy to also take a look at it with hyperledger for better understanding :)

@GregTheGreek - sorry, but your question was a bit ambiguous - a script for what, exactly? I'm preparing a workaround PR for HL Caliper that I hope to submit a bit later today (debugging it now). Otherwise the reproduction is really just to run an Ethereum benchmark with Caliper and observe that the number of newHeads subscriptions that it submits will match the number of transactions submitted.

I would've done this in web3.js directly, but I need it to be working quickly and I think it likely requires substantial structural changes. I'll post another comment shortly with links to the offending web3.js code, however, so that the investigate label can be removed.

Edit: Another easy way to reproduce would be to write a quick script that depends on web3.js to submit n arbitrary transactions. Don't await on the individual PromiEvent objects returned, but instead chuck them in an array and pass that array to Promise.all(). This causes them to execute concurrently. If you run against a client that has RPC logging enabled, you will see n calls to eth_sendTransaction or eth_sendRawTransaction, as well as n calls to eth_subscribe. Note that to see this with Ganache you'll need to be running with a block interval, as otherwise Ganache's "instamining" feature will still cause the transactions to execute in separate blocks.

[benjamincburns]

To address the investigate label:

The control flow of all transactions winds up routing through _confirmTransaction here in web3-core-method via the send function, defined in the same file, here. As you can guess from the name, this is the logic that's responsible for waiting until the transaction is confirmed before resolving the PromiEvent.

When using a provider that supports subscriptions, the logic of _confirmTransaction prefers a newHeads subscription over polling. That logic is here in the nested function assigned to a variable called startWatching. Note that there's no check for existing redundant newHeads subscriptions in that logic.

[benjamincburns]

If someone does improve on this logic, it appears that checkConfirmation actually fires off a eth_getTransactionReceipt RPC call before setting up the newHeads subscription. If the subscription were set up prior to submitting the transaction, we wouldn't need this extra useless eth_getTransactionReceipt call.

Not only is this a performance issue, but I think it's somewhat of a race condition for networks with faster block times, though admittedly it's unlikely that we'd see this in the wild on any networks that exist today. That is, it's possible that we can request the receipt, not find it, the block containing the transaction is mined, and then we set up the newHeads subscription.

I've verified this in the logs from one of my recent Besu benchmarking runs. A snipped of these logs is shown below. Note that if you want Besu to produce these logs you'll need to run it at DEBUG log level.

2020-07-07 22:07:26.008+00:00 | vert.x-worker-thread-15 | DEBUG | WebSocketRequestHandler | WS-RPC request -> eth_sendRawTransaction
2020-07-07 22:07:26.018+00:00 | vert.x-eventloop-thread-10 | DEBUG | WebSocketService | Received Websocket request {"jsonrpc":"2.0","id":5,"method":"eth_getTransactionReceipt","params":["0x5550352e9ec802f2bb9dc279f97be762f3eb93737b969694189c1f076c2af5aa"]} (host=10.0.1.240, port=36914)
2020-07-07 22:07:26.018+00:00 | vert.x-worker-thread-17 | DEBUG | WebSocketRequestHandler | WS-RPC request -> eth_getTransactionReceipt 
2020-07-07 22:07:26.023+00:00 | vert.x-eventloop-thread-10 | DEBUG | WebSocketService | Received Websocket request {"jsonrpc":"2.0","id":6,"method":"eth_subscribe","params":["newHeads"]} (host=10.0.1.240, port=36914)

[benjamincburns]

I'm prepping a PR that should fix this. In this PR I'll be refactoring _confirmTransaction and friends, moving them to a more central component.

I'll be re-implementing the transaction management logic as a basic state machine. So far I've created a type called TransactionLifecycle that tracks the current state of each transaction, guards against invalid state transitions, and emits events for every state transition.

I'm now in the process of writing a TransactionManager class (might wind up just folding it into RequestManager, depending on size & cohesion/semantics). There'll be one instance of that per instance of Web3, and it'll handle the following responsibilities:

1. creating the TransactionLifecycle instances,
2. submitting the various requests,
3. handling their responses,
4. updating the state of each TransactionLifecycle.

Most of that logic will hang off of events fired by the TransactionLifecycle object. I'm also making it capable of detecting and handling reorgs as noted in #3611 , including the case where a transaction was mined successfully, but reorged out of the chain before it was confirmed. Note that it won't actually handle resubmitting the transaction in the case when the transaction was mined but subsequently reorged out of the chain.

There's a chance that some of this might spill over into how the PromiEvent is managed, or what events it fires. So far I'm hopeful that if there are any chances there, they'll be purely additive. If there are any modifications necessary I'll be sure to reach out to the maintainers before proceeding.

Finally, I'm also doing my best to write this in a way that it can be easily ported over to TypeScript when the time comes.

[benjamincburns]

I'm also not quite sure yet what I'll do about eth_call. If TransactionManager lives on as a separate type, I definitely don't intend to make that share the responsibility for calls. I'll likely move the logic that's common to both calls and transactions into some utility module, and then I'll either leave the stuff for eth_call in RequestManager, or I'll make a separate CallManager for it.

[GregTheGreek]

Can you link to the branch? I would like to follow the progress