Drop tor v2 onion production, keep wire codec faithful#10813
Drop tor v2 onion production, keep wire codec faithful#10813erickcestari wants to merge 6 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request removes support for Tor v2 onion services, which have been deprecated since lnd 0.20 and are no longer supported by the Tor network. While the node will no longer produce or accept new v2 addresses, it maintains backward compatibility in the wire codec to correctly handle and propagate existing peer-signed announcements that may contain v2 entries. This ensures that network graph integrity and signature validation remain intact for legacy data. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
erickcestari
force-pushed
the
tor-v2-cleanup
branch
from
9efa2a5 to
956c4cf
Compare
erickcestari
added
the
backport-v0.21.x-branch
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes support for legacy Tor v2 onion services while maintaining wire-level compatibility for existing network announcements. Key changes include the removal of the --tor.v2 flag, the deletion of v2-specific 'fake IP' encoding, and the addition of validation to reject v2 addresses at the input boundary. Feedback was provided to update test keys in tor/cmd_onion_test.go to use the colon separator required by the new production validation logic.
erickcestari
force-pushed
the
tor-v2-cleanup
branch
from
956c4cf to
148450b
Compare
github-actions
Bot
added
the
severity-critical
erickcestari
force-pushed
the
tor-v2-cleanup
branch
from
148450b to
3b46c0c
Compare
|
PR Severity: CRITICAL. Files: lnwire/writer.go (CRITICAL - lnwire wire protocol), server.go (CRITICAL - core server). Also touches graph/db/* and watchtower/* (HIGH), lncfg/, tor/, config.go, lnd.go (MEDIUM), docs and config files (LOW). 17 non-test files, ~365 non-test lines changed - no severity bump triggered. Label: severity-critical. <!-- pr-severity-bot --> |
erickcestari
force-pushed
the
tor-v2-cleanup
branch
from
3b46c0c to
e061c00
Compare
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
Close - guess the deprecation of the addr part has its own complexities.
erickcestari
force-pushed
the
tor-v2-cleanup
branch
from
e061c00 to
e92331d
Compare
erickcestari
force-pushed
the
tor-v2-cleanup
branch
2 times, most recently
from
58876f7 to
a09efe2
Compare
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
LGTM
The v2 removal boundary looks right, but the remaining v2 codec support should be documented more explicitly. Even though lnd no longer creates/dials v2, lnwire/graph decoding must
keep it for historical peer-signed node announcements and persisted graph data. Node signatures commit to the serialized address list, so dropping v2 there would change DataToSign()
and break validation after reconstruction/restart. Could we add a short comment near the remaining v2OnionAddr decode/preserve paths so this does not look like dead code later?
|
|
||
| // NodeFromWireAnnouncement creates a Node instance from an | ||
| // lnwire.NodeAnnouncement1 message. | ||
| // lnwire.NodeAnnouncement1 message. All wire addresses are preserved so |
There was a problem hiding this comment.
Nit: Maybe mention the exact use case, I honestly got a bit confused when reading the second part of the comment, it seems that wire addresses are somehow treated in a special way.
There was a problem hiding this comment.
I've updated it. Do you think is more clear now?
Tor stopped serving v2 onion services in October 2021; lnd should not produce v2 addresses anymore, but it must still verify signatures on and re-broadcast peer NodeAnnouncement messages that carry v2 entries. Stop accepting v2 as configuration input (lncfg), strip the legacy `--tor.v2` flag from the sample config, and remove the `tor.OnionHostToFakeIP` helper. The self-announcement builder now filters any v2 entry inherited from a previously stored self-node so upgraded lnd instances never re-emit their own v2 onions. For inbound announcements, keep the wire codec wire-faithful: `lnwire.WriteOnionAddr`, `graph/db.encodeOnionAddr`, and the matching decoders round-trip v2 bytes so `DataToSign` reproduces the bytes the remote peer signed, signature validation succeeds, and the announcement is persisted to the graph DB and re-broadcast across restarts byte-for- byte. RPC surfaces continue to expose the full address set so external tools can independently reproduce and verify the signed bytes. Add a netann regression test that signs a [v3, v2, ipv4] announcement, round-trips it through Encode/Decode, verifies the signature, and confirms the resulting models.Node preserves the v2 entry.
Describe the new wire-faithful storage behavior introduced by the previous commit so operators know what to expect from peer announcements that still carry v2 entries.
erickcestari
force-pushed
the
tor-v2-cleanup
branch
from
a09efe2 to
b866b0b
Compare
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
Took another closer look from another angle on the problem, found still some issues to clean up:
Another persisted-address path worth checking is the watchtower client DB.
New tower input is already covered because AddTower parses the address through lncfg.ParseAddressString, so a fresh v2 tower address is rejected. The remaining case is upgrade
state: a tower added before this PR could already be stored in the wtclient DB with a v2 onion address.
That stored address does not go through config/input parsing again. It is decoded as part of wtdb.Tower.Addresses, converted into a wtclient.Tower via NewTowerFromDBTower, then
later handed to the session negotiator/session queue dial paths. So an upgraded node can still attempt to dial an old persisted v2 tower address.
I think the best boundary is NewTowerFromDBTower: filter v2 onion addresses before constructing the address iterator. If filtering leaves no usable addresses, the tower should not
become an active dial candidate; we should leave the DB data intact and require the operator to add a fresh v3 address for that tower pubkey.
Continuing the v2 audit, the remaining risky paths are mostly dial boundaries rather than wire/storage paths.
Several subsystems can pass persisted addresses into outbound connection attempts:
- autopilot gets graph node addresses directly from
autopilot.ChannelGraphFromDatabase, thenpilot.goattempts to connect to*tor.OnionAddrwithout distinguishing v2/v3. - SCB restore uses addresses from the static channel backup / restored link node and calls
ConnectToPeerfor each address. - graph bootstrap uses graph-sourced node addresses and calls
connectToPeerdirectly. - persistent reconnection mostly already filters v2 through
withoutV2Onion, but it is still worth keeping in mind as another persisted-address consumer.
This is different from remote node-announcement storage: graph/lnwire should still preserve v2 so old signed announcements round-trip. But when the address leaves storage and becomes a
dial target, v2 should be skipped/rejected.
Potential areas to check:
- Autopilot
autopilot/graph.go exposes graph addresses directly:
addrs: n.Addresses
pilot.go then accepts any *tor.OnionAddr and tries to connect. If a remote graph node only has an old v2 onion, autopilot may still select and attempt to dial it. The graph data should
remain unchanged, but the autopilot connection path should skip v2 onion addresses.
2. SCB restore
chanrestore.go loops over backup/restored addresses and calls ConnectToPeer. If an old static channel backup contains a v2 onion, restore can still try it. Since v2 is no longer
routable on the Tor network, this should probably be skipped so restore proceeds to any remaining usable addresses. If all addresses are v2, restore cannot auto-connect and the operator
needs a fresh reachable address.
3. Graph bootstrap
discovery/bootstrapper.go samples graph node addresses and currently accepts any *tor.OnionAddr. This means graph-sourced v2 onion addresses can still be returned as bootstrap
candidates. The bootstrapper should skip v2 onion addresses while leaving graph storage unchanged.
4. Persistent peer reconnect
This path appears mostly handled already:
- startup link-node addresses use withoutV2Onion
- graph node addresses are skipped with isV2OnionAddr
- live topology updates skip v2
- fetchNodeAdvertisedAddrs filters v2
So I don’t see an obvious missing filter here, but it is still part of the same category: persisted addresses should be filtered before they become dial candidates.
Together with the wtclient-specific fix, this keeps the policy consistent:
- preserve v2 in wire/graph/db for historical signature fidelity;
- do not advertise v2 in our own node announcements;
- do not use v2 as a new dial target.
|
Thanks for the review @yyforyongyu and @ziggie1984! I've pushed 3 new commits with the fixes. I'll squash them when the review is finished. 69946e2 - 69946e2 - fdbe3bd - Filters v2 at the three remaining dial boundaries ziggie called out: autopilot ( |
erickcestari
force-pushed
the
tor-v2-cleanup
branch
from
711ca17 to
0a40749
Compare
| nodes []autopilot.Node | ||
| } | ||
|
|
||
| func (s *stubChannelGraph) ForEachNode(ctx context.Context, |
| return nil | ||
| } | ||
|
|
||
| func (s *stubChannelGraph) ForEachNodesChannels(_ context.Context, |
| // Strip persisted Tor v2 .onion entries: Tor stopped | ||
| // serving them in 2021 and the dial would never | ||
| // succeed. Covered by TestWithoutV2Onion. | ||
| addrs = withoutV2Onion(addrs) |
There was a problem hiding this comment.
Nit we should to do this before the len() = 0 check
4 participants
Context
This PR is split out from #10795 ("multi: remove deprecated RPCs and
config flags scheduled for 0.21"), which removes everything that was
announced for removal in 0.21 via the 0.20 release notes. That parent
PR was carved into smaller, independently-reviewable PRs by topic;
this is the tor v2 piece.
Tor v2 onion services have been obsolete since October 2021, when
the Tor network dropped support for them. The
--tor.v2flag was deprecatedin 0.20 with a scheduled removal in 0.21, and that's what this PR
ships.
lnd should not produce v2 addresses on any code path anymore, but it
must still verify signatures on and re-broadcast peer
NodeAnnouncementmessages that carry v2 entries alongside still-valid addresses, so we cannot strip v2 from the wire codec.
This PR draws that line: stop producing v2 everywhere, keep the codec
byte-faithful.
What changes
Stop producing v2
lncfg.ParseAddressStringrejects v2.onionstrings at theoperator-input boundary, so
--externalip,--listen,lncli connect <pubkey>@<v2>.onion, andlncli wtclient towers add <pubkey>@<v2>.onionfail fast with aclear error.
--tor.v2flag and its sample-config entry are gone (thedeprecation announced in 0.20).
previously stored self-node, so upgraded lnd instances never re-emit
their own v2 onions.
tor.OnionHostToFakeIP(the OnionCat v2 → fake-IPv6 helper) and thematching decoder cluster (
FakeIPToOnionHost,IsOnionFakeIP,onionPrefixBytes) are removed. The scheme is v2-only byconstruction — v3 decodes to 35 bytes and doesn't fit in an IPv6
address.
RSA1024:…) are rejected with the newErrNonV3OnionKeyrather than silently regenerating a fresh v3service, which would change the advertised onion identity.
tor.OnionTypeenum,AddOnionConfig.Type, andwatchtower.Config.Typeare removed — only V3 was left, the fieldwas a no-op pass-through.
Keep the wire codec faithful
lnwire.WriteOnionAddr,graph/db.encodeOnionAddr, and the matchingdecoders round-trip v2 bytes so:
DataToSignreproduces the exact bytes the remote peer signed,across restarts byte-for-byte.
RPC surfaces (
GetNodeInfo,DescribeGraph) continue to expose thefull address set so external tools can independently reproduce and
verify the signed bytes.
Breaking changes / operator-facing notes
This is a breaking change for any node still configured with v2. The
--tor.v2flag has been deprecated since 0.20 and v3 has been theonly working option network-side since October 2021, so there should
be no production node still relying on it, but the migration steps
are:
.onionentry inlnd.conf(underexternalip/tor.privatekeypath/wtclient.toweretc.) will now cause lnd torefuse to start. Remove the entry before upgrading.
v2_onion_private_key,or a renamed key whose contents begin with
RSA1024:) will causethe hidden-service setup to error out at startup with
ErrNonV3OnionKey. Delete the file to let lnd generate a fresh v3service.
still decode correctly through the wire-faithful codec, only new
operator input is refused.
GetNodeInfo/DescribeGraphcontinue to expose v2 entries when aremote peer's announcement contains them, so downstream tools that
independently verify announcement signatures keep working.