Skip to content

Conversation

@cong-or
Copy link
Contributor

@cong-or cong-or commented Dec 17, 2025


Fix: Restore is_pre_publish_completed for P2P Testing

Summary

Restore the original is_pre_publish_completed behavior to unblock P2P testing (just quickstart). The regression introduced in ff8b6d2d caused publishing to wait indefinitely for DHT providers that never appear in gossipsub-only test environments.


Problem

The P2P testing infrastructure (just quickstart) broke after commit ff8b6d2d:

  • ❌ HTTP POST requests timing out (exit code 52)
  • ❌ Publishing hangs at “Step 4/5: DHT not ready”
  • ❌ Error: Other DHT providers not found, aborting
  • ❌ Zero message propagation (0/5 nodes receiving)

Root Cause

Commit ff8b6d2d changed is_pre_publish_completed to require at least one other peer (not ourselves) to be registered as a DHT provider before completing a publish.

This assumption is invalid in P2P testing environments due to the difference between gossipsub and DHT provider announcements.

Content Distribution Flow

  1. ✅ Publishing node adds content to IPFS and announces itself as a DHT provider

  2. ✅ Publishing node broadcasts the message via gossipsub (PubSub)

  3. ✅ Subscriber nodes receive the message via gossipsub

  4. ❌ Subscriber nodes do not announce themselves as DHT providers

    • Nodes only become DHT providers when they explicitly fetch content from IPFS
    • Receiving content via gossipsub does not trigger provider announcements

Result:
The publishing node waits indefinitely for another DHT provider that never appears, blocking the HTTP handler for ~50 seconds before timing out.


Solution

1. Restore is_pre_publish_completed Logic

File: hermes/bin/src/runtime_extensions/hermes/doc_sync/host.rs

Reverted to the original behavior:

  • ✅ Returns true if we are found as a DHT provider
  • ✅ Returns true if other peers are found as providers
  • ❌ Returns false only when no providers exist

Finding ourselves as a provider confirms that:

  • DHT queries are functioning
  • Our provider announcement succeeded
  • The content is available on the network (at minimum from ourselves)

This matches the function’s documented contract and supports isolated P2P test networks.


3. Update Test Expectations

Updated the test case to reflect intended behavior:

#[test_case("OUR", &["OUR"] => true; "our peer is sufficient for P2P testing")]

Previously, this case incorrectly expected false.


4. Increase Bootstrap Initialization Wait

File: p2p-testing/justfile

Increased init-bootstrap peer ID discovery wait from 15s → 30s.

The original timeout was insufficient for nodes to fully initialize and log peer IDs, causing intermittent bootstrap failures on first run.


Testing Results

Before Fix

  • ❌ Exit code 52 (timeout)
  • ❌ 0/5 nodes receiving messages (0% propagation)
  • ❌ 1/30 peer connections

After Fix

  • ✅ Both health checks pass
  • 100% message propagation (5/5 subscribers)
  • Full mesh connectivity: 30/30 peer connections
  • Low latency: 72–347ms propagation
  • ✅ Reliable bootstrap initialization

The previous commit ff8b6d2 changed is_pre_publish_completed to require
at least one provider other than ourselves. This broke P2P testing because:

1. Publishing node announces itself as a DHT provider
2. Other nodes receive messages via gossipsub (PubSub)
3. But they don't automatically become DHT providers unless they fetch content
4. Publishing node waits indefinitely, causing HTTP timeouts (exit code 52)

This restores the original behavior where finding ourselves as a provider
is sufficient for DHT propagation verification, which matches the function's
documented contract: 'For P2P testing and small isolated networks, finding
ourselves as a provider is sufficient.'

Also updates the test case to expect true when only our peer is present.

Fixes: ff8b6d2 ("fix: correct is_pre_publish_completed to require other peers")
The 15-second wait was insufficient for nodes to initialize and log
their peer IDs, causing init-bootstrap to fail intermittently.

Increasing to 30 seconds ensures reliable peer ID discovery during
first-time setup and after 'just clean'.
Improved documentation to explain:
- The two conditions that return true (ourselves OR others as providers)
- Why finding only ourselves is sufficient in P2P testing environments
- The distinction between gossipsub propagation vs DHT provider announcements

This makes the function's behavior and reasoning more explicit.
@cong-or cong-or changed the title Fix p2p testing is pre publish chore(hermes): p2p testing is pre publish Dec 17, 2025
@cong-or cong-or added the rr: hermetics Required review by hermes team label Dec 17, 2025
@cong-or cong-or added this to Catalyst Dec 17, 2025
@cong-or cong-or moved this from New to 👀 In review in Catalyst Dec 17, 2025
@cong-or cong-or marked this pull request as ready for review December 17, 2025 12:11
@cong-or cong-or enabled auto-merge (squash) December 17, 2025 12:15
bkioshn
bkioshn previously approved these changes Dec 17, 2025
@bkioshn
Copy link
Contributor

bkioshn commented Dec 17, 2025

This passes when run locally 👍🏼

Fixes Clippy pedantic lint error requiring technical terms in
documentation to be properly formatted with backticks.
@cong-or cong-or requested a review from bkioshn December 17, 2025 14:14
@github-actions
Copy link

📚 Docs Preview

The docs for this PR can be previewed at the following URL:

https://docs.dev.projectcatalyst.io/hermes/fix-p2p-testing-is-pre-publish

@cong-or cong-or merged commit ab65fd2 into main Dec 17, 2025
56 checks passed
@cong-or cong-or deleted the fix-p2p-testing-is-pre-publish branch December 17, 2025 14:22
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Catalyst Dec 17, 2025
@cong-or cong-or restored the fix-p2p-testing-is-pre-publish branch December 21, 2025 00:13
@cong-or cong-or deleted the fix-p2p-testing-is-pre-publish branch December 21, 2025 21:27
@cong-or cong-or restored the fix-p2p-testing-is-pre-publish branch December 21, 2025 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rr: hermetics Required review by hermes team

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

3 participants