-
Notifications
You must be signed in to change notification settings - Fork 88
feat: adds RLP hex txn storage to tx pool service #4521
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
base: main
Are you sure you want to change the base?
Conversation
|
Haven’t gone through this in depth yet, but it looks like we’re changing the design of the txPool implementation. To avoid adding new changes on top of moving parts, I suggest holding off on merging this into the feature branch until the feature branch is finalized and merged into main first. |
aab2f1e to
f908c27
Compare
e57963a to
d2f61af
Compare
Test Results 20 files ±0 269 suites ±0 30m 27s ⏱️ + 8m 52s For more details on these failures, see this check. Results for commit 5928e28. ± Comparison against base commit 8a7783c. ♻️ This comment has been updated with latest results. |
konstantinabl
left a comment
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.
A comment and a few questions
packages/relay/src/lib/services/transactionPoolService/LocalPendingTransactionStorage.ts
Show resolved
Hide resolved
packages/relay/src/lib/services/transactionPoolService/LocalPendingTransactionStorage.ts
Outdated
Show resolved
Hide resolved
quiet-node
left a comment
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.
I think we should have a sync session to discuss this as a team
packages/relay/src/lib/services/transactionPoolService/LocalPendingTransactionStorage.ts
Outdated
Show resolved
Hide resolved
packages/relay/src/lib/services/transactionPoolService/LocalPendingTransactionStorage.ts
Show resolved
Hide resolved
packages/relay/src/lib/services/transactionPoolService/LocalPendingTransactionStorage.ts
Show resolved
Hide resolved
packages/relay/src/lib/services/transactionPoolService/RedisPendingTransactionStorage.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Simeon Nakov <[email protected]>
Signed-off-by: Simeon Nakov <[email protected]>
Signed-off-by: Simeon Nakov <[email protected]>
Signed-off-by: Simeon Nakov <[email protected]>
Signed-off-by: Simeon Nakov <[email protected]>
Signed-off-by: Simeon Nakov <[email protected]>
Signed-off-by: Simeon Nakov <[email protected]>
2b4d890 to
71c4d1b
Compare
quiet-node
left a comment
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.
Left some comments and questions
packages/relay/src/lib/services/transactionPoolService/LocalPendingTransactionStorage.ts
Outdated
Show resolved
Hide resolved
packages/relay/src/lib/services/transactionPoolService/RedisPendingTransactionStorage.ts
Outdated
Show resolved
Hide resolved
packages/relay/src/lib/services/transactionPoolService/RedisPendingTransactionStorage.ts
Show resolved
Hide resolved
| /** | ||
| * Key for the global pending transactions index. | ||
| */ | ||
| private readonly globalIndexKey = 'txpool:pending:txns'; |
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.
Should we make this a dependant of keyPrefix?
| private readonly globalIndexKey = 'txpool:pending:txns'; | |
| private readonly globalIndexKey = `${keyPrefix}txns`; |
Also should we make it more clear? globalIndexKey is quite confusing. Could it be globalPendingTxsKey or something? and the value could be ${keyPrefix}global?
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.
done
Signed-off-by: Simeon Nakov <[email protected]>
| const key = this.keyFor(address); | ||
|
|
||
| await this.redisClient.sRem(key, txHash); | ||
| await this.redisClient.multi().sRem(key, rlpHex).sRem(this.globalIndexKey, rlpHex).execAsPipeline(); |
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.
Why do we have .execAsPipeline() here but can't be .exec() like in addToList()?
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.
fixed
Signed-off-by: Simeon Nakov <[email protected]>
Signed-off-by: Simeon Nakov <[email protected]>
packages/relay/src/lib/services/ethService/transactionService/TransactionService.ts
Show resolved
Hide resolved
quiet-node
left a comment
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.
Two last things and I believe I'm ready to approve
packages/relay/src/lib/services/transactionPoolService/RedisPendingTransactionStorage.ts
Show resolved
Hide resolved
packages/relay/src/lib/services/transactionPoolService/transactionPoolService.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Simeon Nakov <[email protected]>
quiet-node
left a comment
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.
Coolio LGTM!
| throw new Error('Transaction hash is required for storage'); | ||
| } | ||
| const rlpHex = tx.serialized; | ||
| await this.storage.addToList(addressLowerCased, rlpHex); |
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.
Do you think we have sufficient error handling? On the lower level we have error logging when an error is detected on redis level, but we do not throw any errors. Just wondering if we might need error handling or at least logging on error, currently we use logger.debug im just not sure it will log correctly
Signed-off-by: Simeon Nakov <[email protected]>
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #4521 +/- ##
==========================================
- Coverage 95.52% 95.40% -0.12%
==========================================
Files 127 127
Lines 20493 20624 +131
Branches 1760 1763 +3
==========================================
+ Hits 19576 19677 +101
- Misses 898 928 +30
Partials 19 19
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
Description
Currently, we're saving only the transaction hash in the transaction pool storage but for txpool_* related methods, we need the entire transaction object. This PR adds storage of the entire txn as an RLP hex in both LRU and Redis.
Related issue(s)
Fixes #4500