Skip to content
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.

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

feat(core): check we recieve txs through P2P in assertoor #1610

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

rodrigo-o
Copy link
Collaborator

@rodrigo-o rodrigo-o commented Jan 3, 2025

Motivation

After the implementation of #1129, we should be receiving transactions from p2p, to check that, this PR changes the order of execution of clients in the the assertoor simulation to stop recieving txs directly from the spammer through RPC and instead getting them from other nodes and checking this continues to work after a succesfull epoch. This PR is related to #1450

@rodrigo-o
Copy link
Collaborator Author

rodrigo-o commented Jan 3, 2025

At an specific test I was able to check that it appears to work in the beginning, with our node publishing txs that should have received from p2p:

image

But later it stops, and we are the only node not publishing txs on our blocks:

image

Given the tx spammer check is later in the execution (after the blob tx check) the run fails. There are other runs where no txs are published by our node, even at the beginning, this could be related to the first nodes publishing a block and maybe to: #797

@rodrigo-o
Copy link
Collaborator Author

rodrigo-o commented Jan 6, 2025

If we want also to make sure every client receive txs from the spammer i.e. we are all communicating them correctly through p2p, we need to remove this check and change the previous matrix instead like:

diff --git a/.github/config/assertoor/el-stability-check.yaml b/.github/config/assertoor/el-stability-check.yaml
index d5b43170..bb3fe148 100644
--- a/.github/config/assertoor/el-stability-check.yaml
+++ b/.github/config/assertoor/el-stability-check.yaml
@@ -23,7 +23,7 @@ tasks:
       title: "Check if EL clients are synced"
 
 - name: run_task_matrix
-  title: "Check block proposals from all client pairs"
+  title: "Check block proposals from all client pairs with txs spammed"
-  timeout: 4m
+  timeout: 5m
   configVars:
     matrixValues: "validatorPairNames"
@@ -34,15 +34,9 @@ tasks:
       name: check_consensus_block_proposals
       title: "Wait for block proposal from ${validatorPairName}"
       configVars:
+        minTransactionCount: 240
         validatorNamePattern: "validatorPairName"
 
-- name: check_consensus_block_proposals
-  title: "Check the tx spammer is working as expected for block proposal with >= 240 transactions"
-  timeout: 3m
-  config:
-    minTransactionCount: 240
-    validatorNamePattern: "[\\d]-ethrex-lighthouse"
-
 - name: run_tasks_concurrent
   title: "Check chain stability (reorgs and forks)"
   timeout: 7m

Copy link

github-actions bot commented Jan 8, 2025

The amount of lines of code in the project has not changed.

@lambdaclass lambdaclass deleted a comment from github-actions bot Jan 8, 2025
@lambdaclass lambdaclass deleted a comment from github-actions bot Jan 8, 2025
@mpaulucci
Copy link
Collaborator

To clarify, this PR has a failing test because there are transactions that the client is not receiving through P2P. This ticket still stands: #1450

@mpaulucci
Copy link
Collaborator

This might also be related to #797

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants