Skip to content

bootstrap-peers: add pocketlawb with a dialable p2p multiaddr - #297

Open
boyroywax wants to merge 3 commits into
Twigpine:mainfrom
boyroywax:add-pocketlawb-bootstrap-peer
Open

boyroywax wants to merge 3 commits into
Twigpine:mainfrom
boyroywax:add-pocketlawb-bootstrap-peer

Conversation

@boyroywax

@boyroywax boyroywax commented Aug 1, 2026 •

Copy link
Copy Markdown

Adds pocketlawb to the canonical seed list, with a dialable p2p_multiaddr — currently the only entry in the file that has one.

/dns4/node.pocketlawb.com/udp/7546/quic-v1/p2p/12D3KooWMGuHkbfJ9gTHL7dFozefF3PruxGMvRopE8prPC7eScNH

Why this might be useful beyond one more peer

Every existing entry has "p2p_multiaddr": null, so merge_into_vecs in crates/gitlawb-node/src/bootstrap.rs currently contributes zero addresses to config.p2p_bootstrap. Nodes come up with nothing to dial, and the Kademlia/Gossipsub layer stays idle while federation happens over HTTP. node.gitlawb.com reports connected_peers: 0 and advertises only loopback and private addresses, which is consistent with that.

There's no AutoNAT or identify-based external address discovery in the swarm, so a node cannot learn its own public address and publish it itself — the seed list is the only channel. This node runs on a plain VPS with a static public IPv4 and no NAT, so it can serve as a stable dial target.

Verification

Reachability was confirmed with an isolated node on a separate host and network (different provider region), configured so that a successful connection could only have come from this listener:

GITLAWB_BOOTSTRAP_DISABLE_SEEDS=true
GITLAWB_BOOTSTRAP_PEERS=
GITLAWB_P2P_BOOTSTRAP=/dns4/node.pocketlawb.com/udp/7546/quic-v1/p2p/12D3KooW…eScNH

With the embedded seeds disabled and no HTTP peers, the test node reported:

{"connected_peers": 1, "gossipsub_all_peers": 1}

gossipsub_all_peers: 1 indicates the full Noise + muxer handshake completed and the node joined the gitlawb/ref-updates/v1 mesh, not merely that a UDP packet arrived. The target reported the reciprocal connection. The test host was destroyed afterwards.

/dns4 rather than /ip4 is deliberate: libp2p_dns::tokio::Transport::system(quic) resolves at dial time, so the entry survives an address change without another PR.

Node details

DID did:key:z6MkiKcvf32z2tcNCGKscxmtszZqpBUrVFa82FTvPnfAhDNF
HTTP https://node.pocketlawb.com
Version 0.7.0
p2p QUIC-v1 on udp/7546, publicly reachable

GET /health returns {"status":"ok"} and /ready confirms the database pool. The node is federating and mirroring, and node.gitlawb.com lists it as reachable: true.

Happy to adjust the entry format or drop the updated bump if you'd rather manage that field separately.

Summary by CodeRabbit

  • Chores
    • Updated the bootstrap peer list with the latest timestamp.
    • Added the PocketLawb peer and its connection details.

Every existing entry has "p2p_multiaddr": null, so merge_into_vecs
contributes no addresses to config.p2p_bootstrap and nodes start with
nothing to dial. With no AutoNAT in the swarm a node cannot discover and
publish its own public address, so the seed list is the only channel.

