Skip to content

feat(net): recognise eMuleAI peer capabilities and demux reserved frames - #1288

Open
kno wants to merge 16 commits into
amule-org:masterfrom
kno:feat/peer-capability-recognition
Open

feat(net): recognise eMuleAI peer capabilities and demux reserved frames#1288
kno wants to merge 16 commits into
amule-org:masterfrom
kno:feat/peer-capability-recognition

Conversation

@kno

@kno kno commented Sep 4, 2026

Copy link
Copy Markdown

Refs #1177

This is piece 1 of the seven items listed in #1177 — peer capability recognition — offered on its own and first, as requested in that thread.

What this adds

Reading of eMuleAI's vendor capability tags from a peer hello, and classification of OP_UDPRESERVEDPROT2 frames by frame type instead of treating them as malformed traffic.

  • src/PeerCapabilities.h — the CT_MOD_MISCOPTIONS bitfield: the five bit positions eMuleAI defines, a decoder that masks reserved bits out of what a peer sends, per-bit accessors, and the display text for each bit.
  • src/ReservedProtocolFrames.h — classification of the byte after 0xB2 into truncated / known-type / unknown-type, the payload window handed to a handler, and a rate limit so an unknown-frame flood cannot become a log flood.
  • The CT_MOD_IP_V6 (0xAE) and CT_MOD_SVR_IP_V6 (0xAF) tags, and the ip6 / bi6 Kad tag address encoding.
  • The accessor for the decoded word is KnownBits(). Its earlier name, ToWire(), invited the misreading eMuleAI network parity: interest, and the libutp vendoring question #1177 reports — that a bit added later would be stripped on send — when it only ever fed a log line and the EC tag below.
  • EC_TAG_CLIENT_MOD_CAPABILITIES (0x0632), so a peer's recognised capabilities are readable over External Connections. The Client Details dialog renders the same data, but it needs an X server; the EC tag is what makes the display checkable without one.

Wire format

Three constants are fixed by the peer implementation and are pinned in the tests as literals:

Constant Value
CT_MOD_MISCOPTIONS 0xAA
NAT-T bit within that word 1u << 1
OP_UDPRESERVEDPROT2 0xB2

All three match the maintainer's own reference branch, got3nks/amule@feat/nat-t-utp, which uses tag id 0xAA (src/include/tags/ClientTags.h:61), decodes bit 1 of that word as NAT-T support (MOD_MISCOPT_BIT_NAT_TRAVERSAL = 0x00000002, src/NatTraversal.h:110), and reads 0xB2 datagrams with a type byte following the protocol byte (src/ClientUDPSocket.cpp:188).

Beyond that reference, this piece adds:

  • The remaining bit definitions. The reference documents bits 0–3 as a comment and implements decoding for bit 1 only. Here all five bits eMuleAI defines — extended source exchange (bit 0), NAT-T (bit 1), IPv6 (bit 2), serving buddy pull (bit 3), QUIC NAT-T (bit 4) — are named, decoded and individually readable, with bits 5–31 masked off in both directions so no capability can be inferred from a reserved bit.
  • The 0xB2 frame-type classification in src/ReservedProtocolFrames.h. The reference reads a type byte on its own uTP path; this is a classification of the whole frame-type namespace (0x00 uTP, 0x01 QUIC, 0x02 caps, 0x03 caps-ack, 0xFF key) with an explicit disposition for a truncated frame and for an unregistered type, so an unknown frame is dropped as a known-unknown rather than counted against the sender.

The connect-options byte is deliberately not used

This piece signals nothing through the connect-options byte. As read out of eMuleAI's Opcodes.h:215-221 in the #1177 discussion, that byte claims 0x01, 0x02, 0x04, 0x08, 0x20, 0x40, 0x80, leaving 0x10 as its only free bit and nothing safe to take.

A reader coming from the reference branch will find CONNECT_OPT_BIT_NAT_TRAVERSAL = 0x80 there (src/NatTraversal.h:67). That is left out here on purpose, and for that reason: 0x80 is not free.

Not covered

  • No capability is negotiated. This piece only reads what a peer claims. LocalAdvertisedModMiscOptions() returns zero, so aMule advertises none of the five bits and emits no CT_MOD_MISCOPTIONS tag at all — an absent tag and an all-zero word mean the same thing to the peer, and the absent one costs no bytes. Negotiation is pieces 5–6.
  • No transport is implemented. All five bit positions are defined here, because recognising a peer's claim requires the whole layout, and a decoder that knew only some bits would silently mask the rest. But nothing stands behind them: IPv6, uTP and QUIC arrive with their own pieces, and each turns its own bit on in the change that ships it. A bit set here without a transport behind it is a defect the tests fail on.
  • No frame is handled. A known 0xB2 frame type is classified and then dropped, because the transports that would serve it do not exist in this tree.

Checks

Three new suites (PeerCapabilitiesTest, 11 cases; ReservedProtocolFramesTest, 7; ECClientCapabilitiesTest, 5) bring ctest from 48 to 51, all passing; clang-format 18 reports no violations on the 22 touched sources.

@kno kno closed this Sep 4, 2026
@kno kno reopened this Sep 4, 2026
@got3nks

got3nks commented Sep 4, 2026

Copy link
Copy Markdown

Read the whole diff, and checked every value it puts on or takes off the wire against the implementation that defines it: eMuleAI v1.6 (a2f05dc) for the capability bits and 0xB2 frame types, emule-qt (tip 2026-08-25) for the ip6/bi6 rules, eMule 0.72a-community for the opcodes in note 2. No defects.

