Implement Electrum Protocol v1.6

[tnull]

Closes #189.

I asked Claude Code to implement the changes necessary to update to the recently released Electrum Protocol v1.6.

Putting in draft for now until I got initial feedback (cc oleonardolima ValuedMammal). We also still need to change the test setup to a v1.6-compatible server to actually run the new tests against.

[tnull]

Hmm, test_local_timeout is currently failing, likely because we now need to successfully negotiate the version before returning the client. Though I have to say I don't fully understand what the test is for in the first place. Could someone explain what we try to assert there exactly?

[ValuedMammal]

Concept ACK

[tnull]

Alright, now updated the CI to use a v1.6 server for tests, fixed some minor things that came up, and opted to simply drop the test_local_timeouts test case as its dummy server implementation is incompatible with the v1.6 flow (that requires version negotiation). Let me know if you think that's acceptable @oleonardolima @ValuedMammal.

Undrafted, as this should be ready-to-go.

[ValuedMammal]

I did a first pass, overall it looks reasonable. I left some nits and a few questions. I'm curious if you've had a chance to test this in the wild.

In 38386fe: "feat: Automatically negotiate protocol version on connect"

The commit message refers to a server_version() method that "updates the stored version", but that seems to be hallucinated.

[tnull]

I did a first pass, overall it looks reasonable. I left some nits and a few questions. I'm curious if you've had a chance to test this in the wild.

No, we did not test this in the wild yet, though of course now the tests run against v1.6 mainnet servers, so they are somewhat an 'in the wild' test.

The commit message refers to a server_version() method that "updates the stored version", but that seems to be hallucinated.

Ah, not hallucinated, but this is a stale comment that referred to a previous approach that I ended up amending.

[tnull]

Addressed pending comments and force-pushed the following changes:

> git diff-tree -U2  485c4b0 a439485
diff --git a/src/raw_client.rs b/src/raw_client.rs
index d806a92..3feb754 100644
--- a/src/raw_client.rs
+++ b/src/raw_client.rs
@@ -47,5 +47,5 @@ use crate::types::*;

 /// Client name sent to the server during protocol version negotiation.
-pub const CLIENT_NAME: &str = "rust-electrum-client";
+pub const CLIENT_NAME: &str = "";

 /// Minimum protocol version supported by this client.
@@ -965,4 +965,5 @@ impl<T: Read + Write> ElectrumApi for RawClient<T> {
                 deserialized.headers.push(deserialize(&header_bytes)?);
             }
+            deserialized.header_hexes.clear();
             Ok(deserialized)
         } else {

[tnull]

@ValuedMammal @oleonardolima Any update here? Do you want me to make any further changes or is this good to go?

[oleonardolima]

@ValuedMammal @oleonardolima Any update here? Do you want me to make any further changes or is this good to go?

I'm doing another round of review right now, though I think this one needs to wait on #194 being merged and released first.

[tnull]

this one needs to wait on #194 being merged and released first.

How come? Can't they get released together?

[oleonardolima]

It looks good, thanks for working on this one. I left a few nits, that could also be addressed in a follow-up.

[oleonardolima]

partial tACK a439485

As a matter of getting it ready to merge, some commits could be reworded to conventional commits, and also the changelog could be added to the PR description.

I've currently tested it with old servers, and didn' find any regressions. I didn't test the new methods though, will need to spawn a server on my end to do it.

[oleonardolima]

tACK a439485

It only needs addressing the previous commments, other than that it looks ready to go. I've tested the new broadcast_package on testnet, through: ssl://testnet.qtornado.com:51002.

[tnull]

Thanks, amended to address the pending comments:

> git diff-tree -U2 a439485 638da77
diff --git a/src/api.rs b/src/api.rs
index ea59066..b3c0fef 100644
--- a/src/api.rs
+++ b/src/api.rs
@@ -302,6 +302,7 @@ pub trait ElectrumApi {
     /// Returns the minimum accepted fee by the server's node in **Bitcoin, not Satoshi**.
     ///
-    /// **Note:** This method is deprecated in protocol v1.6+. Use [`mempool_get_info`](#method.mempool_get_info)
-    /// instead, which provides `minrelaytxfee` along with additional mempool fee information.
+    /// **Note:** This method was removed in protocol v1.6+. Use
+    /// [`mempool_get_info`](#method.mempool_get_info) instead, which provides `minrelaytxfee`
+    /// along with additional mempool fee information.
     fn relay_fee(&self) -> Result<f64, Error>;

diff --git a/src/types.rs b/src/types.rs
index b83a47a..4a7d075 100644
--- a/src/types.rs
+++ b/src/types.rs
@@ -231,5 +231,7 @@ pub struct ServerFeaturesRes {
 ///
 /// This is returned as an array of two strings: `[server_software_version, protocol_version]`.
-#[derive(Clone, Debug)]
+
+#[derive(Clone, Debug, Deserialize)]
+#[serde(from = "(String, String)")]
 pub struct ServerVersionRes {
     /// Server software version string (e.g., "ElectrumX 1.18.0").
@@ -239,21 +241,10 @@ pub struct ServerVersionRes {
 }

-impl<'de> Deserialize<'de> for ServerVersionRes {
-    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
-    where
-        D: de::Deserializer<'de>,
-    {
-        let arr: Vec<String> = Vec::deserialize(deserializer)?;
-        let mut iter = arr.into_iter();
-        let server_software_version = iter.next().ok_or_else(|| {
-            de::Error::custom("expected server_software_version as first element")
-        })?;
-        let protocol_version = iter
-            .next()
-            .ok_or_else(|| de::Error::custom("expected protocol_version as second element"))?;
-        Ok(ServerVersionRes {
+impl From<(String, String)> for ServerVersionRes {
+    fn from((server_software_version, protocol_version): (String, String)) -> Self {
+        Self {
             server_software_version,
             protocol_version,
-        })
+        }
     }
 }

.. and also cleaned up the commit messages a bit to reduce on the bullet points...

[oleonardolima]

tACK 638da77

[luisschwab]

tACK 638da77

[ValuedMammal]

ACK 638da77

I just left a comment #190 (comment) about batch_estimate_fee.