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

Fix array query string parsing #5268

Merged
merged 1 commit into from
Mar 15, 2023
Merged

Conversation

nflaig
Copy link
Member

@nflaig nflaig commented Mar 14, 2023

Motivation

The query string parser we use at the moment does not support an array as comma-separated values and on top of that the package itself is deprecated and last release was 2 years ago, querystring node API is considered legacy as well and does not support all parsing requirements we need.

The following request it not correctly parsed at the moment, it result in just one value

curl 'http://localhost:9596/eth/v1/beacon/states/head/validators?id=0xb89bebc699769726a318c8e9971bd3171297c61aea4a6578a7a4f94b547dcba5bac16a89108b6b6a1fe3695d1a874a0b,0xa99a76ed7796f7be22d5b7e85deeb7c5677e88e511e0b337618f8c4eb61349b4bf2d153f649f7b53359fe8b94a38e44c'

To be OpenAPI spec compliant we need to support this query string format, all other 4 CLs support this format as well.

The issue was reported by jcrtp from rocketpool, see discord thread.

Description

Updates fastify querystringParser to use qs instead of querystring.

qs seems to be really popular and is actively maintained. The code looks clean as well and has a good test suite.

We also already use qs package in several other places (e.g. here) in the monorepo.

// defaults to 20 but Beacon API spec allows max items of 30
arrayLimit: 30,
// array as comma-separated values must be supported to be OpenAPI spec compliant
comma: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

koa openapi req/res validator uses this library as well and sets comma: true, this is required to be OpenAPI spec compliant, see Parameter Serialization

