fix: skip validation when encapsulating/decapsulating #420
Conversation
This file contains hidden or 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
achingbrain
force-pushed
the
feat/add-validation-function
branch
3 times, most recently
from
6977a41 to
df20b6b
Compare
Shifts validation to `validate` function which gives the user more control over when expensive validation operations take place. For example we currently run multiple regex passes over IP addresses when creating new multiaddrs which can cause CPU bottlenecks.
achingbrain
force-pushed
the
feat/add-validation-function
branch
from
df20b6b to
ec0635f
Compare
achingbrain
changed the title
|
Refs: ipshipyard/roadmaps#1 |
achingbrain
changed the title
SgtPooki
reviewed
Jun 13, 2025
Comment on lines
+12
to
+13
| if (value < 0) { | ||
| throw new ValidationError('Value must be a positive integer') |
There was a problem hiding this comment.
are we considering 0 positive? We should indicate that in the message,
Suggested change
| if (value < 0) { | |
| throw new ValidationError('Value must be a positive integer') | |
| if (value < 0) { | |
| throw new ValidationError('Value must be a positive integer, or zero') |
or guard against 0
Suggested change
| if (value < 0) { | |
| throw new ValidationError('Value must be a positive integer') | |
| if (value <= 0) { | |
| throw new ValidationError('Value must be a positive integer') |
Comment on lines
+17
to
+21
| export function maxValue (max: number): (value: any) => void { | ||
| return (value) => { | ||
| if (value > max) { | ||
| throw new ValidationError(`Value must be smaller than ${max}`) | ||
| } |
There was a problem hiding this comment.
Suggested change
| export function maxValue (max: number): (value: any) => void { | |
| return (value) => { | |
| if (value > max) { | |
| throw new ValidationError(`Value must be smaller than ${max}`) | |
| } | |
| export function maxValue (max: number): (value: any) => void { | |
| return (value) => { | |
| if (value > max) { | |
| throw new ValidationError(`Value must be smaller than or equal to ${max}`) | |
| } |
| @@ -0,0 +1,40 @@ | |||
| export const CODE_IP4 = 4 | |||
There was a problem hiding this comment.
Can you add an jsdoc comment here so someone else can understand how to check if these are valid or not? or how to add a new key in the future?
| */ | ||
| export const resolvers = new Map<string, Resolver>() | ||
|
|
||
| export type { Resolver } | ||
|
|
||
| export { MultiaddrFilter } from './filter/multiaddr-filter.js' | ||
|
|
||
| /** | ||
| * @deprecated DNS resolving will be removed in a future release |
Comment on lines
+11
to
+26
| function bench (m: typeof multiaddr | typeof multiaddr12): void { | ||
| const ma = m('/ip4/127.0.0.1/udp/1234/quic-v1/webtransport/certhash/uEiAkH5a4DPGKUuOBjYw0CgwjvcJCJMD2K_1aluKR_tpevQ/certhash/uEiAfbgiymPP2_nX7Dgir8B4QkksjHp2lVuJZz0F79Be9JA') | ||
| ma.encapsulate('/p2p-circuit/p2p/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC') | ||
| ma.decapsulate('/quic-v1') | ||
|
|
||
| const ma2 = m(ma.bytes) | ||
| ma2.encapsulate('/tls/sni/example.com/http/http-path/path%2Findex.html') | ||
| ma2.equals(ma) | ||
|
|
||
| ma2.getPath() | ||
| ma2.getPeerId() | ||
|
|
||
| const ma3 = m('/ip6/2001:8a0:7ac5:4201:3ac9:86ff:fe31:7095/udp/1234/quic-v1/webtransport/certhash/uEiAkH5a4DPGKUuOBjYw0CgwjvcJCJMD2K_1aluKR_tpevQ/certhash/uEiAfbgiymPP2_nX7Dgir8B4QkksjHp2lVuJZz0F79Be9JA') | ||
| ma3.encapsulate('/p2p-circuit/p2p/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC') | ||
| ma3.decapsulate('/quic-v1') | ||
| } |
There was a problem hiding this comment.
does this do any validation on the newer version? if not, it might be good to get a benchmark of the performance with the new validation to ensure there isn't a significant hit there..
Also, can we provide some recommendations for folks as to when & how to validate multiaddrs? i.e. only when reading multiaddr as a string, and then we don't need to again if using ma.* functions.. or whatever the case may be.
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.
Shifts expensive validation to
validatefunction which gives the user more control over operations take place.Skips validation when we create a multiaddr from an existing multiaddr.
Adds a
validatemethod that can perform explicit validation.For example we currently run multiple regex passes over IP addresses when creating new multiaddrs which can cause CPU bottlenecks.
Running the benchmark added in this PR it's an order of magnitude faster: