feat: eth_getTransactionReceipt waits for txs in mempool#235
Conversation
alarso16
left a comment
There was a problem hiding this comment.
I think it's correct, but I am still worried about races between the mempool and inclusion in a block (just because I can't figure it out)
| sub := ir.vm.exec.SubscribeEnqueueEvent(ch) | ||
| defer sub.Unsubscribe() | ||
|
|
||
| for ; ; <-ch { |
There was a problem hiding this comment.
This for loop construction was super strange to me. It might just be because I've never seen it, but I had to whip out go.dev/play to double-check the behavior for this. I now think is equivalent of trying the inner loop once before waiting on the channel at all.
There was a problem hiding this comment.
What if we reformat it like this:
for {
// do all normal logic
select {
case <-ch:
// wait for new block, or your song
case <-ctx.Done():
return ctx.Err()
}
}I think the behavior is clearer
| // The mempool might have dropped the tx for reasons other than block | ||
| // inclusion, in which case we'll fall back on regular receipt retrival, | ||
| // which already handles unknown transactions. | ||
| known := ir.vm.mempool.Pool.Has(h) |
There was a problem hiding this comment.
I've been tracing code for a while, and I don't understand when transactions may be dropped from the mempool. Obviously if they're outpriced or something, but when are they removed explicitly?
A typical user flow is to call
eth_sendRawTransactionand then polleth_getTransactionReceipt. While #169 removed the need for polling as long as the transaction was already included in an accepted block, that left the opportunity for a race between block acceptance and the call toeth_getTransactionReceipt. This PR addresses the race.An open question is whether or not we want to add a timeout to the
Context. If we do then we SHOULD NOT returnctx.Err()because "deadline exceeded" will be interpreted incorrectly.In testing, I have no idea how to guarantee that the block is accepted after the call to
GetTransactionReceipt()is initiated without instrumenting the function itself. Since there is another, longer-running test, I simply marked both as parallel. In doing so, I disabled thetparallellinter because of stupid recommendations.