surface result
MOD_MISCOPT_* bits 0-4 match eMuleAI's UModMiscOptions, same order
0xB2 frame types 0x00 0x01 0x02 0x03 0xFF match eMuleAI's dispatch
ip6 / bi6 decode case-insensitive, length pinned at 32, both required by emule-qt's spec
advertised word zero, static_assert-enforced

Merging while 3.1.0 is in flight

3.1.0 is close and master is its release line, while NAT-T and IPv6 are 3.2.0 work. So the question for merging now is not whether a piece is finished but whether a peer can observe it.

This PR touches the network in exactly three places, and all three are reads:

  1. Hello, both directions. A CT_MOD_MISCOPTIONS case in ProcessHelloTypePacket(), which is reached from ProcessHelloPacket() when a peer dials us and from ProcessHelloAnswer() when we dial them. Recorded, never acted on.
  2. Client UDP, unsolicited. An OP_UDPRESERVEDPROT2 case in OnPacketReceived(), routed to a frame classifier rather than ProcessPacket() because 0xB2's second byte is a frame type, not an opcode. Classified, logged, dropped.
  3. Kad search results, triggered by us. ip6/bi6 tags in ProcessResultFile(). Validated, logged, dropped.

SendHelloTypePacket() gains a static_assert and nothing else. Nothing here ever transmits, so aMule's output is byte-identical before and after and a 3.1.0 built from this is the same client to every peer on the network. That is a stronger property than "only affects inbound connections", since it holds for exchanges we start too.

The advertise side is what buys that: the assert binds bit-on, tag emission and tagcount together so a later change cannot half-do it. Per-type drop cases rather than a shared fallthrough serve the same end, and the note that a drop never reaches CPacketTracking shows the Kad flood-ban trap was considered.

For the pieces that follow, the ones that do change what we send: a compile-time gate defaulting off, not a long-lived branch. A branch must be synced against a fast-moving master, and that drift is what stranded the earlier NAT-T branch; a gate has nothing to sync, gets CI on every commit, and 3.2.0 becomes a one-line default flip. We would ask for two guards with it: CI building both gate states, and the release job asserting it is off on 3.1.x.

Stated once, the criterion for the series: each PR must be inert on the wire or gated off by default. Both are checkable from a diff.

1. The user-visible change

Client Details grows a "Vendor capabilities:" row on local and remote GUI, reading "Extended SX", "uTP NAT-T", "IPv6", "Buddy pull", "QUIC NAT-T" or "None". Catalogs are regenerated properly, POTFILES.in plus amule.pot plus all 40 .po files, so the strings reach translators with the change. No action needed; confirming rather than assuming, since a new string this close to a release is worth checking.

The capability names are bare literals while _("Vendor capabilities:") and _("None") are translated. That split reads right, since the names are protocol identifiers rather than prose, but one line saying so would stop the next person wrapping them in _() for consistency.

2. My guidance on the 0.72a opcodes named the wrong dispatch point

I asked in #1177 for 0xB3 and 0xD0-0xD7 to be recognised as known-but-unhandled and framed them as siblings of 0xB2. They are not: both are built as new Packet(data, OP_EMULEPROT) with ->opcode = ... and handled from the client-UDP opcode switch, so they arrive under 0xC5 and would never reach a 0xB2 classifier. Adding them there would have been dead code.

Fix: nothing in this PR. If the door is still worth propping open it is a case in CClientUDPSocket::ProcessPacket's opcode switch, which is a different change from the one I asked for.

3. MOD_MISCOPT_KNOWN_MASK silently drops emule-qt's bits

SetFromWire() masks to 0x1F, so bit 5 (MODMISC_EXTXS_SKIPTAGS) and bit 10 (MODMISC_HTTPCACHE) never reach m_bits. Correct today since aMule acts on neither, and wrong the moment it wants to gate on a peer's bit 5, where the capability would read as absent rather than unknown.

Fix: a comment at the mask naming what it drops and that widening it is the change needed. No code change.

Also checked

The EC split is right, tag code in ECCodes.abstract and only an accessor in ECSpecialTags.h. Search.cpp guards on IsStr(), and since utf8_str().length() is a byte count a multi-byte tag cannot pass the 32-character test. The tests pin wire literals as constants, so a silent renumbering fails the build rather than the network; AdvertisesNoUnimplementedCapability pins the invariant this PR's safety rests on.

@got3nks

got3nks commented Sep 4, 2026

Copy link
Copy Markdown

Following up on the review with a UI point I under-weighted first time. Nothing here is a defect, and none of it touches the protocol work.

The row is noise on almost every peer

GetDisplayText() returns _("None") for a peer with no vendor tag, and eMuleAI's install base is small enough that this is what nearly every user sees, on nearly every peer, permanently. A dialog gains a row that says nothing, for a population close to zero percent of the network.

Fix: hide the row when the capability word is empty, rather than showing "None". The label and value can both be skipped; wxSizer handles a hidden pair without leaving a gap.

"Vendor capabilities" is opaque in a way its neighbours are not

Client Details currently reads: Username, Userhash, Client software, Client version, IP address, User ID, Server IP, Server name, Obfuscation, Kad. Technical, but every row describes something aMule itself participates in, and the two jargon terms both have a home elsewhere in the UI, a preference and a tab.

