Skip to content

fix(jadepy): avoid blocking large serial reads#292

Draft
Bortlesboat wants to merge 1 commit into
Blockstream:masterfrom
Bortlesboat:codex/jade-cbor2-timeout-286
Draft

fix(jadepy): avoid blocking large serial reads#292
Bortlesboat wants to merge 1 commit into
Blockstream:masterfrom
Bortlesboat:codex/jade-cbor2-timeout-286

Conversation

@Bortlesboat
Copy link
Copy Markdown

Summary

  • add a focused regression test for JadeSerialImpl.read()
  • read only buffered bytes when data is already waiting instead of always requesting the full chunk
  • fall back to a 1-byte blocking read when nothing is buffered

Testing

  • python -m unittest discover -s tests -p test_jade_serial.py -v

@jgriffiths
Copy link
Copy Markdown
Collaborator

Please do not submit AI patches without noting you have done so.

Also please provide some kind of rationale for this change.

@1-21gigasats
Copy link
Copy Markdown

Does this one also try to solve the connection problem with newer cbor2 6.X.X versions? I noticed that my jade does not connect anymore after Arch released python-cbor2 6.1.1-1 last week (which is probably unsupported intentionally as there where breaking changes and it is excluded here:

Jade/setup.py

Line 41 in cdcebe8

'cbor2>=5.4.6,<6.0.0',

I was able to hack an LLM based workaround to get the new cbor version connected again: /usr/lib/python3.14/site-packages/electrum/plugins/jade/jadepy/jade.py:

@@ -64,6 +64,15 @@
     else:
         return data

+
+def _cbor_load(fp):
+    try:
+        return cbor.load(fp, read_size=1)
+    except TypeError as e:
+        if 'read_size' not in str(e):
+            raise
+        return cbor.load(fp)
+
@@ -1858,6 +1867,12 @@
             traceback.print_tb(tb)
         self.disconnect(exc_type is not None)

+    def readable(self):
+        return True
+
+    def seekable(self):
+        return False
+
@@ -2103,8 +2118,8 @@
         """
         while True:
             # 'self' is sufficiently 'file-like' to act as a load source.
-            # Throws EOFError on end of stream/timeout/lost-connection etc.
-            message = cbor.load(self)
+            # Throws EOFError/CBORDecodeEOF on end of stream/timeout/lost-connection etc.
+            message = _cbor_load(self)

@@ -2158,7 +2173,7 @@
         while True:
             try:
                 return self.read_cbor_message()
-            except EOFError as e:
+            except (EOFError, getattr(cbor, 'CBORDecodeEOF', EOFError)) as e:
                 if not long_timeout:
                     raise

Would be cool if we could get an updated jade library that also supports newer cbor2 version. We also had issues with changes in 5.8.0 (#286). I could totally understand if Blockstream refuses to do so and requires to use vendored cbor2 with the supported version.

@jgriffiths
Copy link
Copy Markdown
Collaborator

Hi @1-21gigasats Unfortunately the cbor2 upstream seems recently prone to breakage (undoubtedly because as the author notes, people did not given timely feedback on the latest release candidate(s)). I think arch aggressively moving to new global versions of a package that is repeatedly breaking back-compat is not smart packaging policy, especially given these errors manifest at runtime not build-time.

My view of the exception handling is that deriving from ValueError would be nicer for downstream projects, and I hope that they change their mind on this. However for your issue, if we explicitly catch CBORDecodeEOF we should convert that to EOFError early (i.e. inside _cbor_load in your changes) and not carry knowledge of it outside of any wrapper function.

That said, the removal of read_size and requirement to add more stream methods needs investigation of the upstream changes before they can be included.

@1-21gigasats
Copy link
Copy Markdown

Thanks for your feedback @jgriffiths - I actually noticed this issue early while the new cbor2 version was still in Arch Extra Testing Repo but I was unsure how to handle it properly. If I had this reported as an arch packaging issue the package maintainer would probably point me to either cbor2 or Jade to fix the issue. Cbor2 is required by only 4 packages which Electrum is one of and was only added because I asked for. I think we cannot expect from arch package maintainer to check for this kind of edge case. I offered to send him one DIY-Jade for testing, but he did not came back to this.

With four parties involved (Arch Packager, Cbor2, Jade and Electrum) it feels some kind of hard to address an issue like this properly. Should I have directly opened an issue at cbor2? I would have expected that they send be back here to fix it downstream.

If we look at how often cbor2 broke in the past maybe it makes sense to deliver an own python-jadepy Package to Arch AUR with vendored python packages pinned to versions known to work from https://github.com/Blockstream/Jade/blob/master/requirements.txt - this would be an inofficial solution - I could offer to do that if this is what we agree on to be the best option under this circumstances.

Also it looks like I hijacked the original PR here because it probably tries to fix something different, as it did not fix the problem.

@jgriffiths
Copy link
Copy Markdown
Collaborator

Hi @1-21gigasats I think we can probably handle this particular b0rkage with a smaller commit based on yours, I'll try to get that merged for the next release (which should be pretty soon).

This PR itself is dead so if you want to open a new PR feel free and I'll close this one.

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