@@ -43,7 +43,15 @@ export class RestApiServer {
const server = fastify({
logger: false,
ajv: {customOptions: {coerceTypes: "array"}},
querystringParser: querystring.parse,
querystringParser: (str) =>
qs.parse(str, {
Copy link
Member Author

Choose a reason for hiding this comment

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

packages/light-client/package.json Show resolved Hide resolved
querystringParser: (str) =>
qs.parse(str, {
// defaults to 20 but Beacon API spec allows max items of 30
arrayLimit: 30,
Copy link
Member Author

Choose a reason for hiding this comment

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

Beacon API spec defines 30 as maxItems for some values, e.g. getStateValidators - id query, see spec value

yarn.lock Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Mar 14, 2023

Performance Report

✔️ no performance regression detected

Full benchmark results
Benchmark suite Current: 19e6936 Previous: acf3d98 Ratio
getPubkeys - index2pubkey - req 1000 vs - 250000 vc 542.18 us/op 497.04 us/op 1.09
getPubkeys - validatorsArr - req 1000 vs - 250000 vc 50.014 us/op 45.667 us/op 1.10
BLS verify - blst-native 1.2536 ms/op 1.2831 ms/op 0.98
BLS verifyMultipleSignatures 3 - blst-native 2.5379 ms/op 2.6305 ms/op 0.96
BLS verifyMultipleSignatures 8 - blst-native 5.4793 ms/op 5.6995 ms/op 0.96
BLS verifyMultipleSignatures 32 - blst-native 19.901 ms/op 21.017 ms/op 0.95
BLS aggregatePubkeys 32 - blst-native 26.662 us/op 28.442 us/op 0.94
BLS aggregatePubkeys 128 - blst-native 104.66 us/op 108.79 us/op 0.96
getAttestationsForBlock 58.781 ms/op 77.380 ms/op 0.76
isKnown best case - 1 super set check 276.00 ns/op 280.00 ns/op 0.99
isKnown normal case - 2 super set checks 274.00 ns/op 277.00 ns/op 0.99
isKnown worse case - 16 super set checks 274.00 ns/op 265.00 ns/op 1.03
CheckpointStateCache - add get delete 5.6420 us/op 5.3130 us/op 1.06
validate gossip signedAggregateAndProof - struct 2.8826 ms/op 2.8801 ms/op 1.00
validate gossip attestation - struct 1.3553 ms/op 1.3690 ms/op 0.99
pickEth1Vote - no votes 1.3179 ms/op 1.4385 ms/op 0.92
pickEth1Vote - max votes 10.062 ms/op 15.161 ms/op 0.66
pickEth1Vote - Eth1Data hashTreeRoot value x2048 8.9947 ms/op 10.240 ms/op 0.88
pickEth1Vote - Eth1Data hashTreeRoot tree x2048 14.748 ms/op 15.351 ms/op 0.96
pickEth1Vote - Eth1Data fastSerialize value x2048 687.77 us/op 743.51 us/op 0.93
pickEth1Vote - Eth1Data fastSerialize tree x2048 6.5544 ms/op 7.4146 ms/op 0.88
bytes32 toHexString 544.00 ns/op 538.00 ns/op 1.01
bytes32 Buffer.toString(hex) 379.00 ns/op 411.00 ns/op 0.92
bytes32 Buffer.toString(hex) from Uint8Array 581.00 ns/op 594.00 ns/op 0.98
bytes32 Buffer.toString(hex) + 0x 356.00 ns/op 360.00 ns/op 0.99
Object access 1 prop 0.16700 ns/op 0.17800 ns/op 0.94
Map access 1 prop 0.16100 ns/op 0.15300 ns/op 1.05
Object get x1000 6.8060 ns/op 6.7350 ns/op 1.01
Map get x1000 0.61100 ns/op 0.70000 ns/op 0.87
Object set x1000 52.784 ns/op 68.299 ns/op 0.77
Map set x1000 43.617 ns/op 49.156 ns/op 0.89
Return object 10000 times 0.23340 ns/op 0.24710 ns/op 0.94
Throw Error 10000 times 4.2549 us/op 4.0967 us/op 1.04
fastMsgIdFn sha256 / 200 bytes 3.4390 us/op 3.4310 us/op 1.00
fastMsgIdFn h32 xxhash / 200 bytes 284.00 ns/op 287.00 ns/op 0.99
fastMsgIdFn h64 xxhash / 200 bytes 400.00 ns/op 408.00 ns/op 0.98
fastMsgIdFn sha256 / 1000 bytes 11.583 us/op 11.420 us/op 1.01
fastMsgIdFn h32 xxhash / 1000 bytes 403.00 ns/op 410.00 ns/op 0.98
fastMsgIdFn h64 xxhash / 1000 bytes 477.00 ns/op 479.00 ns/op 1.00
fastMsgIdFn sha256 / 10000 bytes 104.69 us/op 103.37 us/op 1.01
fastMsgIdFn h32 xxhash / 10000 bytes 1.9580 us/op 1.9720 us/op 0.99
fastMsgIdFn h64 xxhash / 10000 bytes 1.4150 us/op 1.4110 us/op 1.00
enrSubnets - fastDeserialize 64 bits 1.2830 us/op 1.3080 us/op 0.98
enrSubnets - ssz BitVector 64 bits 491.00 ns/op 496.00 ns/op 0.99
enrSubnets - fastDeserialize 4 bits 172.00 ns/op 168.00 ns/op 1.02
enrSubnets - ssz BitVector 4 bits 491.00 ns/op 506.00 ns/op 0.97
prioritizePeers score -10:0 att 32-0.1 sync 2-0 105.54 us/op 129.98 us/op 0.81
prioritizePeers score 0:0 att 32-0.25 sync 2-0.25 133.26 us/op 147.85 us/op 0.90
prioritizePeers score 0:0 att 32-0.5 sync 2-0.5 173.49 us/op 228.85 us/op 0.76
prioritizePeers score 0:0 att 64-0.75 sync 4-0.75 319.90 us/op 417.65 us/op 0.77
prioritizePeers score 0:0 att 64-1 sync 4-1 362.43 us/op 450.63 us/op 0.80
array of 16000 items push then shift 1.6259 us/op 1.7516 us/op 0.93
LinkedList of 16000 items push then shift 8.7610 ns/op 9.9070 ns/op 0.88
array of 16000 items push then pop 78.027 ns/op 108.70 ns/op 0.72
LinkedList of 16000 items push then pop 8.4370 ns/op 9.1200 ns/op 0.93
array of 24000 items push then shift 2.3448 us/op 2.4443 us/op 0.96
LinkedList of 24000 items push then shift 8.7930 ns/op 9.3060 ns/op 0.94
array of 24000 items push then pop 79.308 ns/op 83.688 ns/op 0.95
LinkedList of 24000 items push then pop 8.5270 ns/op 8.7870 ns/op 0.97
intersect bitArray bitLen 8 13.209 ns/op 13.547 ns/op 0.98
intersect array and set length 8 77.667 ns/op 81.632 ns/op 0.95
intersect bitArray bitLen 128 43.124 ns/op 44.377 ns/op 0.97
intersect array and set length 128 1.0262 us/op 1.1377 us/op 0.90
Buffer.concat 32 items 2.6220 us/op 3.2670 us/op 0.80
Uint8Array.set 32 items 2.7310 us/op 3.4610 us/op 0.79
pass gossip attestations to forkchoice per slot 2.2699 ms/op 2.4062 ms/op 0.94
computeDeltas 2.8015 ms/op 3.3060 ms/op 0.85
computeProposerBoostScoreFromBalances 1.7748 ms/op 2.1397 ms/op 0.83
altair processAttestation - 250000 vs - 7PWei normalcase 2.2480 ms/op 2.3667 ms/op 0.95
altair processAttestation - 250000 vs - 7PWei worstcase 3.5157 ms/op 3.8640 ms/op 0.91
altair processAttestation - setStatus - 1/6 committees join 138.98 us/op 142.78 us/op 0.97
altair processAttestation - setStatus - 1/3 committees join 270.46 us/op 273.01 us/op 0.99
altair processAttestation - setStatus - 1/2 committees join 366.99 us/op 367.60 us/op 1.00
altair processAttestation - setStatus - 2/3 committees join 460.33 us/op 456.34 us/op 1.01
altair processAttestation - setStatus - 4/5 committees join 647.13 us/op 659.15 us/op 0.98
altair processAttestation - setStatus - 100% committees join 762.59 us/op 747.14 us/op 1.02
altair processBlock - 250000 vs - 7PWei normalcase 18.451 ms/op 18.231 ms/op 1.01
altair processBlock - 250000 vs - 7PWei normalcase hashState 27.583 ms/op 27.550 ms/op 1.00
altair processBlock - 250000 vs - 7PWei worstcase 51.937 ms/op 51.527 ms/op 1.01
altair processBlock - 250000 vs - 7PWei worstcase hashState 71.389 ms/op 66.229 ms/op 1.08
phase0 processBlock - 250000 vs - 7PWei normalcase 2.0883 ms/op 2.0335 ms/op 1.03
phase0 processBlock - 250000 vs - 7PWei worstcase 29.442 ms/op 28.206 ms/op 1.04
altair processEth1Data - 250000 vs - 7PWei normalcase 462.05 us/op 475.00 us/op 0.97
vc - 250000 eb 1 eth1 1 we 0 wn 0 - smpl 15 7.7690 us/op 6.6250 us/op 1.17
vc - 250000 eb 0.95 eth1 0.1 we 0.05 wn 0 - smpl 219 21.480 us/op 19.348 us/op 1.11
vc - 250000 eb 0.95 eth1 0.3 we 0.05 wn 0 - smpl 42 8.8170 us/op 8.4030 us/op 1.05
vc - 250000 eb 0.95 eth1 0.7 we 0.05 wn 0 - smpl 18 7.4750 us/op 6.4920 us/op 1.15
vc - 250000 eb 0.1 eth1 0.1 we 0 wn 0 - smpl 1020 87.153 us/op 111.40 us/op 0.78
vc - 250000 eb 0.03 eth1 0.03 we 0 wn 0 - smpl 11777 648.86 us/op 673.33 us/op 0.96
vc - 250000 eb 0.01 eth1 0.01 we 0 wn 0 - smpl 16384 908.88 us/op 917.16 us/op 0.99
vc - 250000 eb 0 eth1 0 we 0 wn 0 - smpl 16384 871.91 us/op 874.43 us/op 1.00
vc - 250000 eb 0 eth1 0 we 0 wn 0 nocache - smpl 16384 2.2910 ms/op 2.3339 ms/op 0.98
vc - 250000 eb 0 eth1 1 we 0 wn 0 - smpl 16384 1.5709 ms/op 1.4972 ms/op 1.05
vc - 250000 eb 0 eth1 1 we 0 wn 0 nocache - smpl 16384 3.9315 ms/op 3.8562 ms/op 1.02
Tree 40 250000 create 297.80 ms/op 309.71 ms/op 0.96
Tree 40 250000 get(125000) 180.65 ns/op 178.69 ns/op 1.01
Tree 40 250000 set(125000) 937.79 ns/op 893.82 ns/op 1.05
Tree 40 250000 toArray() 17.834 ms/op 17.195 ms/op 1.04
Tree 40 250000 iterate all - toArray() + loop 17.266 ms/op 17.222 ms/op 1.00
Tree 40 250000 iterate all - get(i) 67.724 ms/op 66.910 ms/op 1.01
MutableVector 250000 create 10.792 ms/op 10.720 ms/op 1.01
MutableVector 250000 get(125000) 6.5840 ns/op 6.5820 ns/op 1.00
MutableVector 250000 set(125000) 258.71 ns/op 253.52 ns/op 1.02
MutableVector 250000 toArray() 3.3860 ms/op 2.6792 ms/op 1.26
MutableVector 250000 iterate all - toArray() + loop 3.1138 ms/op 2.7751 ms/op 1.12
MutableVector 250000 iterate all - get(i) 1.5112 ms/op 1.5167 ms/op 1.00
Array 250000 create 2.7132 ms/op 2.5522 ms/op 1.06
Array 250000 clone - spread 1.1768 ms/op 1.1693 ms/op 1.01
Array 250000 get(125000) 0.57000 ns/op 0.55400 ns/op 1.03
Array 250000 set(125000) 0.64900 ns/op 0.65100 ns/op 1.00
Array 250000 iterate all - loop 82.938 us/op 85.608 us/op 0.97
effectiveBalanceIncrements clone Uint8Array 300000 27.738 us/op 27.843 us/op 1.00
effectiveBalanceIncrements clone MutableVector 300000 364.00 ns/op 365.00 ns/op 1.00
effectiveBalanceIncrements rw all Uint8Array 300000 165.96 us/op 169.01 us/op 0.98
effectiveBalanceIncrements rw all MutableVector 300000 82.411 ms/op 86.970 ms/op 0.95
phase0 afterProcessEpoch - 250000 vs - 7PWei 114.51 ms/op 120.54 ms/op 0.95
phase0 beforeProcessEpoch - 250000 vs - 7PWei 35.461 ms/op 46.673 ms/op 0.76
altair processEpoch - mainnet_e81889 325.01 ms/op 321.59 ms/op 1.01
mainnet_e81889 - altair beforeProcessEpoch 49.030 ms/op 69.413 ms/op 0.71
mainnet_e81889 - altair processJustificationAndFinalization 16.135 us/op 22.693 us/op 0.71
mainnet_e81889 - altair processInactivityUpdates 5.7154 ms/op 5.3681 ms/op 1.06
mainnet_e81889 - altair processRewardsAndPenalties 70.195 ms/op 58.859 ms/op 1.19
mainnet_e81889 - altair processRegistryUpdates 5.1800 us/op 4.1290 us/op 1.25
mainnet_e81889 - altair processSlashings 600.00 ns/op 922.00 ns/op 0.65
mainnet_e81889 - altair processEth1DataReset 898.00 ns/op 702.00 ns/op 1.28
mainnet_e81889 - altair processEffectiveBalanceUpdates 1.2934 ms/op 1.3777 ms/op 0.94
mainnet_e81889 - altair processSlashingsReset 7.0230 us/op 5.1270 us/op 1.37
mainnet_e81889 - altair processRandaoMixesReset 7.1450 us/op 6.8230 us/op 1.05
mainnet_e81889 - altair processHistoricalRootsUpdate 815.00 ns/op 1.9640 us/op 0.41
mainnet_e81889 - altair processParticipationFlagUpdates 2.4930 us/op 3.7570 us/op 0.66
mainnet_e81889 - altair processSyncCommitteeUpdates 593.00 ns/op 808.00 ns/op 0.73
mainnet_e81889 - altair afterProcessEpoch 128.16 ms/op 133.40 ms/op 0.96
phase0 processEpoch - mainnet_e58758 367.35 ms/op 369.44 ms/op 0.99
mainnet_e58758 - phase0 beforeProcessEpoch 152.13 ms/op 144.44 ms/op 1.05
mainnet_e58758 - phase0 processJustificationAndFinalization 19.371 us/op 23.422 us/op 0.83
mainnet_e58758 - phase0 processRewardsAndPenalties 67.158 ms/op 69.904 ms/op 0.96
mainnet_e58758 - phase0 processRegistryUpdates 8.3190 us/op 10.921 us/op 0.76
mainnet_e58758 - phase0 processSlashings 645.00 ns/op 821.00 ns/op 0.79
mainnet_e58758 - phase0 processEth1DataReset 765.00 ns/op 693.00 ns/op 1.10
mainnet_e58758 - phase0 processEffectiveBalanceUpdates 1.5940 ms/op 1.1478 ms/op 1.39
mainnet_e58758 - phase0 processSlashingsReset 4.2810 us/op 5.5990 us/op 0.76
mainnet_e58758 - phase0 processRandaoMixesReset 10.546 us/op 6.9300 us/op 1.52
mainnet_e58758 - phase0 processHistoricalRootsUpdate 593.00 ns/op 918.00 ns/op 0.65
mainnet_e58758 - phase0 processParticipationRecordUpdates 4.0660 us/op 5.9660 us/op 0.68
mainnet_e58758 - phase0 afterProcessEpoch 100.93 ms/op 105.33 ms/op 0.96
phase0 processEffectiveBalanceUpdates - 250000 normalcase 1.2719 ms/op 1.2772 ms/op 1.00
phase0 processEffectiveBalanceUpdates - 250000 worstcase 0.5 1.5637 ms/op 1.5622 ms/op 1.00
altair processInactivityUpdates - 250000 normalcase 24.585 ms/op 24.038 ms/op 1.02
altair processInactivityUpdates - 250000 worstcase 27.478 ms/op 26.688 ms/op 1.03
phase0 processRegistryUpdates - 250000 normalcase 7.4900 us/op 10.617 us/op 0.71
phase0 processRegistryUpdates - 250000 badcase_full_deposits 275.23 us/op 307.38 us/op 0.90
phase0 processRegistryUpdates - 250000 worstcase 0.5 127.28 ms/op 136.51 ms/op 0.93
altair processRewardsAndPenalties - 250000 normalcase 68.219 ms/op 69.040 ms/op 0.99
altair processRewardsAndPenalties - 250000 worstcase 69.020 ms/op 70.177 ms/op 0.98
phase0 getAttestationDeltas - 250000 normalcase 7.0104 ms/op 7.0702 ms/op 0.99
phase0 getAttestationDeltas - 250000 worstcase 6.9564 ms/op 7.1199 ms/op 0.98
phase0 processSlashings - 250000 worstcase 3.4878 ms/op 3.4899 ms/op 1.00
altair processSyncCommitteeUpdates - 250000 182.96 ms/op 184.76 ms/op 0.99
BeaconState.hashTreeRoot - No change 275.00 ns/op 271.00 ns/op 1.01
BeaconState.hashTreeRoot - 1 full validator 54.526 us/op 54.594 us/op 1.00
BeaconState.hashTreeRoot - 32 full validator 535.35 us/op 539.40 us/op 0.99
BeaconState.hashTreeRoot - 512 full validator 5.4931 ms/op 5.1974 ms/op 1.06
BeaconState.hashTreeRoot - 1 validator.effectiveBalance 62.476 us/op 64.769 us/op 0.96
BeaconState.hashTreeRoot - 32 validator.effectiveBalance 927.31 us/op 959.56 us/op 0.97
BeaconState.hashTreeRoot - 512 validator.effectiveBalance 12.113 ms/op 12.528 ms/op 0.97
BeaconState.hashTreeRoot - 1 balances 48.745 us/op 51.031 us/op 0.96
BeaconState.hashTreeRoot - 32 balances 463.94 us/op 484.94 us/op 0.96
BeaconState.hashTreeRoot - 512 balances 4.6042 ms/op 4.7490 ms/op 0.97
BeaconState.hashTreeRoot - 250000 balances 70.855 ms/op 76.224 ms/op 0.93
aggregationBits - 2048 els - zipIndexesInBitList 18.471 us/op 18.580 us/op 0.99
regular array get 100000 times 33.904 us/op 44.277 us/op 0.77
wrappedArray get 100000 times 33.838 us/op 33.599 us/op 1.01
arrayWithProxy get 100000 times 16.720 ms/op 17.116 ms/op 0.98
ssz.Root.equals 591.00 ns/op 573.00 ns/op 1.03
byteArrayEquals 577.00 ns/op 572.00 ns/op 1.01
shuffle list - 16384 els 7.0668 ms/op 7.0253 ms/op 1.01
shuffle list - 250000 els 102.94 ms/op 103.52 ms/op 0.99
processSlot - 1 slots 9.2140 us/op 9.5310 us/op 0.97
processSlot - 32 slots 1.4036 ms/op 1.3530 ms/op 1.04
getEffectiveBalanceIncrementsZeroInactive - 250000 vs - 7PWei 215.65 us/op 206.91 us/op 1.04
getCommitteeAssignments - req 1 vs - 250000 vc 3.0217 ms/op 2.9666 ms/op 1.02
getCommitteeAssignments - req 100 vs - 250000 vc 4.2459 ms/op 4.2151 ms/op 1.01
getCommitteeAssignments - req 1000 vs - 250000 vc 4.6160 ms/op 4.5722 ms/op 1.01
RootCache.getBlockRootAtSlot - 250000 vs - 7PWei 4.8900 ns/op 4.8300 ns/op 1.01
state getBlockRootAtSlot - 250000 vs - 7PWei 714.70 ns/op 638.94 ns/op 1.12
computeProposers - vc 250000 10.885 ms/op 10.802 ms/op 1.01
computeEpochShuffling - vc 250000 105.32 ms/op 104.09 ms/op 1.01
getNextSyncCommittee - vc 250000 177.52 ms/op 176.67 ms/op 1.00

by benchmarkbot/action

@nflaig nflaig force-pushed the nflaig/fix-query-spec-compliance branch from a198b7e to a9641a9 Compare March 14, 2023 19:47
@nflaig nflaig marked this pull request as ready for review March 14, 2023 19:57
@nflaig nflaig requested a review from a team as a code owner March 14, 2023 19:57
Copy link
Contributor

@nazarhussain nazarhussain left a comment

Choose a reason for hiding this comment

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

The changes seems ok as it does not show up any regression. I tried to identify what are the changes in the minor release update but seems their repo does not document it well.

@nflaig
Copy link
Member Author

nflaig commented Mar 14, 2023

@nazarhussain they document their changes in changelog file, see CHANGELOG.md

Copy link
Contributor

@twoeths twoeths left a comment

Choose a reason for hiding this comment

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

looks good to me. The reason we didn't catch this issue in unit tests is because we always use our client api there.
probably we should also create unit tests with curl

@twoeths twoeths merged commit 73c6c6c into unstable Mar 15, 2023
@twoeths twoeths deleted the nflaig/fix-query-spec-compliance branch March 15, 2023 07:35
twoeths pushed a commit that referenced this pull request Mar 15, 2023
@nflaig
Copy link
Member Author

nflaig commented Mar 15, 2023

@tuyennhv yeah, I looked into adding a test for this but there was no quick way to do it. I think for general cases such as this we just need to improve our spec compliance tests

@wemeetagain
Copy link
Member

🎉 This PR is included in v1.6.0 🎉

@wemeetagain
Copy link
Member

🎉 This PR is included in v1.7.0 🎉

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.

None yet

5 participants