"Vendor capabilities: Extended SX, uTP NAT-T, Buddy pull" is further out on two axes at once. The terms appear nowhere else in aMule, and aMule implements none of them, so there is no action attached. Your own comment says the line reads as "this peer would support X if we did", which is an odd thing for a user-facing string to be.

Fix, label: _("Protocol extensions:"). Accurate, since they are extensions to the eD2k protocol; neutral about who defined them; and it sits beside "Obfuscation:" and "Kad:" without standing out. A user who recognises none of the values still gets the category.

I would avoid naming eMuleAI in the string. Partly to keep a third-party name out of our UI, but mainly because the 0xAA word is not theirs alone: emule-qt already allocates bits 5 and 10 in it, so a vendor-specific label is wrong as soon as a second implementation shows up in the field.

Fix, values: spell them out. Once the row only appears when it has content, length costs nothing.

current suggested
Extended SX Extended source exchange
uTP NAT-T NAT traversal (uTP)
QUIC NAT-T NAT traversal (QUIC)
Buddy pull Buddy info pull
IPv6 unchanged

That reads as "Protocol extensions: Extended source exchange, NAT traversal (uTP)", which needs no glossary.

One consequence worth deciding now rather than after translators have seen the strings: the values are currently bare literals, and I said in the last review that this looked like the right call for protocol identifiers. Spelling them out makes them prose, which argues for wrapping them in _() after all. Either is defensible, but it is cheaper to pick before the catalogs carry them.

Not asking for

Another option I considered was dropping the GUI row entirely until aMule implements one of these, leaving the parsing, the EC tag and the tests to land as they are. I am not asking for that: the row is genuinely useful for diagnosing an interop question in the field, which is exactly the kind of thing this series will generate. But if you would rather defer it, nothing else in the PR depends on it.

@kno kno closed this Sep 4, 2026
@kno kno reopened this Sep 4, 2026
@kno
kno force-pushed the feat/peer-capability-recognition branch 3 times, most recently from 15b9f9b to fd02d34 Compare September 4, 2026 12:52
@kno

kno commented Sep 4, 2026

Copy link
Copy Markdown
Author

On the compile-time gate for the pieces that follow: agreed, and for the reason
you give rather than on principle — your feat/nat-t-utp at ~1250 commits
behind is the argument, and we have paid the same toll on a much smaller scale
today, where this PR needed three rebases in two hours and one of them broke the
build silently when #1271 replaced the dialog's m_client with a
ClientDetailInfo snapshot. Git merged it without a conflict; only a local
build caught it. Code behind a gate in master would have had that from your CI on
the commit that caused it.

Two questions before piece 2, both about who writes what.

1. The two guards — yours or ours?

They touch your workflows, so we would rather ask than open a PR against
.github/.

For CI building both states: ccpp.yml:129 already has a matrix keyed on
build_type, and the flag would reach the configure step at :150 where
-DCMAKE_BUILD_TYPE already is. So the smallest version is a second matrix
dimension — but whether that doubles six jobs or you would rather add one
dedicated gate-on job is your call on CI budget, not ours.

For the release job asserting it is off on 3.1.x: we have not read
release.yml closely enough to propose a shape, and an assertion in a release
path is the kind of thing whose owner should write it.

Happy either way: you add them and we build on top, or you tell us the shape you
want and we send it as its own PR, separate from any piece.

2. What the option is called, and its granularity

cmake/options.cmake names its existing switches BUILD_* for optional
binaries. A feature gate is a different kind of thing, so ENABLE_* may read
better — your naming call.

Granularity matters more than the name. One ENABLE_NAT_TRAVERSAL covering
pieces 5–6 keeps the flag count low but couples uTP to NAT-T, which your own
piece ordering deliberately separates. Two flags let uTP land and be exercised
while NAT-T is still off. We lean towards two, but you own the eventual default
flip and the support burden, so we will use whatever you name.

Piece 3, separately

You asked for the address widening to be split into a mechanical type
introduction that changes no behaviour, then the call sites. That splits cleanly
and we will send it that way. Worth flagging that piece 3 is not gateable in the
same sense: a family-agnostic address type is a refactor of existing code paths
rather than a feature with an off state, so its safety argument has to be
"changes no behaviour", not "off by default". If you would rather it came with a
gate anyway, say so now, because it changes how the first half is written.

@got3nks

got3nks commented Sep 4, 2026

Copy link
Copy Markdown

Three answers, and the third is simpler than you are treating it.

1. Guards

CI: one dedicated job, not a matrix dimension. A second dimension doubles every OS job for asymmetric value. Gate-off breakage is already caught by every existing job, since off is the default; only gate-on is uncovered. One Ubuntu job at one build type closes that, and a compile break in gated code will not be Debug-only or macOS-only in a way the rest would miss. Write it in the same PR as the first gated piece, so it never references a flag that does not exist yet.

Release assertion: not yet, and I am withdrawing my own request. The default is off and no release path passes the flag, so the only route into a release build is a packaging-script edit, which is a diff a human reads rather than something release.yml would naturally catch. It becomes worth writing when we flip the default for 3.2.0 and need it on the 3.1.x line.

2. ENABLE_*, and one flag

ENABLE_*, not close: BUILD_* gates which binaries exist, and the file already has ENABLE_UPNP, ENABLE_IP2COUNTRY, ENABLE_NLS, ENABLE_MMAP, ENABLE_BFD, ENABLE_VERSION_CHECK.

