Problem: concurrent send_tx shares web3 instance #1562
Conversation
it might limit the concurrent performance
yihuang
requested review from
mmsqe and
devashishdxt
and removed request for
a team
WalkthroughThe changes modify the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant sendtx
participant Web3
participant EthereumNode
User->>sendtx: Call sendtx(acct, tx_amount)
sendtx->>Web3: Initialize connection to http://localhost:8545
Web3->>EthereumNode: Connect to Ethereum node
sendtx->>Web3: Retrieve chain_id
Web3->>sendtx: Return chain_id
sendtx->>EthereumNode: Send transaction with chainId
EthereumNode-->>sendtx: Confirm transaction
sendtx-->>User: Return transaction result
Poem
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: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- testground/benchmark/benchmark/sendtx.py (2 hunks)
Additional comments not posted (1)
testground/benchmark/benchmark/sendtx.py (1)
55-55: LGTM!The inclusion of the
chainIdin the transaction dictionary is correct and necessary for proper transaction validation and execution.The code changes are approved.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1562 +/- ##
=======================================
Coverage 15.24% 15.24%
=======================================
Files 67 67
Lines 4874 4874
=======================================
Hits 743 743
Misses 4037 4037
Partials 94 94 |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- testground/benchmark/benchmark/sendtx.py (3 hunks)
Additional comments not posted (2)
testground/benchmark/benchmark/sendtx.py (2)
34-35: Avoid reinitializing the web3 instance inside the function.Reinitializing the
web3.Web3instance inside thesendtxfunction can lead to performance issues and unnecessary overhead. Instead, pass thew3instance as an argument to the function and use it directly.Remove the reinitialization of the
w3instance:-def sendtx(acct: Account, tx_amount: int): - w3 = web3.Web3(web3.providers.HTTPProvider("http://localhost:8545")) +def sendtx(w3: web3.Web3, acct: Account, tx_amount: int):
36-36: Ensure the chain ID is retrieved only once.Retrieving the
chain_idmultiple times can lead to inconsistencies. Ensure it is retrieved only once and reused throughout the function.Move the
chain_idretrieval outside the function if it is used in multiple places:-def sendtx(acct: Account, tx_amount: int): - chain_id = w3.eth.chain_id +def sendtx(w3: web3.Web3, acct: Account, tx_amount: int, chain_id: int):
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- testground/benchmark/benchmark/sendtx.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- testground/benchmark/benchmark/sendtx.py
|
fixed with asyncio: #1587 |
it might limit the concurrent performance
Solution:
it seems improve a bit locally here, can you try to reproduce the improvement? @mmsqe
👮🏻👮🏻👮🏻 !!!! 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
New Features
Bug Fixes