This node is on a static public IPv4 with no NAT. Reachability was
verified from an isolated node on a separate host and network with
GITLAWB_BOOTSTRAP_DISABLE_SEEDS=true and no HTTP peers, which reported
connected_peers=1 and gossipsub_all_peers=1 — a completed handshake and
mesh join, not just an inbound packet.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 1, 2026 08:57
@github-actions github-actions Bot added the needs-issue PR has no linked issue label Aug 1, 2026
@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Thanks for the contribution. A couple of things will help us review this faster:

  • Link the issue this addresses (Closes #123). For protocol changes, open an issue first.

See CONTRIBUTING.md. Update the PR and these notes will clear automatically.

@coderabbitai

coderabbitai Bot commented Aug 1, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

Next included review available in 57 minutes.

View limit details

Limit details: You’ve used the included review currently available.

This review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9da5dcb3-b1d8-4c43-aebf-13236f08d9ca

📥 Commits

Reviewing files that changed from the base of the PR and between 424c03d and 8e6d969.

📒 Files selected for processing (1)
  • crates/gitlawb-node/src/bootstrap.rs
📝 Walkthrough

Walkthrough

The bootstrap peer registry timestamp was updated to 2026-08-01. The pocketlawb peer was added with its operator, DID, HTTP endpoint, QUIC multiaddress, and addition date.

Changes

Bootstrap peer registry

Layer / File(s) Summary
Update peer registry
bootstrap-peers.json
The registry timestamp changed to 2026-08-01. The pocketlawb peer metadata was added.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Suggested reviewers: copilot, kevincodex1

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes adding pocketlawb with a dialable P2P multiaddress to the bootstrap peers.
Description check ✅ Passed The description explains the change, motivation, implementation details, and independent reachability verification, but omits the template headings and checklist items.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Updates the Gitlawb network’s canonical bootstrap seed list to include a new peer (pocketlawb) with a dialable libp2p QUIC-v1 multiaddr, enabling nodes to actually populate config.p2p_bootstrap and attempt P2P connections at startup.

Changes:

  • Bumped the seed list updated date to 2026-08-01.
  • Added a new bootstrap peer entry (pocketlawb) including a non-null p2p_multiaddr.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@bootstrap-peers.json`:
- Around line 45-52: Add a regression test covering the new pocketlawb
p2p_multiaddr through merge_into_vecs, rather than only parse_seed_list; verify
the embedded JSON or specific address is successfully parsed and added to
config.p2p_bootstrap without failure, reusing the existing bootstrap
configuration and test helpers.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 46c51ec2-cd6a-4fe4-84e9-ccfad732909b

📥 Commits

Reviewing files that changed from the base of the PR and between c83cbc5 and 424c03d.

📒 Files selected for processing (1)
  • bootstrap-peers.json

Comment thread bootstrap-peers.json

@beardthelion beardthelion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The pocketlawb multiaddr is well-formed for the merge path. Multiaddr::from_str accepts it, merge_into_vecs of the embedded JSON adds exactly that string to p2p_bootstrap, and GET https://node.pocketlawb.com/api/v1/p2p/info reports the same peer_id as the /p2p/ component. /health and /ready are ok.

Findings

  • [P2] Pin the embedded multiaddr through merge_into_vecs
    crates/gitlawb-node/src/bootstrap.rs:310
    Same ask as CodeRabbit. embedded_seed_list_parses_successfully only calls parse_seed_list. I corrupted pocketlawb's p2p_multiaddr to "not-a-multiaddr" and that test stayed green; merge_into_vecs then added zero p2p entries. Extend the embedded regression (or add a sibling) so parse_seed_list(EMBEDDED_PEERS_JSON) plus merge_into_vecs asserts counts.p2p >= 1 and the pocketlawb address is present. That is the load-bearing check for a dialable seed entry.

One process note, not a finding: this would be the first non-null p2p_multiaddr in the canonical seed list, and it would make a third-party node the fleet's first compile-time dial target. Public-node PRs are welcome for the HTTP seed path; landing a dialable multiaddr in every binary is a separate network-ops call. We are holding on that admission until we settle it on our side. The merge-path test above still applies either way.

Not an ask, recorded only: merge does not bind the optional did field to the /p2p/ PeerId. Trust for seed dial targets stays the PR review of this file. Operators can still set GITLAWB_BOOTSTRAP_DISABLE_SEEDS.

Parsing alone let a corrupted p2p_multiaddr stay green while the merge
silently skipped it; assert the embedded list yields at least one dialable
entry and that pocketlawb's address survives the real merge path.
@beardthelion
beardthelion dismissed their stale review August 31, 2026 22:57

Stale: reviewed 424c03d, head is now 8e6d969.

@beardthelion beardthelion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The round-1 ask landed. embedded_seed_list_merges_a_dialable_p2p_entry drives the real embedded JSON through parse_seed_list plus merge_into_vecs, and it is load-bearing for two corruption classes: I broke the address to an unparseable one and it went RED at bootstrap.rs:342, and I removed the entry and it went RED at bootstrap.rs:333, both while embedded_seed_list_parses_successfully stayed green. Baseline is 12/12. CodeRabbit's inline thread asked for the same thing and is resolved.

Findings

  • [P2] Pin the expected address as a literal, not a value read from the file under test
    crates/gitlawb-node/src/bootstrap.rs:348-351

    pocketlawb_addr is cloned out of the same embedded JSON the assertion then searches, so contains is true for whatever address the file happens to hold. Swapping the entry to a different but still parseable /dns4/attacker.example.com/.../p2p/12D3KooWDpJ7... leaves the suite 12/12 green. The guard binds presence and parseability, not identity, which is the corruption that actually matters here since a substituted host or peer id redirects the dial in every shipped binary rather than dropping it. The comment above the test claims it pins the specific address, so the prose overstates what the code checks. Hoist the string to a const and assert against that: with assert!(p2p_bootstrap.iter().any(|a| a == POCKETLAWB_ADDR)) the same swap fails, and the real address still passes. Prefer any over indexing first() so it survives a second dial seed being added ahead of this one.

Separately, and not a code finding: the admission question from round 1 is unchanged. Every seed on main still carries "p2p_multiaddr": null, so this would be the fleet's first and only compile-time dial target, and there is still no first-party dial seed and no written seed-operator bar. We are holding on that on our side, not on yours. The technical ask above applies either way, and GITLAWB_P2P_BOOTSTRAP remains the path for dialing the node today.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I found issues that need to be addressed before this is ready.

Why this PR has seen so many review rounds (and how to stop the drip)

This PR is small on paper (one JSON row + one test), but it sits at a contract boundary the repo had never exercised before: the first embedded seed with a non-null p2p_multiaddr. That activated two different review tracks that kept getting conflated:

  1. Production behavior — Does the pocketlawb entry parse, merge, and dial correctly? On head 8e6d969, yes. bootstrap-peers.json is well-formed, Multiaddr::from_str accepts the address, merge_into_vecs adds it to p2p_bootstrap, and your manual reachability check is credible. Nothing in merge_seeds, main.rs, or p2p/mod.rs needs changing for this PR's stated goal.

  2. Regression-test contract — What exact failure must CI block? That question shifted across rounds:

    • Round 0: Only embedded_seed_list_parses_successfully existed. A corrupted p2p_multiaddr could stay green at parse time while merge_into_vecs silently skipped it (by design — one bad entry must not poison the rest).
    • Round 1 (CodeRabbit / beardthelion): Add a test that drives the embedded JSON through parse_seed_list and merge_into_vecs, asserting at least one dialable p2p entry survives. You delivered embedded_seed_list_merges_a_dialable_p2p_entry. That ask is done.
    • Round 2 (beardthelion, still open): The new test's comment says it "pin[s] … the specific pocketlawb address," but the code reads that address from the same file it is guarding and asserts contains on the clone. That satisfies round 1 (merge path + presence) but not round 2 (identity pin). Reviewers re-opened because the test prose promised more than the code checks.
  3. Network-ops admission (not a code finding) — beardthelion round 1 and 2 both recorded that making a third-party node the fleet's first compile-time P2P dial target is a maintainer policy call, held on their side, separate from the technical test ask. GITLAWB_P2P_BOOTSTRAP and GITLAWB_BOOTSTRAP_DISABLE_SEEDS remain the operator escape hatches. Do not treat unresolved admission as something you must "fix" in code to clear review.

Root cause of the drip: each round fixed the previous narrow ask without locking the full test invariant up front. Reviewers (and bots) then re-scanned the wider bootstrap surface — HTTP announce, DID fields, gossipsub trust, live dialability — and surfaced items that are pre-existing or out of scope, which reads like endless feedback even when production code is fine.

What this review is asking for: one concrete test fix (below), then stop. I am not asking for additional rounds on DID binding, HTTP seed regression, merge_seeds wrapper tests, live dial probes in CI, or gossipsub hardening. Those are drift for this PR.


Merge readiness

  • [P2] Resolve open CHANGES_REQUESTED from beardthelion at head 8e6d969
    GitHub mergeStateStatus is BLOCKED. The remaining technical blocker matches the finding below: literal pin of the approved multiaddr in the regression test. Branch is current with main (no rebase needed). Checks pass.

Findings

  • [P2] Pin the approved pocketlawb multiaddr as a Rust literal, not a value read from the embedded JSON under test
    crates/gitlawb-node/src/bootstrap.rs:317-352

    What is wrong (precisely)

    The test embedded_seed_list_merges_a_dialable_p2p_entry does useful work for round 1: it proves the embedded list yields counts.p2p >= 1 after merge_into_vecs, which embedded_seed_list_parses_successfully alone cannot catch. I verified on head that corrupting the address to an unparseable string fails at the counts.p2p >= 1 assertion, and removing the pocketlawb row fails at the .find(|p| p.name == "pocketlawb") guard.

    What it does not catch — and what the comment at lines 325-326 claims it does — is substitution of the approved dial target with another parseable multiaddr. Today:

    let pocketlawb_addr = list.peers.iter()
        .find(|p| p.name == "pocketlawb")
        ...
        .p2p_multiaddr.clone().expect(...);
    
    assert!(p2p_bootstrap.contains(&pocketlawb_addr), ...);

    pocketlawb_addr and p2p_bootstrap both derive from the same EMBEDDED_PEERS_JSON input, so contains is tautological for any parseable value the file holds.

    Repro on head: change only the hostname in bootstrap-peers.json from node.pocketlawb.com to attacker.example.com (keep the same /p2p/12D3KooWMGuHkbfJ9gTHL7dFozefF3PruxGMvRopE8prPC7eScNH suffix). cargo test -p gitlawb-node embedded_seed_list_merges_a_dialable_p2p_entry still passes. Every shipped binary would dial the substituted host with no CI failure.

    This is a test-contract bug, not a production-runtime bug. merge_into_vecs and the pocketlawb JSON entry on head behave correctly.

    Root cause

    The test was written to close the parse-vs-merge gap (CodeRabbit's ask) but the comment was written as if it also locked identity of the network-approved address. Those are different invariants:

    Invariant Current test Literal-pin test
    Embedded JSON parses (sibling test) unchanged
    Valid p2p_multiaddr survives merge_into_vecs counts.p2p >= 1 keep
    pocketlawb row exists with a multiaddr .find("pocketlawb") keep
    Embedded file matches this approved multiaddr string not checked any(|a| a == CONST)

    Without the last row, any PR that changes the multiaddr string (host rotation, typo fix, malicious substitution) stays green as long as the new string parses — which defeats the stated goal of pinning the specific address.

    Fix (minimal, non-drifting)

    Hoist the maintainer-approved string to a const beside the other bootstrap tests and assert against that, not against a clone from the file under test:

    /// Canonical dial target for pocketlawb as merged from bootstrap-peers.json.
    /// Update this const whenever the embedded seed list entry is intentionally changed.
    const POCKETLAWB_P2P_MULTIADDR: &str =
        "/dns4/node.pocketlawb.com/udp/7546/quic-v1/p2p/12D3KooWMGuHkbfJ9gTHL7dFozefF3PruxGMvRopE8prPC7eScNH";
    
    #[test]
    fn embedded_seed_list_merges_a_dialable_p2p_entry() {
        let list = parse_seed_list(EMBEDDED_PEERS_JSON)
            .expect("embedded bootstrap-peers.json must always parse");
    
        let mut http_peers = Vec::new();
        let mut p2p_bootstrap = Vec::new();
        let counts = merge_into_vecs(list, &mut http_peers, &mut p2p_bootstrap);
    
        assert!(
            counts.p2p >= 1,
            "embedded seed list merged zero dialable p2p entries"
        );
        assert!(
            p2p_bootstrap.iter().any(|a| a == POCKETLAWB_P2P_MULTIADDR),
            "embedded bootstrap-peers.json must merge the approved pocketlawb multiaddr"
        );
    }

    Use any rather than p2p_bootstrap[0] or first() so the test still passes if a second dialable seed is added ahead of pocketlawb later.

    Also align the comment: either (a) keep the comment and apply the fix above, or (b) if you intentionally only want the parse-vs-merge invariant, rewrite the comment to say that explicitly and drop "pin the specific pocketlawb address." Option (a) is what beardthelion's open review requests and what stops the drip.

    What you do not need to do (explicit anti-drift list)

    To be clear — addressing the above should be sufficient for the technical merge gate. Please do not expand scope in response to review noise:

    • No merge_seeds(&mut Config) integration test — the wrapper only adds disable-env, logging, and config mutation; it was not changed by this PR and round 1 did not ask for it.
    • No HTTP URL regression for https://node.pocketlawb.com — sibling HTTP seeds have never had per-entry embedded tests; pocketlawb's HTTP path uses the same merge_into_vecs branch as the other five seeds.
    • No runtime DID ↔ /p2p/ PeerId binding — BootstrapPeer.did is documentary; trust for seed contents is PR review of bootstrap-peers.json (maintainer-stated policy).
    • No CI live dial / gossipsub handshake probe — your PR body already documents manual verification; offline parse+merge is the right CI boundary.
    • No changes to gossipsub validation mode, announce-back Unproven authority, or is_public_http_url gating — all pre-existing; this PR only adds another HTTP seed row like the others.
    • No linked issue required to fix the test — the needs-issue label is triage guidance, not a code defect.

    How to verify locally

    cargo test -p gitlawb-node bootstrap::tests

    Optional sanity check after the fix: temporarily change the hostname in bootstrap-peers.json without updating the const — the test should fail. Restore both — it should pass.


Closing note to the author

The production change (pocketlawb row + dialable multiaddr) looks sound. The remaining work is one test invariant that was underspecified across review rounds. Apply the literal const pin (or honestly narrow the comment if maintainers agree identity pinning is out of scope), push, and ask beardthelion to re-review. That should close the technical loop without opening new surfaces.

If maintainers are still holding on network-ops admission for the first embedded P2P dial target, that is a merge decision on their side — not something to solve with more code in this PR.

@euxaristia euxaristia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The entry itself checks out: the multiaddr is well-formed and /p2p/-pinned, the DID and PeerId are internally consistent (I verified the pre-#324 derivation reproduces the pinned PeerId exactly from the committed DID), the diff scope is clean at two files, and the merge-path test is a good start. But I think this needs to stay held, for three reasons:

  1. The test pins presence, not identity. embedded_seed_list_merges_a_dialable_p2p_entry clones the expected address out of the very file it guards, so substituting a parseable /dns4/attacker.example/... address would stay green. This is the same point raised in both standing reviews: hoist the expected address to a literal const and assert any(|a| a == POCKETLAWB_ADDR), and additionally bind the did field to the /p2p/ PeerId in merge, which currently does not check that they match.

  2. Pre-#324, the pin is anti-typo, not anti-impersonation. The libp2p keypair on current main is derived from the public DID string, so anyone can recompute the private key and answer at the pinned PeerId. And with #323 still open (gossipsub ref-updates admitted unsigned under ValidationMode::Permissive), making this peer every fresh node's first compile-time dial target also makes it, or anyone impersonating it, an unauthenticated injection channel into those nodes. Sequencing matters too: #324's migration rotates every PeerId on first start, which would stale this pin for all upgraded nodes, so landing #324 first is probably the right order anyway.

  3. This is a trust-escalation decision, not a code review: the shipped default would make a third-party operator the fleet's first p2p contact. GITLAWB_BOOTSTRAP_DISABLE_SEEDS and GITLAWB_P2P_BOOTSTRAP exist as escape hatches, but defaults ship dialable. That call belongs to the maintainers, and the two standing reviews suggest it is deliberately still open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-issue PR has no linked issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants