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: support http(s) multiaddr with no tcp port #122

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

NikolasHaimerl
Copy link
Contributor

@NikolasHaimerl NikolasHaimerl commented Mar 13, 2025

Description

This PR adds support for parsing multiaddresses in the direct DNS + HTTP(S) format, which doesn't include a TCP port segment.

Previously, our multiaddr parser could handled addresses with dns following the /dns/hostname/tcp/port/http(s) pattern, but we encountered addresses in the wild using the more concise format: /dns/hostname/http(s). The current implementation would throw an "unsupported protocol" error when encountering these valid addresses.

Changes

  • Added a condition to detect and properly handle DNS-based addresses with direct HTTP/HTTPS protocol specification
  • Extended test cases to cover the new format
  • Maintained backward compatibility with all existing multiaddr formats

Examples of newly supported formats:

/dns/meridian.space/https → https://meridian.space
/dns/meridian.space/http → http://meridian.space

Testing

All existing tests continue to pass, and new tests for the direct DNS format were added to verify the implementation. I've also manually verified that the updated parser correctly handles the problematic provider multiaddress that was failing in production.

Related Issues

CheckerNetwork/roadmap#250

Closes #121

@NikolasHaimerl
Copy link
Contributor Author

Manual test of manuel-check succeeds:

zinnia run manual-check.js

Calling Filecoin JSON-RPC to get PeerId of miner f03303347
Found peer id: 12D3KooWJ91c6xQshrNe7QAXPFAaeRrHWq2UrgXGPf8UmMZMwyZ5
Querying IPNI to find retrieval providers for bafyreiaxqptvdcxmyiwhb5kpvkmaxv5e3svniomf6ptvxvl7ypnmlrs22a
IPNI returned 4 provider results
Fetching: https://f03303347-market.duckdns.org/ipfs/bafyreiaxqptvdcxmyiwhb5kpvkmaxv5e3svniomf6ptvxvl7ypnmlrs22a?dag-scope=block
Testing HEAD request: https://f03303347-market.duckdns.org/ipfs/bafyreiaxqptvdcxmyiwhb5kpvkmaxv5e3svniomf6ptvxvl7ypnmlrs22a?dag-scope=block
Measurement: {
  cid: "bafyreiaxqptvdcxmyiwhb5kpvkmaxv5e3svniomf6ptvxvl7ypnmlrs22a",
  minerId: "f03303347",
  indexerResult: "OK",
  statusCode: 200,
  byteLength: 303,
  providerId: "12D3KooWJ91c6xQshrNe7QAXPFAaeRrHWq2UrgXGPf8UmMZMwyZ5",
  protocol: "http",
  providerAddress: "/dns/f03303347-market.duckdns.org/https",
  startAt: 2025-03-13T16:54:58.530Z,
  carChecksum: "1220d6337701df29192f0112e16220fa829b683ec87e3ad76d46bf938c31a472ac26",
  endAt: 2025-03-13T16:55:00.198Z,
  headStatusCode: 405
}

@NikolasHaimerl NikolasHaimerl changed the base branch from main to nhaimerl-smart-contract-support March 13, 2025 17:04
@NikolasHaimerl NikolasHaimerl marked this pull request as ready for review March 13, 2025 17:06
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Great start!

@NikolasHaimerl NikolasHaimerl changed the title fix: dns http url support fix: support http(s) multiaddr with no tcp port Mar 14, 2025
@NikolasHaimerl NikolasHaimerl requested a review from bajtos March 14, 2025 09:58
Base automatically changed from nhaimerl-smart-contract-support to main March 14, 2025 10:57
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

👏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

DNS multiaddr format support missing: /dns/hostname/https
2 participants