Skip to content
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

added function name for pool transactions #1425

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

bogdan-rosianu
Copy link
Contributor

Reasoning

  • pool transactions did not have the 'function' field which would make the pool analysis easier

Proposed Changes

  • add function name to pool transactions if it exists

How to test

  • /transaction/pool should contain the transactions functions

@bogdan-rosianu bogdan-rosianu self-assigned this Dec 17, 2024
Copy link

github-actions bot commented Dec 17, 2024

k6 load testing comparison.
Base Commit Hash: c051521
Target Commit Hash: e1c6460

Metric Base Target Diff
AvgMax9095AvgMax9095AvgMax9095
Mex49.33281.1553.3355.2146.18556.8349.7551.22-6.37% ✅+98.05% 🔴-6.71% ✅-7.24% ✅
Tokens49.85397.1653.3455.3045.93368.2549.7351.13-7.85% ✅-7.28% ✅-6.77% ✅-7.54% ✅
Blocks56.771131.6753.8656.3553.071318.5450.2052.89-6.51% ✅+16.51% 🔴-6.79% ✅-6.13% ✅
Transactions62.802315.2453.8256.7859.402588.7850.4352.73-5.42% ✅+11.81% 🔴-6.29% ✅-7.14% ✅
Nodes50.04675.4653.3855.3345.57154.9549.6751.12-8.94% ✅-77.06% ✅-6.97% ✅-7.61% ✅
Pool49.84586.3853.3955.3345.82267.4749.7351.16-8.06% ✅-54.39% ✅-6.85% ✅-7.55% ✅
Accounts50.21640.2853.3155.2147.161411.3449.6851.14-6.07% ✅+120.42% 🔴-6.81% ✅-7.36% ✅
Test Run Duration60009.1960002.56

Legend: Avg - Average Response Time, Max - Maximum Response Time, 90 - 90th Percentile, 95 - 95th Percentile
All times are in milliseconds.

@@ -105,6 +109,11 @@ export class PoolService {
transaction.receiverShard = AddressUtils.computeShard(AddressUtils.bech32Decode(transaction.receiver), shardCount);
}

const metadata = await this.transactionActionService.getTransactionMetadata(this.poolTransactionToTransaction(transaction), false);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our Python implementation, we have a small issue with parsing some badly-formatted transactions. If the issue exists in the JS implementation, as well, a tiny failure might make the whole request fail, I think. Perhaps have a more pessimistic approach?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, maybe we should benchmark this? Or is the logic applied for a mere page of transactions, not all?

If applied on all currently in pool, we might have a small performance issue?

💭

Copy link
Contributor Author

@bogdan-rosianu bogdan-rosianu Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a tiny failure might make the whole request fail, I think

no, if the decoding fails, it will just returned undefined, therefore not applying the function name

regarding the performance impact, it is ok since we cache the entire pool so the processing only happens once

@cfaur09
Copy link
Contributor

cfaur09 commented Dec 17, 2024

Add also the function filter to be able to filter by multiple functions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants