Support parsing altair sync committee gossip topics #2589
Conversation
github-actions
bot
added
the
scope-networking
|
Code Climate has analyzed commit 8b7f1d8 and detected 1 issue on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
| return `/eth2/${forkDigestHex}/${topicType}/${topic.encoding ?? DEFAULT_ENCODING}`; | ||
| const forkDigestHexNoPrefix = toHexStringNoPrefix(forkDigest); | ||
| const topicType = stringifyGossipTopicType(topic); | ||
| return [topicPrefix, forkDigestHexNoPrefix, topicType, topic.encoding ?? DEFAULT_ENCODING].join("/"); |
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| export const AttestationSubnetRegExp = new RegExp("^/eth2/[a-f0-9]{8}/beacon_attestation_([0-9]+)/\\w+$"); | ||
| const topicStrNoPrefix = topicStr.slice(topicPrefix.length + 1); // +1 for the second '/' in '/eth2/...' | ||
| const [forkDigestHexNoPrefix, gossipTypeStr, encodingStr] = topicStrNoPrefix.split("/"); |
There was a problem hiding this comment.
| const [forkDigestHexNoPrefix, gossipTypeStr, encodingStr] = topicStrNoPrefix.split("/"); | |
| const [,,forkDigestHexNoPrefix, gossipTypeStr, encodingStr] = topicStrNoPrefix.split("/"); |
I think that now you can remove slice line above. Maybe it will be a little bit easier to read code.
There was a problem hiding this comment.
But it's not the same logic, you are doing extra work since you have to run .split() on extra characters that you already now are not useful.
There was a problem hiding this comment.
do you think that split vs slice would have notable performance impact?
There was a problem hiding this comment.
do you think that split vs slice would have notable performance impact?
Maybe, I've not quantified it. Tho this is a hot path since we want to process a lot of gossip objects per second. To understand your proposal do you suggest
const [,,forkDigestHexNoPrefix, gossipTypeStr, encodingStr] = topicStrNoPrefix.split("/");or
const [,,forkDigestHexNoPrefix, gossipTypeStr, encodingStr] = topicStr.split("/");?
Assuming the latter I don't see the advantage of the [,, syntax towards readability, such that it justifies doing extra unnecessary work. I can add more comments to make the intention and rationale more clear.
There was a problem hiding this comment.
I've benchmark it and
- splice is very fast, 250000 opts / ms
- split is costlier at 2000 opts / ms
Using a regex is slightly faster to extract topic parts (~10%) so I've updated that.
Motivation
Gossip topic parser does not support altair sync committee topics.
Description
I've refactored the parsing functions to not use a Regex. Also now Typescript won't compile if we forget to add tests for new GossipType.
Closes #2588