One flag, ENABLE_NAT_TRAVERSAL, and it requires libutp. You leaned towards two and I initially agreed, on the grounds that the coordination half needs no new dependency. That is true and it does not matter: the only configuration two flags buys is coordination with no transport, which punches a hole and then carries nothing. The one real user of it is a HighID relay forwarding for others, which is not a build anyone ships.

Under our feature-gating rule an explicitly requested option with a missing dependency must fail the configure, and here that is simply the right answer: if you asked for NAT-T, you need libutp. Splitting later costs nothing if QUIC ever makes a transport-level flag meaningful; carrying two now costs a combination nobody wants.

3. Piece 3 needs no gate

The criterion was inert on the wire or gated off by default. Those are alternatives, not a sequence, and a mechanical type introduction that changes no behaviour is the first branch.

Gating a refactor would be worse: two address representations alive at once, selected at compile time, doubling the surface every later piece reasons about and guaranteeing the disabled path rots. "Changes no behaviour" is checkable by tests and review in a way an off switch is not. Write the first half as you planned.

Still open from the reviews

Tracked rather than pressed. None is a defect, none blocks merge, and all four are unchanged since the reviews, which reads as sequencing.

  • Mask comment. The existing one is good on the masking; it does not say which real bits are dropped. emule-qt allocates bit 5 (MODMISC_EXTXS_SKIPTAGS) and bit 10 (MODMISC_HTTPCACHE), so widening is the change needed if aMule ever consumes one. A sentence.
  • Hide the capability row when the word is empty, rather than "None" on effectively every peer.
  • Relabel to _("Protocol extensions:"), no vendor name, since the 0xAA word is not eMuleAI's alone.
  • Spell the values out, and decide _() for them now that they read as prose rather than identifiers.

Your #1271 example is a better argument for gate-in-master than anything I wrote. A semantic conflict like that, m_client replaced by a snapshot with no textual overlap, is one git merges clean and only a compiler objects to. That is exactly what a long-lived branch hides until merge day, mixed in with everything else that accumulated. Code sitting in master behind a gate gets compiled on every commit, so the PR that caused it goes red instead.

@kno

kno commented Sep 4, 2026

Copy link
Copy Markdown
Author

All four are applied — pushed at 13:46 as e8edc4693, before my last comment,
which failed to mention them. Sorry for the wasted paragraph.

ask commit
Mask comment naming bit 5 / bit 10 7bfdc145d
Values spelled out and translated 238928b55
Row hidden, label relabelled b7017d2af
Catalogues regenerated e8edc4693

Three things in there you did not ask for and may want to push back on.

_() decided, but via wxTRANSLATE. The values are translated, as the prose
reading argues. Not with a literal _() though: the name table is static, so
_() in its initialiser translates once on first call — possibly before the
locale is loaded — and never follows a language change. It is
wxTRANSLATE in the table with wxGetTranslation() at the use site, which
extracts identically and keeps the table single, so a new bit cannot be added to
one half and forgotten in the other.

GetDisplayText() returns an empty string; _("None") is gone. Keeping it
would have left the dialog unable to tell absence from a real claim without
string-comparing a translated word. No test had asserted "None" — the only
assertion on that function was ECClientCapabilitiesTest.cpp:142, now
"IPv6, NAT traversal (QUIC)" — so two tests were added to pin what the dialog
now hides on: ClaimingNothingDisplaysAsEmpty (default-constructed, wire 0x0,
and reserved-bit-only 0x80 all render empty) and EachBitDisplaysItsOwnName.

A new control id, IDT_MOD_CAPABILITIES_LABEL. The label was created with
id -1, so it could not be found to hide. Appended at the end of the id space
rather than inserted, since the ids are bound by value.

Not verified: the dialog itself. No display session here, so "the sizer leaves no
gap behind a hidden pair" rests on wxSizer semantics plus the existing
Layout() call, not on an observed dialog. Worth a glance if you build it.

On your three answers: one dedicated gate-on job written in the same PR as the
first gated piece, ENABLE_NAT_TRAVERSAL as a single flag failing the configure
without libutp, and piece 3 ungated on "changes no behaviour". All clear, nothing
further from us. Piece 2 next.

@got3nks got3nks 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.

Reviewed the whole diff, built the monolithic GUI on macOS, and ran the suite: 51/51, including the three new ones. I checked the wire constants against the eMuleAI and emule-qt sources rather than against the description, since that is the part with no runtime signal. CT_MOD_MISCOPTIONS 0xAA and CT_MOD_IP_V6 0xAE, the five UModMiscOptions bits in that order with Reserved : 27, the five OP_NATT_FRAME_* values, and "ip6" / "bi6" all match, and the IsHash() guard on 0xAE matches how eMuleAI both writes and reads it. The frame-length edge lines up too: a type-byte-only datagram is a valid zero-payload frame on both sides, and a protocol-byte-only one is dropped on both. The hidden capability row also leaves no gap, since Build() calls SetSizeHints() after OnInitDialog() has hidden the pair.

One constant does not hold up, and there is one gap.

  1. CT_MOD_SVR_IP_V6 (0xAF) is not a hello tag. It means the server's own public IPv6 and only ever appears in OP_SERVERIDENT. emule-qt is the only tree that parses it, inside its OP_SERVERIDENT loop and only to log it (ServerSocket.cpp:328, "informational, we still reach it on the address we dialed"). Nothing emits it at all, not eMuleAI, not ed2k-server. In eMuleAI it is a parked id: its one case (BaseClient.cpp:1210) sits inside the // RESERVED TAGS FOR THE FUTURE RELEASES OF THIS MOD --> / <-- block and only sets m_bUnofficialOpcodes = true for ghost-mod detection. eMuleAI implemented "advertise my own v6" (0xAE) and never implemented "learn the server's v6".

    So m_hasModServerIPv6 can only become true when a peer sends a tag nobody sends, in a packet where the reference implementation reads it as evidence against that peer. The field is unreachable by any honest peer, and those 16 bytes are attacker-chosen by construction, stored under a name that reads as server-authoritative and waiting for the dual-stack change to trust it.

    Fix: drop the case along with m_modServerIPv6, m_hasModServerIPv6 and the two accessors. A server IPv6 belongs in the OP_SERVERIDENT tag loop in ServerSocket.cpp when we get there, not in ProcessHelloTypePacket. For a v6 address carried in a hello the tag is CT_MOD_IP_V6 (0xAE) for the peer's own, or CT_EMULE_SERVINGBUDDYIPV6 (0xA0) for a buddy's, which eMuleAI parses as a hash and which pairs with the "bi6" Kad tag this PR already defines.

  2. amuleapi does not see the new field. src/webapi publishes per-client obfuscation status but not the capability word, so the EC surface and the REST surface now differ. Could you add EC_TAG_CLIENT_MOD_CAPABILITIES to the amuleapi client view in this PR, with the curl test and the docs/api/REFERENCE.md entry? Same masked word the remote GUI gets.

Everything else I stand behind, including CPeerCapabilities::Set() having no production caller yet. Keep it, it is the setter the change that ships a transport will want.

@got3nks

got3nks commented Sep 5, 2026

Copy link
Copy Markdown

Correction to my review above, and an addition.

I checked every constant this PR introduces against the reference sources, but I did not run the sweep in the other direction, and that is the one that decides whether this is a foundation we can build a client on that speaks to eMuleAI, emule-qt and the eMule 0.72a NAT-T edition. Not "is what is here correct" but "is what is correct here". Having now enumerated the hello tag surface of all three trees and diffed it against this PR, two tags are missing.

Tag id eMuleAI emule-qt 0.72a this PR action
CT_EMULE_SERVINGBUDDYIPV6 0xA0 parsed parsed n/a missing add
CT_MOD_MISCOPTIONS 0xAA parsed parsed n/a parsed keep
CT_MOD_YOUR_IP 0xAD parsed parsed n/a missing add
CT_MOD_IP_V6 0xAE parsed parsed n/a parsed keep
CT_MOD_SVR_IP_V6 0xAF reserved marker not in hello n/a parsed remove

0.72a parses no vendor tags in its hello at all, only the stock eMule set, so its NAT-T surface falls outside this file entirely. Which means the hello side is complete for all three once these land.

0xA0, add. It is the v6 counterpart of CT_EMULE_BUDDYIP 0xFC, which aMule already parses two cases above. eMuleAI reads it as a hash into m_ServingBuddyIP (BaseClient.cpp:1174); emule-qt into m_buddyIPv6, gated on isPublicIP(). It also pairs with the "bi6" Kad tag this PR already handles, so as it stands the buddy's v6 is read from Kad and ignored in the hello.

0xAD, add, and follow emule-qt rather than eMuleAI. The two references disagree here in a way that matters. eMuleAI accepts the integer form and will set its own public IPv4 from one peer's unverified claim. emule-qt refuses that outright ("The IPv4 form is ignored here"), accepts only the hash form, and feeds it to a corroboration tracker that needs several distinct peers to agree, keyed on the observed socket address rather than the self-declared user hash, because "a hash costs nothing to invent". Please take emule-qt's shape.

The 0xAF removal and the amuleapi request in the review above both still stand.

@got3nks

got3nks commented Sep 5, 2026

Copy link
Copy Markdown

All four look right on inspection: 0xAF is gone completely, 0xA0 is in with its value pinned, 0xAD takes only the hash form and corroborates it, and the capability word reaches amuleapi with the doc and curl-test entries. 43-client-protocol-extensions.sh being appended after 42 rather than slotted in the middle is the right call too, those sections share one daemon and an insertion reorders what the later ones see.

I checked the corroboration keying hardest, since the whole property rests on it. GetConnectIP() holds the observed peer address by the time the tag loop runs: Init() calls SetIP(m_socket->GetPeerInt()) and the incoming-connection constructor assigns m_socket before calling it, so the SetIP() after the tag loop is a refresh rather than the first assignment. An outgoing connection keys on the address we dialled, which the completed handshake verifies. Sound in both directions.

Could you rebase onto master? The branch conflicts, and GitHub will not build a merge commit for a conflicting branch, so no CI has run on any of this yet. The four conflicts are all in the amuleapi surface I asked you to add: src/webapi/Api.cpp, src/webapi/EventDiff.cpp, docs/api/REFERENCE.md and docs/api/EVENTS.md. #1291, #1292 and #1295 all landed there since your branch point. The protocol and EC parts merge clean.

I will do the full pass and a local monolithic build once it is green.

Reads eMuleAI's CT_MOD_* vendor tags and misc-options capability bits from a peer
hello, and dispatches OP_UDPRESERVEDPROT2 payloads by frame type instead of
treating them as malformed traffic. An unknown or unsupported frame type is
dropped without advancing the sender's flood or ban counters.

aMule advertises none of the five new bits: this change only teaches it to read
what eMuleAI sends. Unknown tags never abort a handshake.

Negotiated capabilities are surfaced both in the client-details dialog and as
External Connection tags, so they remain observable where no GUI can start.

(cherry picked from commit abb71be)
The three CT_MOD_* tag ids and the two Kad tag names are wire format shared with
eMuleAI, and a wrong value has no runtime signal: aMule would write its IPv6
address under an id the peer decodes as something else, or under a name the peer
never looks up, and the handshake would carry on regardless.

Pinned as literals rather than read from the symbol under test, so a
renumbering or a rename cannot pass.

(cherry picked from commit ab9f7ec, test file only; the rest of that commit is
Kad identity-rotation work that does not belong to this change.)
ToWire() never touched the eD2k wire; it only fed a debug log line
and the EC_TAG_CLIENT_MOD_CAPABILITIES tag for client-details display.
The real advertise path is AdvertisedModMiscOptions(), guarded by the
static_assert in CUpDownClient::SendHelloTypePacket(). The name invited
exactly the misreading amule-org#1177 reported: that a bit added
later would be silently stripped on send.

Also drop the mask in the renamed accessor. m_bits is masked once on
write, in SetFromWire(); Set() and Reset() never introduce reserved
bits, so re-masking on every read was dead code.

Reported in amule-org#1177, which this does not close: that issue
tracks eMuleAI network parity as a whole.

(cherry picked from commit 8f7aeed)
The Client Details dialog gained a 'Vendor capabilities:' row and one label per
capability bit; the catalogues had not been regenerated to carry them, and the
i18n check fails on that drift. Regenerated with scripts/update-po.sh; no
translation was hand-edited.
amule-org#1271 replaced the dialog's live CClientRef with a ClientDetailInfo snapshot so a
stored Known row can be shown without a session. The capabilities line still
read m_client, which no longer exists. They come from the peer's hello, so they
are session state: filled from the live client, shown as '-' for a row that has
no session, the same way Kad is just below.
The 0xAA capability word is not eMuleAI's alone. emule-qt allocates bit 5
(MODMISC_EXTXS_SKIPTAGS) and bit 10 (MODMISC_HTTPCACHE) in the same word,
and MOD_MISCOPT_KNOWN_MASK clears both on the way in, so nothing downstream
can see them.

That is the behaviour we want today, because aMule acts on neither and a bit
it cannot use is a bit it must not relay. It stops being right the moment
aMule wants to gate on one: a cleared bit reads as absent rather than as
unknown, and no query site can recover it. Say so at the mask, and name
widening it as the change that has to come first.

Comment only, no behaviour change.
The capability list rendered abbreviations aimed at someone who already
knows the wire format -- "Extended SX", "uTP NAT-T", "Buddy pull" -- and
left them untranslated beside a translated label, which invites the next
reader to wrap them in _() for consistency and get a half-translated row.

Spell them out as prose and translate them, since that is what they now
are. The names carry no protocol meaning: the bit positions are pinned as
literal words in PeerCapabilitiesTest and the bit-to-name pairing by the
new display test, so a reword or a translation cannot move a bit. Keep the
single bit-and-name table so a new bit cannot be added in one place and
forgotten in the other; wxTRANSLATE with wxGetTranslation() at use is what
lets the table stay one table, and it also avoids translating a static
initialiser once, before the locale is loaded, and never again.

GetDisplayText() now returns an empty string for a peer that claims
nothing, instead of _("None"). An empty list is an empty string, and the
caller is the only place that knows how to present the absence -- the
dialog hides its row on exactly this contract, and could not tell "None"
apart from a real claim. Pinned by ClaimingNothingDisplaysAsEmpty, which
also covers a peer that sets only a reserved bit.
Two changes to the same row in Client Details.

The label read "Vendor capabilities:", which named eMuleAI's word in the
UI. The 0xAA word is not theirs alone -- emule-qt allocates bits in it too
-- so a vendor-specific label is wrong as soon as a second implementation
turns up in the field. "Protocol extensions:" is accurate, since these are
extensions to the eD2k protocol, says nothing about who defined them, and
sits beside "Obfuscation:" and "Kad:" without standing out.

The row also showed a placeholder for every peer that claimed nothing,
which is very nearly every peer on the network, and "-" for every stored
row with no session. A row that permanently says nothing takes the space
of one that says something, so hide the label and the value together. The
sizer closes over a hidden pair and Layout() reflows the rest, so no gap is
left behind. The label needs an id of its own for that -- id -1 cannot be
found to hide -- appended at the end of the id space like
IDT_MOD_CAPABILITIES, since the ids are bound by value.
scripts/update-po.sh. Picks up "Protocol extensions:" and the five
spelled-out extension names, and drops "Vendor capabilities:" and the
abbreviations they replaced.
… all

Two corrections to the eMuleAI hello tags this branch recognises, both from
reading the reference trees rather than the tag names.

0xAF is dropped. The name CT_MOD_SVR_IP_V6 reads as the server's own address,
and recording it from a peer would state something the wire does not say. In
eMuleAI the id appears exactly twice in the whole tree -- the #define, and one
case inside the block bracketed by "RESERVED TAGS FOR THE FUTURE RELEASES OF
THIS MOD", beside CT_MOD_RESERVED_B, whose only effect is to flag the sender as
running an unofficial build. Nothing writes it and the server-ident path never
reads it. emule-qt does define it as an address, but as the server's own public
IPv6, sent server->client -- which is evidence about a server, not a claim a
peer gets to make about one. Reading it as either would be wrong, so it is not
named at all: 0xAF now falls through to the unknown-vendor-tag log like any
other 0xA? id, and why it is unnamed is written where the next reader will look
for it.

0xA0 is added. CT_EMULE_SERVINGBUDDYIPV6 is the v6 counterpart of
CT_EMULE_BUDDYIP (0xFC), which this file already parses two cases above:
eMuleAI reads it into m_ServingBuddyIP, emule-qt into m_buddyIPv6. It also
pairs with the "bi6" Kad tag this branch already decodes, so without it the
buddy's v6 address arrived over Kad and was dropped in the hello. Parsed as a
hash, the same shape as CT_MOD_IP_V6, and recorded for the dual-stack change:
aMule has no IPv6 stack, so like 0xAE nothing reads it yet.

The tag ids are pinned as literals in PeerCapabilitiesTest, where a
renumbering would otherwise have no runtime signal.
…elieving it

0xAD carries a peer's opinion of the address we are reachable at. The two
reference implementations disagree about what to do with it, and the
disagreement is the whole content of this change.

eMuleAI accepts the tag's integer form and sets its own public IPv4 from it:
one peer, unverified, decides what that client believes its own address to be.
A client that is wrong about its own address is wrong about whether it is
firewalled, about what it publishes to Kad and about where it asks to be
called back -- and none of that fails loudly. emule-qt refuses the integer form
outright and takes only the 128-bit hash form, then feeds it to a corroboration
tracker rather than believing it on one peer's word. This follows emule-qt.

The tracker lives in a header of its own, CUpDownClient being unreachable from
a unit test, and keys on the address the packet was observed arriving from
rather than on the sender's user hash: a hash costs nothing to invent, so a
single host could otherwise manufacture as many distinct corroborating peers as
any threshold demands. A routable source address cannot, because a reply has to
come back through it.

Three distinct observed addresses is the threshold. Two would be wrong -- two
source addresses is one dual-homed host, one host that reconnected on a new
lease, or one attacker holding a second socket, none of which is a second
opinion. Three is the smallest count that forces a claimant to hold addresses
it does not control alone, and is still reachable in an ordinary session. It is
a floor rather than a proof, and the comment says so: three addresses under one
operator still agree with each other, which is acceptable only while nothing
acts on the result. The candidate table is bounded for the same reason the key
is the observed address -- the input is attacker-chosen.

Nothing consumes the corroborated address. Like the rest of this branch this is
recognition only: aMule has no IPv6 stack, and nothing about what it sends
changes.

The test suite states the rule rather than waiting for a symptom, since there
is no symptom to wait for: one peer is not believed, two are not either, one
host repeating itself is one host, disagreeing peers never pool into a quorum,
and a claim with no observed source address is ignored rather than counted
under a zero key.
amuled has carried EC_TAG_CLIENT_MOD_CAPABILITIES since this branch added it,
and the desktop GUI renders it as its "Protocol extensions" row, but the REST
surface published a peer's obfuscation status and not its capability word. The
two surfaces onto the same peer therefore disagreed about what was known of it,
and a headless caller had no way to see the word at all.

It reaches the client view by the path obfuscation status already takes: a
field on ClientSnapshot, decoded in the refresher off the INC_UPDATE wire,
written by WriteClientBaseFields so the list row, the detail object and the
client_* SSE payloads all carry it -- a field in the REST writer and not in the
diff writer is a row that silently stops updating, which is why EventDiff's
Equal() compares it too.

Taken as delivered, never re-derived. The daemon has already dropped the bits
it does not define (CPeerCapabilities::SetFromWire), and a second copy of that
mask in the webapi process would be free to drift from the one that actually
saw the handshake, at which point the surface reports something no peer ever
claimed. The curl test asserts exactly that: the word carries no bit outside
MOD_MISCOPT_KNOWN_MASK, on all three views.

A number rather than one of this surface's enumerated tokens, because it is not
an enumeration: a peer claims any combination of the bits, so there is no
single value to name. The key is unconditional -- 0 is a real answer here, and
what nearly every peer on the network reports.
0x0632 went to EC_TAG_CLIENT_CONNECTED in amule-org#1292 while this branch was open, and
the generated ECCodes.h then had two cases with value 1586, which the compiler
rejects. Moved to 0x0633, the first free code after the client block, and the
literal the test pins updated with it.
@kno
kno force-pushed the feat/peer-capability-recognition branch from 41559e8 to c2e7151 Compare September 5, 2026 13:08
Reinserted by hand while resolving the rebase against amule-org#1291 and amule-org#1292, so the
line wrapping no longer matched clang-format 18.
muleunit pulls in MuleDebug.cpp and StringFunctions.cpp, which use CFormat, so
every suite needs mulecommon whether its own code touches CFormat or not --
ReservedProtocolFramesTest links it for the same reason. macOS resolved the
symbols anyway; gcc and lld did not, so the link failed on Ubuntu and mingw-w64
in both build types and took both clang-tidy jobs down with them.
clang-tidy Tier-2 flags push_back(Candidate()). Its return value is unused:
emplace_back() only returns a reference from C++17, and the unit-test targets
carry no -std flag and take clang's default of C++14.
@kno

kno commented Sep 5, 2026

Copy link
Copy Markdown
Author

Rebased onto master. The four conflicts were all in the amuleapi surface, as you
said; the protocol and EC parts merged clean. Each was upstream adding something
next to what this branch adds, so both sides are kept: #1291's null-aware
WriteStringOrNull/JsonStrOrNull helpers and connected alongside
protocol_extensions.

Two things the rebase turned up that were invisible before it.

EC_TAG_CLIENT_MOD_CAPABILITIES had to move. 0x0632 went to
EC_TAG_CLIENT_CONNECTED in #1292 while this was open, so the generated
ECCodes.h had two cases with value 1586 and would not compile. It is 0x0633
now, with the literal the test pins moved with it.

PublicIPv6CorroborationTest needed mulecommon. muleunit pulls in
MuleDebug.cpp and StringFunctions.cpp, which use CFormat; macOS resolved
the symbols anyway, gcc and lld did not. That broke four builds and both
clang-tidy jobs, which is why the first CI run after the rebase was red.

Green now, 14/14.

@got3nks got3nks 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.

Rebased clean, 14/14 CI green, and a local monolithic build on macOS passes 52/52. Good catch on the EC code: 0x0632 really had been taken by EC_TAG_CLIENT_CONNECTED from #1295, and a duplicate there is a collision only a Debug build would have caught. Updating Equal() in EventDiff.cpp is the other easy one to miss, since without it the SSE diff suppresses every change to the new field. Appending 43- after 42 rather than slotting it in is right as well.

Three things.

1. The corroboration tracker is missing its primary control, and that is what makes its other gaps matter.

emule-qt filters a claim before tallying it: recordPeerObservedIPv6() drops anything not global unicast, then drops anything not assigned to a local interface, and only then records a vote. Its reasoning is the part worth quoting: "Peers are unauthenticated, so corroboration alone must never be able to make us advertise a foreign address. Requiring the candidate to be one we actually hold bounds the damage to 'picks the wrong one of our own addresses', which is exactly what this tier exists to disambiguate."

So the locally-assigned filter is the security control, and corroboration is only the tie-breaker between addresses we genuinely hold. Here corroboration is the whole of it, and three distinct source addresses is the entire bar for making aMule believe an address it does not own.

That also explains the candidate table. Because emule-qt only ever tallies addresses the host actually holds, its set is naturally tiny and needs no cap at all. PUBLIC_IPV6_CORROBORATION_MAX_CANDIDATES is standing in for the absent filter, and it is a poor substitute: refusing the ninth value permanently converts unbounded memory into permanent denial, and since occupying a slot costs one claim while corroborating a value costs three, denial is the cheaper attack and it never expires. Separately, the file has no timestamps at all, so "three distinct peers agree" really means "three peers have said this at any point since the daemon started", and a corroborated address is never re-validated against a rotating prefix.

Suggested shape, in this order:

  • Lift the interface enumeration out of PrefsUnifiedDlg.cpp into a shared translation unit the daemon links, and add the v6 branch. The cross-platform work is already done and already skips loopback: the Windows path asks for AF_UNSPEC and the POSIX path walks every family, and both then filter v6 out. It only has to move because amuled does not link muleappgui. LibSocketAsio.cpp holds a second copy of the same enumeration, so there is a call site to consolidate while you are in there.
  • Gate AddClaim() on global-unicast and locally-assigned before it tallies, and keep no per-candidate state for rejections, for the reason emule-qt gives.
  • Then add the window and re-election. Pass the time in rather than calling time() internally, the way CUnknownFrameLogThrottle already takes its tick, so the tests can fail.

With the filter in place the cap stops carrying weight it was never suited to, and the tracker shrinks to the job it is actually good at.

2. protocol_extensions is the only raw bitfield on the API surface. Every other multi-state field there is an enumerated token with an unknown fallback. An integer makes every consumer hardcode aMule's bit meanings, so PeerCapabilities.h's table gets reimplemented in JS for the Web UI and again in every third-party client. Your own justification, that a peer claims any combination of the bits rather than one state, argues for an array of tokens rather than an integer: ["extended_source_exchange", "ipv6"]. That reuses naming that already exists, and v0 has no compatibility to keep, so it is cheap now and awkward later.

3. Field order disagrees three ways. REST writes connected, protocol_extensions, friend_slot; the SSE serialiser writes obfuscation_state, protocol_extensions, connected; both docs show it after connected. Nothing breaks, JSON is unordered, but the sample payloads are meant to mirror real output.

@got3nks

got3nks commented Sep 6, 2026

Copy link
Copy Markdown

One thing my point 1 left implicit, and the obvious reading of "locally-assigned" would get wrong: that list has to be a refreshed cache. Not a startup snapshot, and not a re-scan per claim.

emule-qt's isLocalIPv6() is a plain in-memory lookup, so nothing enumerates on the corroboration path. A fresh scan is pushed in from three places: startup, every server connect, and a roughly 5 minute tick. The periodic one is there because they hit the gap without it: "Without this the only triggers are startup and a server connect, so a prefix renumber or a privacy-address rotation goes unnoticed for as long as the session stays connected, and we keep advertising an address we no longer hold."

What makes the cache safe is what a refresh does, not how often it runs: "The set of addresses we actually hold gates every reflected address, so publish it first. This call also re-validates any reflection already adopted, which is how a prefix renumber gets noticed." So a refresh is not only an allow-list update, it re-checks whatever was believed on the strength of the old list and drops what no longer holds. Without that half, a corroborated address outlives the prefix it came from.

Two details in their scan that aMule's IPv4-only enumeration has no reason to have yet: it skips tentative addresses, which are not ours until duplicate address detection finishes, and on Linux it reads /proc/net/if_inet6 for the per-address flags that getifaddrs() does not expose.

Re-scanning on every claim would be wrong in the other direction, since it puts an interface walk on a path any peer can drive.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants