-
Notifications
You must be signed in to change notification settings - Fork 278
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
refactor: index submitted txs in tx client and improve nonce management #3830
base: main
Are you sure you want to change the base?
Conversation
3e4e1d1
to
faa460c
Compare
WalkthroughWalkthroughThe changes introduce a new struct, Changes
Assessment against linked issues
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
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
square size test started failing, investigating that.
pkg/user/tx_client.go
Outdated
@@ -137,6 +148,9 @@ type TxClient struct { | |||
defaultGasPrice float64 | |||
defaultAccount string | |||
defaultAddress sdktypes.AccAddress | |||
txPool map[string]poolTxInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Question] do we want to have a way of periodically clearing the cache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evan-forbes @cmwaters I'd appreciate your input here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally, it seems like we should never be a scenario where we need this to be cleared. Either a tx is included and we continually try to get it included, or we handle each case when that does not occur and remove the tx at that point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only time tx doesn't get removed from this local cache is if context gets cancelled while polling for tx status which means that the said tx was never confirmed. If the transaction doesn't get confirmed it can linger in that cache until it ooms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by periodically, do we mean a ttl?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scenario that I see as more problematic is if a user decides to use BroadcastTx which creates the map entry and doesn't use ConfirmTx which cleans it up (because maybe they don't care if it lands or not).
I think we should garbage collect, but not as a separate go routine but within BroadcastTx.
I would give at least 10 minutes before we clean up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consistently use the term txTracker instead of txTracker in some places and local pool in other places.
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- pkg/user/pruning_test.go (1 hunks)
- pkg/user/tx_client.go (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- pkg/user/tx_client.go
Additional comments not posted (1)
pkg/user/pruning_test.go (1)
11-51
: Excellent test coverage for the transaction tracker pruning functionality!The test is well-structured and covers the essential aspects of pruning:
- It sets up a controlled environment with transactions of different ages.
- It calls the
pruneTxTracker
method to trigger the pruning process.- It verifies the expected state of the transaction tracker after pruning, ensuring that only transactions older than the pruning threshold are removed.
The test provides confidence in the correctness of the pruning implementation.
txClient.pruneTxTracker() | ||
// Prunes the transactions that are 10 minutes old | ||
// 5 transactions will be pruned | ||
require.Equal(t, txTrackerBeforePruning-txsToBePruned, txsToBePruned) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the assertion by swapping the expected and actual values.
The assertion on line 48 compares the wrong values. It should compare the difference between the tracker size before pruning and the number of transactions to be pruned with the actual size of the tracker after pruning.
Swap the expected and actual values in the assertion:
-require.Equal(t, txTrackerBeforePruning-txsToBePruned, txsToBePruned)
+require.Equal(t, txsToBePruned, txTrackerBeforePruning-txsToBePruned)
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
require.Equal(t, txTrackerBeforePruning-txsToBePruned, txsToBePruned) | |
require.Equal(t, txsToBePruned, txTrackerBeforePruning-txsToBePruned) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Would be nice to apply this comment of coderabbit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes #3768
The PR description will close that issue when this PR merges which doesn't seem like what you want. Since #3768 is large in scope, it is not immediately obvious which parts of that issue this PR addresses. One idea is to convert that issue into a tracking issue with several smaller issues. Then this PR can close one of the smaller issues and leave the remaining ones open.
} | ||
numTransactions := 10 | ||
|
||
// Add 10 transactions to the tracker that are 10 minutes old |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment seems incorrect. It looks like the implementation below adds 5 transactions that are 10 minutes old and 5 transactions that are 5 minutes old.
// Add 10 transactions to the tracker that are 10 minutes old |
The 'evictions issue' became its own epic which we discussed during the offsite. I can update #3768 to be more accurate to what this pr resolves and the other issues will follow shortly. |
Created a new issue that this pr directly resolves #3899 i decided to keep the old one for context. Me and Callum can groom it/close it/delete it when we pm best txs stuff. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
txClient.pruneTxTracker() | ||
// Prunes the transactions that are 10 minutes old | ||
// 5 transactions will be pruned | ||
require.Equal(t, txTrackerBeforePruning-txsToBePruned, txsToBePruned) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Would be nice to apply this comment of coderabbit.
Overview
Fixes #3899