Problem: mempool nonce logic is not tested #1734
Conversation
yihuang
requested review from
devashishdxt and
thomas-nguy
and removed request for
a team
WalkthroughThis pull request introduces a new integration test function Changes
Sequence DiagramsequenceDiagram
participant User
participant Mempool
participant Blockchain
User->>Mempool: Send Transaction (Nonce 1)
User->>Mempool: Send Transaction (Nonce 2)
User->>Mempool: Send Transaction (Nonce 3)
Blockchain->>Mempool: Create New Block
Mempool-->>Blockchain: Validate Transactions
User->>Mempool: Send Additional Transactions
Blockchain->>Mempool: Check Nonce Sequence
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
integration_tests/test_mempool.py (6)
12-12: Remove unused import.The
send_transactionimport is not used in the code.- send_transaction,🧰 Tools
🪛 Ruff (0.8.2)
12-12:
.utils.send_transactionimported but unusedRemove unused import:
.utils.send_transaction(F401)
104-114: Consider adding transaction success verification.The helper function should verify that the transaction was successfully included in a block.
def send_with_nonce(nonce): tx = { "to": ADDRS["community"], "value": 1, "gas": 4121000, "data": "0x" + "00" * tx_bytes, "nonce": nonce, } signed = sign_transaction(w3, tx, KEYS["validator"]) txhash = w3.eth.send_raw_transaction(signed.rawTransaction) + receipt = w3.eth.wait_for_transaction_receipt(txhash) + assert receipt.status == 1, f"Transaction {txhash.hex()} failed" return txhash
108-108: Consider extracting magic number.The gas limit
4121000should be defined as a constant with a descriptive name.+GAS_LIMIT_LARGE_TX = 4121000 # Gas limit for transactions with large data payload + def send_with_nonce(nonce): tx = { "to": ADDRS["community"], "value": 1, - "gas": 4121000, + "gas": GAS_LIMIT_LARGE_TX,
116-120: Simplify loop using range object.The loop control variable 'i' is not used in the loop body.
- for i in range(3): + for _ in range(3):🧰 Tools
🪛 Ruff (0.8.2)
116-116: Loop control variable
inot used within loop body(B007)
121-123: Format expression with consistent spacing.The expression
new_height-heightneeds consistent spacing around the operator.- assert orig_nonce + (new_height-height) == w3.eth.get_transaction_count(sender) + assert orig_nonce + (new_height - height) == w3.eth.get_transaction_count(sender)🧰 Tools
🪛 GitHub Check: Lint python
[failure] 122-122:
./integration_tests/test_mempool.py:122:36: BLK100 Black would make changes.
92-92: Split long line for better readability.The line exceeds the 88-character limit.
- then we'll try to send a new tx with local nonce to see if it still get accepted even if + then we'll try to send a new tx with local nonce to see if it still + get accepted even if🧰 Tools
🪛 GitHub Check: Lint python
[failure] 92-92:
./integration_tests/test_mempool.py:92:89: E501 line too long (92 > 88 characters)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration_tests/test_mempool.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
integration_tests/test_mempool.py
12-12: .utils.send_transaction imported but unused
Remove unused import: .utils.send_transaction
(F401)
116-116: Loop control variable i not used within loop body
(B007)
🪛 GitHub Check: Lint python
integration_tests/test_mempool.py
[failure] 92-92:
./integration_tests/test_mempool.py:92:89: E501 line too long (92 > 88 characters)
[failure] 122-122:
./integration_tests/test_mempool.py:122:36: BLK100 Black would make changes.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (unmarked)
🔇 Additional comments (2)
integration_tests/test_mempool.py (2)
86-94: Well-documented test scenario.The docstring clearly explains the test's purpose and methodology.
🧰 Tools
🪛 GitHub Check: Lint python
[failure] 92-92:
./integration_tests/test_mempool.py:92:89: E501 line too long (92 > 88 characters)
86-133: Comprehensive test coverage for mempool nonce handling.The test effectively verifies:
- Mempool acceptance of transactions with future nonces
- Proper nonce tracking after block creation
- Transaction ordering based on nonces
🧰 Tools
🪛 Ruff (0.8.2)
116-116: Loop control variable
inot used within loop body(B007)
🪛 GitHub Check: Lint python
[failure] 92-92:
./integration_tests/test_mempool.py:92:89: E501 line too long (92 > 88 characters)
[failure] 122-122:
./integration_tests/test_mempool.py:122:36: BLK100 Black would make changes.
Signed-off-by: yihuang <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
integration_tests/test_mempool.py (6)
91-94: Format long lines in docstring.The docstring lines exceed the 88-character limit. Consider wrapping them for better readability.
- then we'll try to send a new tx with local nonce to see if it still get accepted even if check-tx state get reset. + then we'll try to send a new tx with local nonce to see if it still get accepted + even if check-tx state get reset. - the expected behavior is when mempool.recheck=true, this test should pass, because although check-tx state get reset when new blocks generated, but recheck logic will bring it back in sync with pending txs, so the client can keep sending new transactions with local nonce. + the expected behavior is when mempool.recheck=true, this test should pass, because + although check-tx state get reset when new blocks generated, but recheck logic will + bring it back in sync with pending txs, so the client can keep sending new + transactions with local nonce.🧰 Tools
🪛 GitHub Check: Lint python
[failure] 91-91:
./integration_tests/test_mempool.py:91:89: E501 line too long (92 > 88 characters)
[failure] 94-94:
./integration_tests/test_mempool.py:94:89: E501 line too long (276 > 88 characters)
103-103: Enhance comment clarity for tx_bytes.The comment could be more explicit about why this specific size ensures one transaction per block.
- tx_bytes = 1000000 # can only include one tx at a time + tx_bytes = 1000000 # Size large enough to fill block gas limit, ensuring only one tx per block
105-116: Consider using dynamic gas estimation.The function uses a hardcoded gas value which might be insufficient if the transaction data size or network conditions change.
def send_with_nonce(nonce): tx = { "to": ADDRS["community"], "value": 1, - "gas": 4121000, "data": "0x" + "00" * tx_bytes, "nonce": nonce, } + tx["gas"] = w3.eth.estimate_gas(tx) signed = sign_transaction(w3, tx, KEYS["validator"]) txhash = w3.eth.send_raw_transaction(signed.rawTransaction) return txhash
117-125: Consider usingrange(3)more idiomatically.The loop control variable
iis not used within the loop body. Consider using_to indicate an unused variable.- for i in range(3): + for _ in range(3):🧰 Tools
🪛 Ruff (0.8.2)
117-117: Loop control variable
inot used within loop body(B007)
🪛 GitHub Check: Lint python
[failure] 123-123:
./integration_tests/test_mempool.py:123:36: BLK100 Black would make changes.
123-123: Format assertions consistently.Add spaces around the minus operator in the assertions for better readability.
- assert orig_nonce + (new_height-height) == w3.eth.get_transaction_count(sender) + assert orig_nonce + (new_height - height) == w3.eth.get_transaction_count(sender)Also applies to: 133-133
🧰 Tools
🪛 GitHub Check: Lint python
[failure] 123-123:
./integration_tests/test_mempool.py:123:36: BLK100 Black would make changes.
85-134: Consider adding error case tests.The test effectively validates the happy path, but could benefit from additional test cases:
- Test with out-of-order nonces
- Test with duplicate nonces
- Test with gaps in nonce sequence
Would you like me to help implement these additional test cases?
🧰 Tools
🪛 Ruff (0.8.2)
117-117: Loop control variable
inot used within loop body(B007)
🪛 GitHub Check: Lint python
[failure] 91-91:
./integration_tests/test_mempool.py:91:89: E501 line too long (92 > 88 characters)
[failure] 94-94:
./integration_tests/test_mempool.py:94:89: E501 line too long (276 > 88 characters)
[failure] 123-123:
./integration_tests/test_mempool.py:123:36: BLK100 Black would make changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration_tests/test_mempool.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
integration_tests/test_mempool.py
117-117: Loop control variable i not used within loop body
(B007)
🪛 GitHub Check: Lint python
integration_tests/test_mempool.py
[failure] 91-91:
./integration_tests/test_mempool.py:91:89: E501 line too long (92 > 88 characters)
[failure] 94-94:
./integration_tests/test_mempool.py:94:89: E501 line too long (276 > 88 characters)
[failure] 123-123:
./integration_tests/test_mempool.py:123:36: BLK100 Black would make changes.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (unmarked)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1734 +/- ##
==========================================
+ Coverage 16.87% 17.10% +0.23%
==========================================
Files 72 74 +2
Lines 6163 6184 +21
==========================================
+ Hits 1040 1058 +18
- Misses 5000 5002 +2
- Partials 123 124 +1 |
Signed-off-by: yihuang <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
integration_tests/test_mempool.py (3)
107-107: Document the rationale for the transaction size.Add a comment explaining why 1MB was chosen as the transaction size. This helps future maintainers understand the test's constraints.
- tx_bytes = 1000000 # can only include one tx at a time + # Set transaction size to 1MB to ensure only one transaction can be included per block + # due to block gas limit constraints + tx_bytes = 1000000
113-113: Consider using dynamic gas estimation.Instead of hardcoding the gas value, consider using
w3.eth.estimate_gas(tx)to make the test more resilient to network parameter changes.
121-124: Address unused loop variable.The loop control variable 'i' is unused. Consider using a clearer construct.
- for i in range(3): + for _ in range(3):🧰 Tools
🪛 Ruff (0.8.2)
121-121: Loop control variable
inot used within loop body(B007)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration_tests/test_mempool.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
integration_tests/test_mempool.py
121-121: Loop control variable i not used within loop body
(B007)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (unmarked)
🔇 Additional comments (2)
integration_tests/test_mempool.py (2)
85-99: LGTM! Well-documented test function.The docstring clearly explains the test's purpose, strategy, and expected behavior. The flaky test decorator with max_runs=3 is appropriate given the nature of mempool tests.
136-139: Strengthen transaction inclusion verification.The current check might be flaky as transactions could be included in subsequent blocks. Consider implementing a more robust verification:
- Wait for transaction receipt instead of just new blocks
- Verify transaction inclusion in blocks explicitly
Here's a script to analyze the transaction inclusion patterns in the codebase:
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit