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

fix(l1): made the tx-spammer work with our node #1027

Merged
merged 9 commits into from
Oct 31, 2024

Conversation

rodrigo-o
Copy link
Collaborator

Motivation

Have the tx spammer running instead of immediatly shutting down when we are the first node in the devnet setup.

Description

This PR tackled a couple of issues until we were able to process an initial transaction, (they are small changes):

  • We were returning an error on eth_getTransactionCount when we either didn't have a latest block before genesis or at any point when we don't have pending blocks. The solution in this case was returning 0x0 in those cases.
  • When requesting eth_getTransactionCount on pending after some transaction went through we were defaulting to 0x0, it appears that the idea is to default to the latest in those case which make sense.
  • There were a missing filter that made the node panic when building payloads for transactions with fees lower than the base_fee_per_gas, this generated that those kind of transactions stopped the node's ability to build blocks when they were in the mempool, we made a quick workaround in this PR, but will remove it in favor of fix(l1): add temporary fix to transaction filtering & handle tx filtering unwrap edge case #1018

Closes #1026

@rodrigo-o rodrigo-o changed the title Tx spammer and kurtosis initial params fix(l1) made the tx-spammer work with our node Oct 30, 2024
@rodrigo-o rodrigo-o changed the title fix(l1) made the tx-spammer work with our node fix(l1): made the tx-spammer work with our node Oct 30, 2024
@rodrigo-o
Copy link
Collaborator Author

This is an example of a tx going through, the caveat here is that this tx was sent at the beggining and we needed to get to the 90th block to have a base_fee lower than our gas_fee_cap:

image

gas_price was set to 5000 on those transactions and it required the network to ran for a particular amount of time before getting the base_fee below that number (we didn't had gas_fee_cap set).

    pub fn gas_fee_cap(&self) -> u64 {
        self.max_fee_per_gas().unwrap_or(self.gas_price())
    }
    
    pub fn effective_gas_tip(&self, base_fee: Option<u64>) -> Option<u64> {
        let Some(base_fee) = base_fee else {
            return Some(self.gas_tip_cap());
        };
        self.gas_fee_cap()
            .checked_sub(base_fee)
            .map(|tip| min(tip, self.gas_tip_cap()))
    }

We'll need to look into it deeper, given that I was able to run the same transactions in geth; but on our side, bypassing the check we still get an error from the VM that the gas_price is lower than the base_fee_per_gas. Here is an example of them being immediatly picked up, I started the spammer in the 13th slot.

image

@rodrigo-o
Copy link
Collaborator Author

Some other errors spotted but not resolved

nonce 0 too low:

Error on tx_spammer: Vm execution error: Invalid Transaction: nonce 0 too low, expected 240

But hitting the eth_getTransactionCount with "params": ["0xf93ee4cf8c6c40b329b0c0626f28333c132cf241", "pending" | "latest"], the answer is correct

{
	"id": 1,
	"jsonrpc": "2.0",
	"result": "0xf0"
}

EOF:

Error on tx_spammer: Post "http://127.0.0.1:50506": EOF

Logs shows an issue with an overflow outside of u64

thread 'tokio-runtime-worker' panicked at /usr/local/cargo/registry/src/index.crates.io-6f17d22bba15001f/primitive-types-0.12.2/src/lib.rs:38:1:
Integer overflow when casting to u64

This happens here in some particular transactions:

        if self.transaction.gas_price != 0 {
            highest_gas_limit = recap_with_account_balances(
                highest_gas_limit,
                &self.transaction,
                &storage,
                block_header.number,
            )?;
        }

@rodrigo-o rodrigo-o marked this pull request as ready for review October 31, 2024 13:35
@rodrigo-o rodrigo-o requested a review from a team as a code owner October 31, 2024 13:35
- 'https://raw.githubusercontent.com/lambdaclass/lambda_ethereum_rust/refs/heads/main/test_data/el-stability-check.yml'
tx_spammer_params:
tx_spammer_extra_args: ["--accounts=10", --txcount=10]
Copy link
Collaborator

Choose a reason for hiding this comment

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

these parameters are based on our current capabilities?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really, this is mostly to be able to track the execution instead of having hundreds of transactions (Which by the way we still need to check why we just add one as shown in the image). Most of the initial issues regarding the node being unable to propose weren't related to concurrency as I initially thought but to the errors mentioned in the PR description.

@rodrigo-o rodrigo-o added this pull request to the merge queue Oct 31, 2024
Merged via the queue into main with commit acd0365 Oct 31, 2024
11 checks passed
@rodrigo-o rodrigo-o deleted the tx-spammer-and-kurtosis-initial-params branch October 31, 2024 17:45
h3lio5 pushed a commit to h3lio5/lambda_ethereum_rust that referenced this pull request Oct 31, 2024
**Motivation**

Have the tx spammer running instead of immediatly shutting down when we
are the first node in the devnet setup.

**Description**

This PR tackled a couple of issues until we were able to process an
initial transaction, (they are small changes):
- We were returning an error on `eth_getTransactionCount` when we either
didn't have a `latest` block before genesis or at any point when we
don't have `pending` blocks. The solution in this case was returning
`0x0` in those cases.
- When requesting `eth_getTransactionCount` on `pending` after some
transaction went through we were defaulting to `0x0`, it appears that
the idea is to default to the `latest` in those case which make sense.
- There were a missing filter that made the node panic when building
payloads for transactions with fees lower than the base_fee_per_gas,
this generated that those kind of transactions stopped the node's
ability to build blocks when they were in the mempool, we made a quick
workaround in this PR, but will remove it in favor of lambdaclass#1018

Closes lambdaclass#1026
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.

Make the tx_spammer additional service start working with our client
2 participants