fix: eth_getTransactionByHash + eth_getTransactionReceipt fixes #9933
Conversation
Hardhat doesn't like logs: null in transaction receipts when no logs were emitted. Wants empty array. Size logsBloom correctly.
raulk
changed the title
| // require that the hashes are identical | ||
| require.Equal(t, hash, mpoolTx.Hash) | ||
|
|
||
| // these fields should be nil because the tx hasn't landed on chain. |
There was a problem hiding this comment.
Yeah, let's not play this game, we're inviting flakiness here.
Can we just try to lookup the receipt before making these assertions? If found, let's simply skip them (with a log saying that we're doing so).
There was a problem hiding this comment.
This is actually a pretty important assertion (we had bugs here before, so want to lock these for regression testing). I'm onboard with deflaking, as long as we don't withdraw coverage we need.
There was a problem hiding this comment.
The Miners in the itest framework have pause and unpause functions exposed, but we should implement a function for the test kit that allows us to pause and unpause all mining.
| blockTime := 100 * time.Millisecond | ||
| // reasonable blocktime so that the tx sits in the mpool for a bit during the test. | ||
| // although this is non-deterministic... | ||
| blockTime := 1 * time.Second |
There was a problem hiding this comment.
I'd consider increasing this -- this test isn't really constrained by epochs (needs ~3 epochs), so even if the blocktime were like 5 seconds, it would still finish pretty quickly
There was a problem hiding this comment.
Plus the boot up time for the chain (around 10 epochs).
| var receipt *api.EthTxReceipt | ||
| for i := 0; i < 10000000000; i++ { | ||
| for i := 0; i < 20; i++ { |
There was a problem hiding this comment.
Let's just actually StateWaitMsg here instead of this funkiness -- if we don't have the tooling to easily get the msgCid, slap a TODO on it for now (I think the work @geoff-vball is doing will make this easy to do).
There was a problem hiding this comment.
We do need to check that the receipt is null while the transaction hasn't landed on chain -- it's part of the test's coverage.
There was a problem hiding this comment.
Sure, keep doing that every 500ms, but the cue to stop is StateWaitMsg returning (not "i waited 10 seconds").
There was a problem hiding this comment.
We still need a timeout for the test (should not let it run indefinitely). We could have a timed context with StateWaitMsg, but at that point we're achieving the same effect as here with more complicated code, so it's a matter of choice.
|
In case you're wondering if I'm making up the how this API should behave, I am not. Sources include: |
Related Issues
Proposed Changes
Fix these issues and add coverage in FEVM integration tests.
Additional Info
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>fix: mempool: Introduce a cache for valid signaturesPR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, testarea, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps