LW-11609 Web socket ChainHistoryProvider #1489
Open
+1,832
−200
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
iccicci
force-pushed
the
feat/web-socket-chain-history-provider
branch
4 times, most recently
from
517a6a7
to
8b0e7ac
Compare
iccicci
requested review from
mirceahasegan,
mkazlauskas,
ginnun,
mchappell,
rhyslbw,
AngelCastilloB,
bernokl and
gytis-ivaskevicius
mkazlauskas
requested changes
Sep 25, 2024
packages/core/src/WebSocket.ts
Outdated
| } | ||
|
|
||
| interface EmitHealthOptions { | ||
| notRecoverable?: boolean; |
There was a problem hiding this comment.
Nitpick/suggestion: in general non-negated boolean variables result in more readable boolean logic
Suggested change
| notRecoverable?: boolean; | |
| recoverable?: boolean; |
There was a problem hiding this comment.
Yes... in this case I used the negative form as it also has the meaning "this error needs to end up to a server restart".
There was a problem hiding this comment.
Sure there are exceptions to the rule 🙏
On a different topic:
has the meaning "this error needs to end up to a server restart".
In that case, server should probably die, and all ws connections get disconnected? 🤔
There was a problem hiding this comment.
Yes... in case of not recoverable error there is no alternative...
The server tries to, and often it does... due to the nature of a not recoverable error, sometimes it could be something is not correctly closed and self shutdown is not successful; anyway the health check is set to inform the orchestrator it has to perform a forced shutdown.
There was a problem hiding this comment.
@gytis-ivaskevicius what is the preferred behavior for unrecoverable errors: die or report unhealthy?
There was a problem hiding this comment.
We discussed this in deep: we are working on health checks since some weeks ago.
The ws-server implements the health check logic as specified by SRE (two distinct endpoints one for readiness one for the health status).
We didn't specifically discuss the self shutdown option, anyway if the required action is a restart, making the server able to do it by itself (rather than waiting for the orchestrator to do that) sounds as an improvement...
For sure, any insights from @gytis-ivaskevicius will be appreciated.
There was a problem hiding this comment.
if a restart may resolve the error - either liveliness probe should fail or it should exit with non 0 exit code. Service shutting down in this case is a better option since it gives a faster feedback loop
if the error can not be resolved by restarting - it would be better if liveliness probe would fail to reduce restarts and possibly keep other parts of application working
| const isEventError = (error: unknown): error is { error: Error } => | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| typeof error === 'object' && !!error && (error as any).error instanceof Error; | ||
|
|
||
| export const isTxRelevant = ( |
There was a problem hiding this comment.
Also need to check certificates
There was a problem hiding this comment.
Actually no. This is not a general purpose function, it is dedicated to ChainHistoryProvider.transactionsByAddresses.
This is meant to check transaction only by addresses as DbSyncChainHistoryProvider.transactionsByAddresses does.
There was a problem hiding this comment.
If DbSyncChainHistoryProvider.transactionsByAddresses doesn't check the certificates then it's an edge case bug. You can make a transaction that has a certificate (e.g. delegation) without using any inputs/outputs of an associated address
There was a problem hiding this comment.
It could be, anyway fixing pre-existing bugs is OT for this PR.
The intent here is to replicate what the (DbSync|http)Providers do.
There was a problem hiding this comment.
Please log this bug for separate consideration @iccicci
iccicci
force-pushed
the
feat/web-socket-chain-history-provider
branch
from
8b0e7ac
to
84ef892
Compare
iccicci
changed the title
rhyslbw
approved these changes
Oct 1, 2024
gytis-ivaskevicius
requested changes
Oct 1, 2024
Comment on lines
+95
to
+101
| const result: Cardano.TxOut = { address: txOut.address, value: txOut.value }; | ||
|
|
||
| if (txOut.datum) result.datum = txOut.datum; | ||
| if (txOut.datumHash) result.datumHash = txOut.datumHash; | ||
| if (txOut.scriptReference) result.scriptReference = txOut.scriptReference; | ||
|
|
||
| return result; |
There was a problem hiding this comment.
Why not this?
Suggested change
| const result: Cardano.TxOut = { address: txOut.address, value: txOut.value }; | |
| if (txOut.datum) result.datum = txOut.datum; | |
| if (txOut.datumHash) result.datumHash = txOut.datumHash; | |
| if (txOut.scriptReference) result.scriptReference = txOut.scriptReference; | |
| return result; | |
| return { address: txOut.address, value: txOut.value, datum: txOut.datum, datumHash: txOut.datumHash, scriptReference: txout.scriptReference }; |
There was a problem hiding this comment.
I'd prefer the suggested form as well, but I had to chose for the used approach to make the output exactly equal to the DbSyncChainSyncProvider ones.
In case of an output without any additional data, the DbSyncChainSyncProvider output is:
{
"address": "abc",
"value", 10n
}
while the output of the preferred form would be:
{
"address": "abc",
"value", 10n,
"datum": undefined,
"datumHash": undefined,
"scriptReference": undefined
}
I know there is no difference between the two outputs, but the comparison tests would need a lot of additional work to inspect each value rather just comparing the serialized form of the two outputs.
There was a problem hiding this comment.
Ill leave this to the team to decide, not my code, not my call
There was a problem hiding this comment.
The target here is to produce an output exactly equal to the DbSyncChainSyncProvider one to be sure the change has no impacts.
Refactoring the original Provider is out of the scope of this PR.
Comment on lines
+28
to
+31
| bytes, | ||
| tx.hash AS tx_id | ||
| FROM ${collateral ? 'collateral_tx_out' : 'tx_out'} AS tx_out | ||
| LEFT JOIN datum ON datum.id = inline_datum_id |
There was a problem hiding this comment.
Suggested change
| bytes, | |
| tx.hash AS tx_id | |
| FROM ${collateral ? 'collateral_tx_out' : 'tx_out'} AS tx_out | |
| LEFT JOIN datum ON datum.id = inline_datum_id | |
| datum.bytes, | |
| tx.hash AS tx_id | |
| FROM ${collateral ? 'collateral_tx_out' : 'tx_out'} AS tx_out | |
| LEFT JOIN datum ON datum.id = inline_datum_id |
There was a problem hiding this comment.
bytes is not an ambiguous column (otherwise the query would fail): there is no need to specify the source table.
Specifying the source table when it's not required would only (slightly) increases the DB workload to parse the query.
There was a problem hiding this comment.
It's just a readability improvement. If it was up to me, i'd say queries with multiple tables should always have explicitly defined select clause.
Load change will not be noticeable at all
| @@ -97,6 +98,8 @@ export interface ScriptModel { | |||
| bytes: Buffer; | |||
| hash: Buffer; | |||
| serialised_size: number; | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| json: any; | |||
There was a problem hiding this comment.
Not sure how its used but unknown type might be a better option
There was a problem hiding this comment.
any can be used as an Object while unknown can't be.
We know db-sync provides the data as a JSON Object.
There was a problem hiding this comment.
yes, that is the point for type safety. I am not sure where json is used, but ideally it would be unknown to force developers to type check before using it
There was a problem hiding this comment.
Being the source of this data db-sync, we know it returns an Object.
Adding a check which we know it will never fail sounds to me as a resources waste both in implementing and in running it.
|
|
||
| const loadTransactions = async () => { | ||
| const addressesMap = new Map<Cardano.PaymentAddress, true>(); | ||
| for (const ws of this.wss.clients) for (const address of ws.addresses) addressesMap.set(address, true); |
There was a problem hiding this comment.
Suggested change
| for (const ws of this.wss.clients) for (const address of ws.addresses) addressesMap.set(address, true); | |
| const addressesMap = this.wss.clients | |
| .flatMap(it => it.addresses) | |
| .reduce((result, it) => ( {...result, `${it}`: true})) |
There was a problem hiding this comment.
The proposed version, maybe is a bit more readable, but would have a considerably higher CPU cost.
There was a problem hiding this comment.
I was wondering how well v8 optimizes these functions, I ran a few benchmarks:
And here is an even faster option:
There was a problem hiding this comment.
Interesting!
Would you mind to have a pairing session to perform some more tests?
Comment on lines
+372
to
+393
| const compound = < | ||
| C1 extends Cardano.HydratedCertificate, | ||
| C2 extends Cardano.HydratedCertificate, | ||
| C3 extends Cardano.HydratedCertificate | ||
| >( | ||
| certs1: (readonly [number, C1])[], | ||
| certs2: (readonly [number, C2])[], | ||
| merge: (c1: C1, c2: C2) => C3 | ||
| ) => { | ||
| const result1: (readonly [number, C1])[] = []; | ||
| const result: (readonly [number, C3])[] = []; | ||
| const foundIndexes2: number[] = []; | ||
|
|
||
| for (const c1 of certs1) { | ||
| const c2index = certs2.findIndex((c2) => c1[0] === c2[0]); | ||
|
|
||
| if (c2index === -1) result1.push(c1); | ||
| else { | ||
| foundIndexes2.push(c2index); | ||
| result.push([c1[0], merge(c1[1], certs2[c2index][1])]); | ||
| } | ||
| } |
There was a problem hiding this comment.
I feel like a couple of comments would be nice here, at least I have no idea what it does
| }; | ||
|
|
||
| // eslint-disable-next-line complexity, max-statements, sonarjs/cognitive-complexity | ||
| const mapCertificates = (tx: TxModel) => { |
There was a problem hiding this comment.
The functions in this file is the code which will run for 99% of time.
I preferred to chose the approach which will require the lower CPU load as possible.
Comment on lines
+549
to
+554
| const result: Cardano.TxOut = { | ||
| address: output.address, | ||
| value: { coins: BigInt(output.value) } | ||
| }; | ||
|
|
||
| if (assets) result.value.assets = assets; |
There was a problem hiding this comment.
Suggested change
| const result: Cardano.TxOut = { | |
| address: output.address, | |
| value: { coins: BigInt(output.value) } | |
| }; | |
| if (assets) result.value.assets = assets; | |
| const result: Cardano.TxOut = { | |
| address: output.address, | |
| value: { coins: BigInt(output.value) }, | |
| assets: assets ? assets : undefined, | |
| // any issues with this? is it null sometimes or something? | |
| assets: assets, | |
| }; |
building object this way would be more readable
| values: [ids] | ||
| }); | ||
|
|
||
| const rows = onlyUtxos |
There was a problem hiding this comment.
for utxo this probalby should be a separate funciton
Comment on lines
+9
to
+13
| const { HD_ACTIVE_ADDR_COUNT, HD_MAX_TX_HISTORY, TARGET_NET, WALLETS } = Object.assign( | ||
| { HD_ACTIVE_ADDR_COUNT: '10', HD_MAX_TX_HISTORY: '100', TARGET_NET: 'mainnet', WALLETS: '100' }, | ||
| // eslint-disable-next-line no-undef | ||
| __ENV | ||
| ); |
There was a problem hiding this comment.
Suggested change
| const { HD_ACTIVE_ADDR_COUNT, HD_MAX_TX_HISTORY, TARGET_NET, WALLETS } = Object.assign( | |
| { HD_ACTIVE_ADDR_COUNT: '10', HD_MAX_TX_HISTORY: '100', TARGET_NET: 'mainnet', WALLETS: '100' }, | |
| // eslint-disable-next-line no-undef | |
| __ENV | |
| ); | |
| const { HD_ACTIVE_ADDR_COUNT, HD_MAX_TX_HISTORY, TARGET_NET, WALLETS } = | |
| { HD_ACTIVE_ADDR_COUNT: '10', HD_MAX_TX_HISTORY: '100', TARGET_NET: 'mainnet', WALLETS: '100', ...__ENV}; |
There was a problem hiding this comment.
K6 JavaScript environment doesn't support the proposed syntax. 😞
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Lace performs a huge amount of HTTP calls.
Proposed Solution
Implemented event based
ChainHistoryProvider.transactionsByAddressesandUtxoProvider.The load tests demonstrate this back end implementation is able to sustain even an unexpected workload: 100 users simultaneously performing wallet restoration.
Follow some load test results.
1 user performing wallet restoration:
2 users simultaneously performing wallet restoration:
3 users simultaneously performing wallet restoration:
4 users simultaneously performing wallet restoration:
5 users simultaneously performing wallet restoration:
10 users simultaneously performing wallet restoration:
20 users simultaneously performing wallet restoration:
50 users simultaneously performing wallet restoration:
Here is not relevant the time, but moreover that even under an insane workload the server is able to complete its job without timeout errors for the customers.
100 users simultaneously performing wallet restoration:
100 users with lots of transactions simultaneously performing wallet restoration:
Important Changes Introduced
The
DbSyncChainHistoryProviderand theDbSyncUtxoProviderperform almost the same actions about utxos but with some slight difference.They were aligned to produce exactly the same data with two purposes:
CardanoWsClientuses the same source for both the pseudo-providers implementations; this simplified a lot the comparison tests.