[Fix] Wait for chain head and eth "latest" in createAndFinalizeBlock()#1855
[Fix] Wait for chain head and eth "latest" in createAndFinalizeBlock()#1855
createAndFinalizeBlock()#1855Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe utility now derives the block hash from Changes
Sequence Diagram(s)sequenceDiagram
participant TestUtil as Test Util
participant Chain as Chain Service
participant EthRPC as Ethereum RPC
TestUtil->>Chain: submit tx / create block -> receive `response.result.hash`
TestUtil->>Chain: poll `chain_getHeader(blockHash)` until `header.number` present (<=10s)
alt header.number available
Chain-->>TestUtil: header (hex `number`)
TestUtil->>TestUtil: parse header.number -> expectedNumber
TestUtil->>EthRPC: poll `eth_blockNumber` until >= expectedNumber (<=10s)
EthRPC-->>TestUtil: eth_blockNumber >= expectedNumber
else timeout / errors
Chain-->>TestUtil: headerLastError
EthRPC-->>TestUtil: rpcLastError
TestUtil-->>TestUtil: throw error including last observed errors
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ts-tests/tests/util.ts`:
- Around line 102-107: The loop that polls eth_blockNumber uses strict equality
which can miss cases where the node advanced past the expected block; in the
polling logic that calls customRequest and computes n from current.result (using
rpcStart, rpcSyncTimeout, expectedNumber), change the comparison from n ===
expectedNumber to n >= expectedNumber so the function returns when the chain has
reached or passed the expected block height; keep the rest of the loop and
timeout behavior unchanged.
- Around line 91-93: The timeout path currently only checks that head.number is
present, not that it advanced beyond prevNumber; update the conditional in the
timeout/check block that uses head and prevNumber (the one after createBlock) to
assert that head.number > prevNumber (or compare as BigInt/Number if head.number
is string) and throw the existing error if it has not advanced, ensuring the
test waits for an increased head rather than merely a truthy value.
librelois
left a comment
librelois
left a comment
There was a problem hiding this comment.
New polling loops fail fast on transient transport errors instead of retrying, which can make createAndFinalizeBlock flakier under brief RPC hiccups.
Evidence: both new loops call customRequest(...) without try/catch at ts-tests/tests/util.ts (line 84) and ts-tests/tests/util.ts (line 104); customRequest rejects on provider send errors at ts-tests/tests/util.ts (line 19).
Impact: a single transient provider error now aborts the helper, whereas surrounding retry logic suggests this path is expected to be eventually consistent.
|
@librelois |
librelois
left a comment
librelois
left a comment
There was a problem hiding this comment.
createAndFinalizeBlock can still return before the newly created block is indexed if the pre-call head snapshot is stale.
Evidence: ts-tests/tests/util.ts (line 71) ts-tests/tests/util.ts (line 89), ts-tests/tests/util.ts (line 104).
Why: the logic only checks chain_getHeader > prevNumber. If prevNumber was already behind real tip, the loop can break on an older block (not the one from this engine_createBlock), and subsequent waits validate the wrong height.
librelois
left a comment
librelois
left a comment
There was a problem hiding this comment.
createAndFinalizeBlock no longer explicitly waits for eth_getBlockByNumber visibility before returning, despite its comment saying it does.
Evidence: ts-tests/tests/util.ts lines 68-71 and 104-125.
Risk: returning on eth_blockNumber >= expectedNumber can still race with mapping-sync availability, causing intermittent failures in tests that immediately read block details.
Suggestion:
Re-introduce an explicit mapping-sync wait before return (for example waitForBlock(web3, "0x" + expectedNumber.toString(16), 10_000)), or prove/document that eth_blockNumber is strictly gated on mapping-sync in this stack.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ts-tests/tests/util.ts`:
- Around line 74-77: The code currently assumes response is non-null and
accesses response.result, which will throw if customRequest returned undefined;
update the guard to check response itself and the nested hash (e.g. if
(!response || !response.result?.hash) { throw new Error(`Unexpected response:
${JSON.stringify(response)}`); }) and then assign blockHash from
response.result.hash (const blockHash = response.result.hash as string) so the
intended error path is used instead of an uncaught property access error.
Refactor ts-tests/tests/test-contract-storage.ts to remove timing-sensitive transaction flow in the SSTORE gas-cost test. Replace callback-based contract.methods().send() sequencing with signed raw tx submission. Seal a block explicitly after each tx and wait for receipt availability with bounded polling. Add a per-test timeout (30s) and remove unused helper/wallet setup. Keep existing gas-cost assertions unchanged; only test execution reliability is improved.
…()` (polkadot-evm#1855) * createAndFinalizeBlock() must wait for the target block * Coderabbit review * Wrap customRequest() in try-catch * Get blockNumber from engine_createBlock result.hash * Get waitForBlock() back * test(ts-tests): make contract storage SSTORE test deterministic Refactor ts-tests/tests/test-contract-storage.ts to remove timing-sensitive transaction flow in the SSTORE gas-cost test. Replace callback-based contract.methods().send() sequencing with signed raw tx submission. Seal a block explicitly after each tx and wait for receipt availability with bounded polling. Add a per-test timeout (30s) and remove unused helper/wallet setup. Keep existing gas-cost assertions unchanged; only test execution reliability is improved. * Make test-selfdestruct deterministic --------- Co-authored-by: Eloïs <c@elo.tf>
…kadot-evm#1855) Cherry-pick of upstream polkadot-evm#1855
2 participants
Summary
Fixes flaky ts-tests (e.g. test-block, receipt/latest-block consistency) caused by
createAndFinalizeBlock()returning before the new block was visible on the eth RPC.Background
The node exposes "latest" and
eth_blockNumberas the latest indexed canonical block (mapping-sync), which can lag behind the chain head. If we only wait for the block by number and don’t ensure the chain head has advanced and that "latest" has caught up, tests can observe the previous block (e.g.getBlockNumber() === 1instead of 2) and fail or become flaky.Changes
createAndFinalizeBlock()prevNumberfromchain_getHeaderbeforeengine_createBlock.chain_getHeaderuntil the head number is >prevNumber(10s timeout, 50ms interval) instead of a single read, so we don’t rely on a possibly stale head.waitForBlock(web3, head.number, 10_000)so the block is indexed and visible viaeth_getBlockByNumber.eth_blockNumberuntil it equals the new block number (10s timeout), then return, so callers that use "latest" orgetBlockNumber()see the block we just created.Timeouts for head advance,
waitForBlock, andeth_blockNumbersync are set to 10s to stay stable when the full test suite runs under load.Testing
npm run test-sqlshould pass consistently when run in isolation and as part of the full suite.