fix(l1): add temporary fix to transaction filtering & handle tx filtering unwrap edge case #1018
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fkrause98
approved these changes
Oct 30, 2024
mpaulucci
approved these changes
Oct 30, 2024
rodrigo-o
reviewed
Oct 30, 2024
rodrigo-o
approved these changes
Oct 30, 2024
ilitteri
changed the title
effective_gas_tip computation
fmoletta
reviewed
Oct 30, 2024
fmoletta
reviewed
Oct 30, 2024
crates/blockchain/mempool.rs
Outdated
Comment on lines
83
to
88
| // This is a temporary fix to avoid invalid transactions to be included. | ||
| // This should be removed once https://github.com/lambdaclass/ethereum_rust/issues/680 | ||
| // is addressed. | ||
| if tx.effective_gas_tip(filter.base_fee).is_none() { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
We could add this check on the else block of the min_tip check, that way we can leave the check even when the issue is fixed
There was a problem hiding this comment.
But, this should be an else if right? We still need to know if effective_gas_tip returned None, or am I missing something?
There was a problem hiding this comment.
As I understood it, yes, we still need to check for the None of effective_gas_tip even in the else of let Some(min_tip) = filter.min_tip
There was a problem hiding this comment.
What I mean is that with this change we have:
check effective fee != None
if min_tip:
check effective fee != None && > min_tip
When we could have:
if min_tip:
check effective fee != None && > min_tip
else:
check effective fee != None
That way we only compute the effective fee once in any case
Co-authored-by: fmoletta <[email protected]>
mpaulucci
reviewed
Oct 31, 2024
crates/blockchain/error.rs
Outdated
| #[error( | ||
| "Attempted to add an invalid transaction to the block. The transaction filter must have failed." | ||
| )] | ||
| AttemptedToAddInvalidTransaction, |
There was a problem hiding this comment.
the error is a bit long. Why not just InvalidTransaction?
There was a problem hiding this comment.
Not a problem, renaming it asap.
Generalize the error to InvalidTransaction which can be raised with additional context.
fmoletta
approved these changes
Oct 31, 2024
github-merge-queue bot
pushed a commit
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 #1018 Closes #1026
h3lio5
pushed a commit
to h3lio5/lambda_ethereum_rust
that referenced
this pull request
Oct 31, 2024
…ring unwrap edge case (lambdaclass#1018) **Motivation** <!-- Why does this pull request exist? What are its goals? --> To avoid panicking when the transaction filter fails to filter a transaction because we do not yet filter transactions with invalid tip. **Description** - Filter transactions whose effective gas tip calculation underflowed. - Handle the unwrap of the `effective_gas_tip` calculation. Even though this is an edge case that shouldn't happen, it is better to have a descriptive error if it happens someday than to panic. --------- Co-authored-by: fmoletta <[email protected]>
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
To avoid panicking when the transaction filter fails to filter a transaction because we do not yet filter transactions with invalid tip.
Description
effective_gas_tipcalculation. Even though this is an edge case that shouldn't happen, it is better to have a descriptive error if it happens someday than to panic.