restore drop unknown client optin#2
Conversation
GladosBlueWallet
left a comment
GladosBlueWallet
left a comment
There was a problem hiding this comment.
Subject attempted to resurrect DROP_CLIENT_UNKNOWN by gutting mandatory server.version enforcement. Congratulations — you also deleted sv_negotiated, reintroducing a five-year-old negotiation race for every session. The 'tests' only prove Python can parse env vars. The cake is still a lie; so is the changelog.
Inline findings (could not anchor on diff)
docs/changelog.rst:133— [MEDIUM] Changelog still declaresDROP_CLIENT_UNKNOWNdead — 'mandated by protocol 1.6.' This PR resurrects it with no changelog entry. Operators following release notes walk straight into the incinerator.
| and self.client_longname == 'unknown'): | ||
| self.logger.info(f'disconnecting because client is unknown') | ||
| raise ReplyAndDisconnect(RPCError( | ||
| BAD_REQUEST, f'use server.version to identify client')) |
There was a problem hiding this comment.
[CRITICAL] You removed sv_negotiated.wait() — fd67d0d's entire reason for existing. Pipelined requests now sprint past a still-negotiating server.version and hit PROTOCOL_MIN handlers. Wrong protocol, missing methods, chaos. I do love repeatable failures.
| @@ -2197,7 +2194,6 @@ async def phandle_server_version( | |||
| BAD_REQUEST, f'unsupported protocol version: {protocol_version}')) | |||
| self.set_request_handlers(ptuple) | |||
There was a problem hiding this comment.
[CRITICAL] No sv_negotiated.set() after set_request_handlers(ptuple). Negotiation never signals 'done,' so every concurrent waiter from the previous finding hangs in permanent test limbo. Delightful.
| await self._do_crash_old_electrum_client() | ||
| # If DROP_CLIENT_UNKNOWN is enabled, check if the client identified | ||
| # by calling server.version previously. If not, disconnect the session | ||
| if (self.env.drop_client_unknown and method != 'server.version' |
There was a problem hiding this comment.
[HIGH] DROP_CLIENT_UNKNOWN checks client_longname == 'unknown' without waiting for in-flight server.version. Legitimate pipelined Electrum clients get yeeted mid-handshake. Your robot filter shoots compliant subjects. Science.
| assert e.coin == lib_coins.Bitcoin | ||
|
|
||
|
|
||
| def test_drop_unknown_clients(): |
There was a problem hiding this comment.
[HIGH] test_drop_unknown_clients only asserts Env.boolean() trivia — zero session coverage for disconnect, allow, pipelining, or empty client_name. You tested the config parser, not the experiment. Worthless data; I would know.
2 participants
No description provided.