-
Notifications
You must be signed in to change notification settings - Fork 151
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
fix(BlocksService): refactor api.derive for performance, and add historicApi to BlocksService #699
Conversation
src/services/blocks/BlocksService.ts
Outdated
@@ -496,13 +507,13 @@ export class BlocksService extends AbstractService { | |||
} | |||
|
|||
const multiplier = | |||
await api.query.transactionPayment?.nextFeeMultiplier?.at(parentHash); | |||
await historicApi.query.transactionPayment?.nextFeeMultiplier(); |
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.
From what I understand we may actually need (await api.at(parentHash)).query.transactionPayment?.nextFeeMultiplier()
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.
In order to make sure we are fetching at the paren block
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.
Ahh yes, you are totally correct! good catch
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.
sweet, I actually reverted this bit to its original form, as I cant instantiate another api.at, without throwing a ton of websocket errors in our tests.
Left a few questions but will come back for a full review
Is |
Thanks! So yea |
@@ -148,7 +148,7 @@ | |||
"info": { | |||
"weight": "195000000", | |||
"class": "Normal", | |||
"partialFee": "3333333" | |||
"partialFee": "166666666" |
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 is this partialFee changed?
I guess because the historialApi is used or the calculate fee bug....
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.
Yup so I found a bug for calculating fees in regards to historic blocks. I fixed it in this PR thats why I changed the e2e tests too for partialFee responses.
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
api, | ||
parentHash | ||
); | ||
this.blockWeightStore[specVersion] ||= this.getWeight(historicApi); |
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.
TIL OR assignments are a thing
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, have the same question as niklasad1 though
rel: #698
rel: #693
Performance bump
Benchmarks were done against
/blocks
where the main known regression took place. There are 3 versions we are testing here: v9.1.9, v9.2.0, and this current branch which I will denote as 'beta'.Api.at()
Local Benchmarks were also ran against instantiating
api.at()
. Initially for a particular runtime if the information isn't cached inside of the api it will take about 3 seconds too initialize. But after its cached it usually takes 1-50 milliseconds to recall.All instances of
api.query....at()
inBlocksService
are now using a historicApi to get any information, regardingapi.query
. During this I found a bug where we were getting the wrongtransactionByteFee
for historic runtimes because the api was always using the latest metadata. This is also now fixed, and all e2e tests for kusama reflect this change.api.derive
I noticed
api.derive
was a large area of regression for sidecar while doing local isolated benchmarks, therefore I took the method apart and abstracted the things we need from it in order to speed the request time up.extra notes
I fixed the mockApi for tests in
PalletsStakingProgressService.spec.ts
. Moving forward until all services are switched to using thehistoricApi
the mockApi enviornment will be going through some adjustments. Once the final Service is fixed up, the mockApi will be readjusted to have a generated historicApi, as well as mockApi.Part 2: #702