Skip to content

Wait for transaction receipt if instant seal is enabled #9914

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 15 commits into from
Oct 10, 2025
Merged

Wait for transaction receipt if instant seal is enabled #9914

merged 15 commits into from
Oct 10, 2025

Conversation

@marian-radu
Copy link
Contributor

@marian-radu marian-radu commented Oct 2, 2025

Fixes paritytech/contract-issues#165

The main changes in this PR are:

  1. Add a new API to revive-dev-node to check whether the node has instant seal enabled.
  2. Add a new debug API to eth-rpc to check whether the node has instant seal enabled. (optional)
  3. Query and cache the node’s instant seal status during eth-rpc initialization.
  4. If instant seal is enabled, wait for the transaction receipt to be available

@marian-radu
Wait for transaction receipt if instant seal is enabled.
055a231 
Add a new revive-dev-node RPC API to check whether the node has instant seal enabled. The query is performed once during client initialization.
@marian-radu
Merge remote-tracking branch 'origin/master' into 165-ethena-hardhatc…
f7f6f03 
…haimatchersassertionerror-assertion-error-receipt-should-not-be-null
@marian-radu marian-radu requested a review from pgherveou October 2, 2025 14:56
pgherveou
pgherveou reviewed Oct 3, 2025
View reviewed changes
Copy link
Contributor

@pgherveou pgherveou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quick review, will look in more details when I get back

@pgherveou pgherveou mentioned this pull request Oct 7, 2025
Closed
@marian-radu
Merge remote-tracking branch 'origin/master' into 165-ethena-hardhatc…
e9f1b34 
…haimatchersassertionerror-assertion-error-receipt-should-not-be-null
@marian-radu
Address review comments.
d9eada1
@marian-radu marian-radu requested a review from pgherveou October 8, 2025 12:24
@marian-radu marian-radu force-pushed the 165-ethena-hardhatchaimatchersassertionerror-assertion-error-receipt-should-not-be-null branch from 2d61ff6 to 004f8c5 Compare October 8, 2025 12:43
pgherveou
pgherveou reviewed Oct 9, 2025
View reviewed changes
pgherveou
pgherveou reviewed Oct 9, 2025
View reviewed changes
Copy link
Contributor

@pgherveou pgherveou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would update the code to do the following:

  • first as mentioned in comment, the broadcaster should be internal to the client.
  • to have no perf impact on production setup we can also set it to optional and only set it to some if the automine is enabled

pgherveou
pgherveou reviewed Oct 9, 2025
View reviewed changes
pgherveou
pgherveou reviewed Oct 9, 2025
View reviewed changes
pgherveou
pgherveou reviewed Oct 9, 2025
View reviewed changes
@marian-radu
Copy link
Contributor Author

marian-radu commented Oct 9, 2025

/cmd prdoc --audience runtime_dev --bump patch

@marian-radu marian-radu added the T7-smart_contracts This PR/Issue is related to smart contracts. label Oct 9, 2025
@marian-radu marian-radu marked this pull request as ready for review October 9, 2025 13:19
@marian-radu
eth-rpc: Handle RecvError::Lagged, remove unused ClientError, fix clippy
740f5f6 
- Treat broadcast::RecvError::Lagged appropriately in wait_for_transaction_to_be_mined.
- Remove unused ClientError variant.
- Fix clippy warnings.
@marian-radu marian-radu requested a review from athei October 9, 2025 14:25
athei
athei reviewed Oct 9, 2025
View reviewed changes
pgherveou
pgherveou reviewed Oct 9, 2025
View reviewed changes
marian-radu and others added 2 commits October 9, 2025 20:51
@marian-radu
Address review comments.
4869a7c
@pgherveou
Simplifications
acfabc1 
I think it's good enough to wait for the next block, no need to check if
the actual transaction has been mined, this should be enough for test
framework that assume that the next block is mined when eth_sendRawTransaction returns
pgherveou
pgherveou approved these changes Oct 9, 2025
View reviewed changes
Copy link
Contributor

@pgherveou pgherveou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some change in the last commit,
as explained in the commit, it's good enough to just wait for the next block to be mined, this simplify the code for this feature.

Take a look see if I haven't done anything stupid

@marian-radu
Copy link
Contributor Author

marian-radu commented Oct 10, 2025 •
edited
Loading

Made some change in the last commit, as explained in the commit, it's good enough to just wait for the next block to be mined, this simplify the code for this feature.

Take a look see if I haven't done anything stupid

@pgherveou

I think the latest solution offers fewer guarantees compared to the initial one, but it is indeed simpler. We need to decide whether this will be good enough for the testing framework.

In the initial solution, a successful result from wait_for_tx method guaranteed that the transaction was included in a block, while the latest solution only ensures that a new block was mined, which may not contain the transaction.

Sequence of events that leads to a "false positive":

  1. ETH-RPC: Receives Transaction_1 and executes: subscribe_to_new_blocks, submit_transaction and wait_for_new_blocks
  2. NODE: Executes Transaction_1, mines BLOCK_1 and sends a notification to eth-rpc process
  3. ETH-RPC: Receives Transaction_2 and executes: subscribe_to_new_blocks, submit_transaction and wait_for_new_blocks
  4. ETH-RPC: Receives the 'BLOCK_1 was mined' notification and unblocks both transactions, even though Transaction_2 is not part of this block. This may or may not be an issue, depending on the test and how long it takes for Transaction_2's receipt to reach the ETH-RPC DB.

@athei
Copy link
Member

athei commented Oct 10, 2025

We are just talking about a few additional lines of code in a single function that is only executed during testing. I'd argue in this case we take the more complex solution in order to gain the benefits.

@pgherveou
Copy link
Contributor

pgherveou commented Oct 10, 2025

I would argue that this is good enough
Testing framework won't send transactions concurrently they will always await for the response before sending the next one.

but majority wins so pushed a new commit, @marian-radu please review

@marian-radu
Copy link
Contributor Author

marian-radu commented Oct 10, 2025

LGTM!

@paritytech-workflow-stopper
Copy link

paritytech-workflow-stopper bot commented Oct 10, 2025

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/18406839335
Failed job name: test-linux-stable-runtime-benchmarks

@pgherveou pgherveou requested a review from athei October 10, 2025 13:58
@athei
Merge branch 'master' into 165-ethena-hardhatchaimatchersassertionerr…
1d7fb42 
…or-assertion-error-receipt-should-not-be-null
@pgherveou pgherveou enabled auto-merge October 10, 2025 15:36
@pgherveou pgherveou added this pull request to the merge queue Oct 10, 2025
auto-merge was automatically disabled October 10, 2025 16:23

Pull Request is not mergeable

Merged via the queue into master with commit 1cbdb4d Oct 10, 2025
280 of 287 checks passed
@pgherveou pgherveou deleted the 165-ethena-hardhatchaimatchersassertionerror-assertion-error-receipt-should-not-be-null branch October 10, 2025 16:44
alvicsam pushed a commit that referenced this pull request Oct 17, 2025
@marian-radu @github-actions
@pgherveou @athei @alvicsam
Wait for transaction receipt if instant seal is enabled (#9914)
b94a533 
Fixes paritytech/contract-issues#165

The main changes in this PR are:

1. Add a new API to revive-dev-node to check whether the node has
instant seal enabled.
2. Add a new debug API to eth-rpc to check whether the node has instant
seal enabled. (optional)
3. Query and cache the node’s instant seal status during eth-rpc
initialization.
4. If instant seal is enabled, wait for the transaction receipt to be
available

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: pgherveou <[email protected]>
Co-authored-by: Alexander Theißen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pgherveou pgherveou pgherveou approved these changes

@athei athei athei approved these changes

Assignees

No one assigned

Labels

T7-smart_contracts This PR/Issue is related to smart contracts.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Ethena] HardhatChaiMatchersAssertionError: Assertion error: receipt should not be null

4 participants

@marian-radu @athei @pgherveou