Enhance transaction broadcasting with advanced node and account management strategies#48
Enhance transaction broadcasting with advanced node and account management strategies#48
Conversation
…ement strategies - Implement per-account node assignment and locking mechanism - Add sophisticated gas strategy tracking with node-specific minimum gas price detection - Improve transaction signing and encoding for Cosmos SDK v0.50.x compatibility - Optimize gas limit and fee calculation with adaptive strategies - Add configuration for AtomOne chain with comprehensive node registry
WalkthroughThis pull request enhances the transaction broadcasting workflow and gas management in the system. It introduces global synchronization constructs and a node-assignment function to manage account-node relationships, refines the transaction building and signing process by using a more specific TxConfig, and improves gas strategy tracking with updates to gas-related structures and methods. A new configuration file for the AtomOne network has been added, and error handling in chain tests has been simplified by removing balance adjustment checks. Changes
Sequence Diagram(s)sequenceDiagram
participant L as Broadcast Loop
participant A as assignNodeToAccount
participant T as BuildAndSignTransaction
participant G as GasStrategyManager
participant N as Node
L->>A: Request node assignment (account address, node URL)
A-->>L: Return node assignment (if not previously assigned)
L->>T: Build and sign transaction using TxConfig
T-->>L: Return signed transaction along with gas/fee data
L->>G: Record transaction result (gas used, txGasLimit, requiredFee)
G-->>L: Update MinGasPrice if necessary
L->>N: Broadcast transaction to node
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24) ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 1
🧹 Nitpick comments (8)
broadcast/broadcast.go (3)
19-24: Consider verifying the usage pattern of global concurrency maps and locks.
Async.Mapis concurrency-safe for loads/stores, but locking it externally can restrict parallel reads/writes. If atomic check-then-set logic is needed, ensure it won’t introduce performance bottlenecks or memory growth over time.
71-82: Evaluate consolidating concurrency control with atomic operations.
Although locking around async.Mapensures atomic assignment, you could simplify this withLoadOrStore, removing the extra mutex to handle the check-then-set pattern.
151-155: Ensure fallback for missing or invalidcalculated_gas_amount.
The same concern applies here in the failure path. If the key is not present, it will default to0without warning.broadcast/gas_strategy.go (4)
15-15: Doc comment clarity.
Consider briefly mentioningMinGasPricein the comment for quicker reference.
20-20: Consider using decimal-based data type.
Storing gas price asfloat64can introduce rounding issues. Using an SDKDec-like type or an integer in micro-units might be more reliable.-type NodeGasCapabilities struct { - ... - MinGasPrice float64 - ... +type NodeGasCapabilities struct { + ... + MinGasPrice sdk.Dec // or a similar fixed-point representation + ... }
32-32: Optional: add back a brief comment.
Retaining a short description of the purpose ofRecentGasValuesmay aid newcomers.
139-150: Logic for updating MinGasPrice.
This logic correctly updates the node’s min gas price on detecting “insufficient fee”. If multi-denom support is ever needed, consider capturing different denoms instead of filtering onlyuatone.broadcast/transaction.go (1)
629-637: Fee calculation uses floating point.
MultiplyinggasPrice * float64(gasLimit)could be risky for large values or high precision. Consider usingsdk.Decor integer-based micro-denoms to avoid floating inaccuracies.- feeAmount := int64(gasPrice * float64(gasLimit)) + decGasPrice := sdk.NewDecFromInt(sdk.NewInt(int64(gasPrice * 1_000_000))).Quo(sdk.NewDec(1_000_000)) + decGasLimit := sdk.NewDec(int64(gasLimit)) + feeAmountDec := decGasPrice.Mul(decGasLimit) + feeAmount := feeAmountDec.RoundInt64()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
configurations/atomone/peerdiscovery.logis excluded by!**/*.log
📒 Files selected for processing (7)
broadcast/broadcast.go(7 hunks)broadcast/gas_strategy.go(8 hunks)broadcast/grpc.go(1 hunks)broadcast/rpc.go(1 hunks)broadcast/transaction.go(4 hunks)configurations/atomone/nodes.toml(1 hunks)modes/registry/registry.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- configurations/atomone/nodes.toml
- modes/registry/registry.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (20)
broadcast/rpc.go (1)
41-41: Switching toTxConfigis appropriate and consistent.
This ensures that the builder and signer use the correct transaction configuration, aligning with other changes in this PR.broadcast/grpc.go (1)
39-39: Consistent usage ofTxConfig
Similar to the RPC approach, passingencodingConfig.TxConfigclarifies which configuration is being used for building and signing the transaction.broadcast/broadcast.go (5)
6-6: Import ofsyncis appropriate.
This is required for the concurrency primitives introduced below.
95-108: Lock-based serialization for per-account operations is valid.
This ensures that only one goroutine at a time broadcasts transactions for a specific account, preventing sequence collisions. The approach usingLoadOrStore(txParams.AcctAddress, &sync.Mutex{})is a straightforward solution.
110-111: Logging improvements look good.
Providing node information is helpful for debugging.
135-137: Confirm error code usage for sequence mismatch.
Ensure thatresp.Code == 32actually corresponds to sequence mismatch on all target chains. If it differs, this check could lead to misleading error handling.Would you like a script to search for references to code 32 in the codebase or relevant docs to confirm its usage?
163-167: Check for missing gas parameter on success path.
Just like the failure path, confirm thatcalculated_gas_amountis reliably set to avoid unexpected zero values in your gas strategy methods.broadcast/gas_strategy.go (4)
6-6: Import usage looks fine.
This import is necessary for checking substrings (e.g., “insufficient fee”) in the failure message.
11-11: Import usage validated.
This import is required to calllib.ExtractRequiredFee(). No concerns.
79-90: Extended transaction result tracking.
The added parameterstxGasLimitandrequiredFeeexpand how transaction data is tracked. Ensure you handle empty or non-fee-related errors gracefully, sincerequiredFeemight not always have "insufficient fee" content.
277-281: Method correctness.
This conveniently exposes the node’s minimum gas price. If adopting ansdk.Dectype, ensure the return type stays consistent.broadcast/transaction.go (9)
14-24: New imports usage.
These references tobankmodule,client, andauthsigningalign well with the updated transaction-building flow.
278-278: SignerData usage.
Usingauthsigning.SignerDatawithSIGN_MODE_DIRECTis correct for SDK v0.50.x signing.
597-600: Function doc improvement.
The revised doc comment clearly reflects the new behavior of building and signing a transaction with custom encoding.
603-610: Gas limit fallback for multisend.
Fetching a pre-calculated gas inMsgParamsis useful for large multisends. The fallback of200000may still be too low for heavier scenarios; just verify or incorporate higher defaults if needed.
615-627: Minimum gas price adjustment logic.
Overriding the localgasPricewithGetMinGasPriceis sensible. Double-check you’re not missing scenarios where the config’sGas.Priceis higher than the node’s minimum.
639-677: Switch block for message creation.
Leveragingbankmodule.CreateBankSendMsgand a distributor forbank_multisendis consistent. Validate the type assertion for the distributor in tests.
679-683: Memo and timeout height.
This is straightforward and matches typical Cosmos SDK usage.
686-720: Simplified signing approach.
Creating an empty signature first, then encoding & signing the full tx is a known pattern for v0.50.x. Double-check that you’re signing the canonical bytes matchingSIGN_MODE_DIRECT.
723-723: Final transaction encoding.
Re-encoding the builder’s final transaction is the standard approach. No issues spotted.
| GetGasStrategyManager().RecordTransactionResult( | ||
| txParams.NodeURL, success, 0, txParams.MsgType, txParams.MsgParams["calculated_gas_amount"].(uint64), err.Error()) | ||
| continue |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate key existence before casting to uint64.
Casting txParams.MsgParams["calculated_gas_amount"] to uint64 without checking might cause a panic if the key is missing or if the type is different.
🏁 Script executed:
#!/bin/bash
# This script searches for potential calls to `RecordTransactionResult` where
# "calculated_gas_amount" may not be set in `txParams.MsgParams`.
rg -A 5 "RecordTransactionResult" Length of output: 1886
Action: Validate key existence of "calculated_gas_amount" before type casting
Before calling RecordTransactionResult, please ensure that the txParams.MsgParams map actually contains the calculated_gas_amount key and that its value is of type uint64. Directly casting without this check can lead to a panic if the key is missing or holds an unexpected type.
For example, update:
GetGasStrategyManager().RecordTransactionResult(
txParams.NodeURL, success, 0, txParams.MsgType, txParams.MsgParams["calculated_gas_amount"].(uint64), err.Error())
continueto something like:
gasAmount, ok := txParams.MsgParams["calculated_gas_amount"].(uint64)
if !ok {
// Handle the error case - either assign a default value or log the error
// For example:
errMsg := "calculated_gas_amount missing or not a uint64"
GetGasStrategyManager().RecordTransactionResult(
txParams.NodeURL, success, 0, txParams.MsgType, 0, errMsg)
continue
}
GetGasStrategyManager().RecordTransactionResult(
txParams.NodeURL, success, 0, txParams.MsgType, gasAmount, err.Error())
continueThis change will make the code more resilient against potential panics due to unexpected input.
1 participant
Summary by CodeRabbit
New Features
Refactor