bring back `DROP_CLIENT_UNKNOWN` as opt out?

[Overtorment]

Updated Electrum to recent version, and discovered that after #325 electrum rejects all the clients that dont start with version negotiaton first.
It used to be an opt-it via DROP_CLIENT_UNKNOWN env, but now its forced with no way to opt-out.
I quickly patched the production instance (basically unmerged #325), but would you be open to accept PR that reintroduces same configuration, but in 'opt out' fashion?

[cculianu]

I think it was a mistake to change the server to do this -- it should be as permissive as possible and if the client fails to version negotiate, it should assume the more permissive (most compatible) version number it supports such as baseline 1.4 or whatnot.

In Fulcrum we don't require version negotiation because I consider it bad design to require this esp. for one-off connections. It's on the client if his app works or doesn't work correctly given a server he decided to connect to -- requiring him to version negotiate to do anything is just unnecessarily rigid bad design.

If a client fails to version negotiate and then gets "unexpected" behavior as a result (such as a protocol version he wasn't expecting) -- that's on the client. The server doesn't need to worry about that -- thus enforcing this rule is kind of outside the concern of the server anyway.

[SomberNight]

This was agreed on as part of the protocol 1.6 changes. (though as noted there, by nature it is a breaking change affecting prior versions) So it is not specific to ElectrumX.

@Overtorment what is your client doing that it does not send version negotiation first? Why can't it be changed to do that first?

I quickly patched the production instance (basically unmerged #325),

That PR contains multiple commits. Please don't revert b2d6d04. That was done to prepare for future changes.

[Overtorment]

This was agreed on as part of the protocol 1.6 changes. (though as noted there, by nature it is a breaking change affecting prior versions) So it is not specific to ElectrumX.

@Overtorment what is your client doing that it does not send version negotiation first? Why can't it be changed to do that first?

I quickly patched the production instance (basically unmerged #325),

That PR contains multiple commits. Please don't revert b2d6d04. That was done to prepare for future changes.

our client (bluewallet) luckily implemented the protocol with proper version negotiation from the start, so all good here.

but our monitoring scripts broke. simple bash oneliners wont work too
(like echo '{"id": 1, "method": "blockchain.scripthash.get_balance", "params": ["716decbe1660861c3d93906cb1d98ee68b154fd4d23aed9783859c1271b52a9c"]}' | timeout 2 nc electrum1.bluewallet.io 50001 -i 1)

[cipig]

but our monitoring scripts broke. simple bash oneliners wont work too
(like echo '{"id": 1, "method": "blockchain.scripthash.get_balance", "params": ["716decbe1660861c3d93906cb1d98ee68b154fd4d23aed9783859c1271b52a9c"]}' | timeout 2 nc electrum1.bluewallet.io 50001 -i 1)

you can do version thingy on bash too: (echo '{ "id": 1, "method": "server.version", "params": ["whatever", ["1.4", "2.0"]] }'; echo '{ "id": 1, "method": "blockchain.headers.subscribe" }'; sleep 1) | ncat IP port

[SomberNight]

Right, bash, copied straight from my local notes:
printf '{"id": 1, "method": "server.version", "params": ["testing", "1.4"]}\n{"id": 2, "method": "blockchain.block.header", "params": [1413366]}\n' | nc testnet.qtornado.com 51001

[SomberNight]

To give rationale for the breaking change, my main issue with the pre-1.6 behaviour is that it mandates something
that is actually ill-defined and not practically possible to remain backwards compatible with:

Old text (from before spesmilo/electrum-protocol#6):

When a connection is made, both client and server must initially
assume the protocol to use is their own protocol_min.

ElectrumX in the past regularly bumped its protocol_min. Electrs and Fulcrum, I guess, never did that as they are "too new" compared to the iteration of the protocol versions :) but at some point once they support many protocol versions, it becomes really tempting for server impls to bump protocol_min, to clean up the codebase. And while the letter of the old text allows this, I think it's really ugly. I would say each such bump is a backwards incompatible change.

In contrast, by enforcing version negotiation is done first, the behaviour is clear. As a client, getting back a proper error message at the start is preferable to silent behaviour changes later in the stream (due to the server's protocol_min changing).

---

I think if it's just your own monitoring scripts, it would be cleaner to update them.
If we really want, we could do what you suggest and add back DROP_CLIENT_UNKNOWN as an opt-out in the server. Certainly anyone could already patch that in anyway, just as you already did with the revert. But do we really want that? I am not fully convinced.

[cculianu]

I think it's really ugly

Ugly is subjective. I think it's ugly to require one-off scripts to include an extra version message when in the past they did not need to. Makes the protocol unnecessarily inelegant -- and "ugly".

You traded rare forms of ugliness that happen once every 5 years for daily routing ugliness that happens everytime somone tries to hit an Electrum protocol server with a message from a script.

I think rare ugliness versus daily ugliness is preferable.

Don't you?

---

The extra version message buys you nothing practical, really. In the previous case (where version negotiation was not required) if the server no longer supports old methods, either unaware clients continue to work or they do not. If they continue to work -- great! -- No extra work is required by anybody. If they do not work, then someone needs to edit the scripts and make them work.

Now, with this change -- if the versions aren't matched up -- everything breaks every time more often than they need to. Often we bump the protocol version but old, basic methods are still there. This increases the frequency where systems that nobody touched for years suddenly "break"... that's less ugly? Maybe. To me that is more ugly. It's subjective.