Replace quirc and BC-UR libraries with lighter pure-C implementations#303
Replace quirc and BC-UR libraries with lighter pure-C implementations#303odudex wants to merge 5 commits into
Conversation
|
Hi @odudex awesome stuff! I have been irked by bc-ur for a long time (we also use it in gdk) - .the c++ impl is inefficient and unwieldy to work with, and the upstream maintainers are resistant to any kind of changes. Its long been a dream of mine to have time to rewrite it as clean C, so its great to see you've done exactly that :) The code size reduction and performance improvements are also very welcome, so I'm very interested in merging these changes. Swapping from our current impl would require us to fully review/test both libraries, which will take a while, and we'd almost certainly do this a library at a time, starting with bc-ur. So please bear with us as we review cUR first and then we will look at migrating to it in due course. |
|
Note I've cherry-picked the libjade change into master so you don't have to keep carrying it. |
|
Great to hear there's a demand for cUR! Your review and feedback are really welcome, and you can count on me to make adjustments if needed. |
|
I'd say for the purposes of keeping this more easily rebase-able, it would be a good idea to structure it as the following:
I'd leave the removal of the existing components out for now since nuking them after the migration is trivial, and rebasing with them deleted will be painful if there are changes there while this is under review. |
|
Commits structured as requested. |
| static bool ur_decoder_is_failure(ur_decoder_t* decoder) | ||
| { | ||
| return ur_decoder_is_complete(decoder) && !ur_decoder_is_success(decoder); | ||
| } |
There was a problem hiding this comment.
(Note this is a design nit from the reference impl and not a bug)
The reference API is pretty confusing I think, and could be made better in your impl. Given the following:
- is complete is either true or false, but an error can occur in either state, and if the error is fetched when incomplete it will return UR_DECODER_OK (even if the decoder is null, despite having an error code available for that)
- is success is either true or false but can only be true when is complete is true and if its false before then it doesn't mean the decode was not successful. Also no error when null as above.
- error code (get_last_error) handles null, but can also return OK when neither is complete or is success are true AFAICS.
I think a better design is to replace ur_decoder_is_complete/is_success/get_last_error with ur_decode_get_state(). The states being:
- OK: finished without error and a result is available (caller can call get_result() and expect a non-null return)
- PROCESSING: not finished, and no error yet encountered. Keep feeding the decoder more data.
- NO_RESULT: finished without decoding error but there is no result content (caller decides if this is an error condition or not)
- an error (i.e the already existing error codes)
The goal of such an API change would be that the caller is less likely to have logic errors (it would simplify the Jade code for example)
There was a problem hiding this comment.
The reference API is pretty confusing I think, and could be made better in your impl.
Thank you! I focused on performance optimizations but forgot to refine API from early sketches. Now is the time to make it more consistent and intuitive. I'll refactor it and let you know when I have something better!
There was a problem hiding this comment.
On the subject of performance, for esp32 float operations are much faster/smaller than double (float is cpu native while some double operations are emulated and require linking a support library). The same is probably not true on modern desktop/laptop processors however. It would be nice if the double type could be chosen at compile time (conditionally defining a ur_float_t typedef, for example).
I'm not immediately sure if this would change the resulting encoded data however. TBH I have no idea why this spec includes a floating point based sampler at all, it seems like completely useless overkill.
There was a problem hiding this comment.
On the subject of performance, for esp32
floatoperations are much faster/smaller thandouble
Right! I'll evaluate all number variables, and simplify to float or integer math wherever possible.
There was a problem hiding this comment.
Reading the spec, it it not possible to remove the double type from the decoder, since the encoder and decoder must have consensus on which parts have been xor-d together. So you also need the random number generator, sampler, ieee double support etc etc.
This is a stupidly over-complicated design. There is no reason at all why two messages of the same length but with different content should have their fragments xor-d in a different (random) order. The packets to xor should be deterministic based only on the number of packets, which would mean no random numbers, sampling or doubles required.
So, you can't get rid of a lot of that spec garbage unfortunately, only try to make it more efficient...
There was a problem hiding this comment.
You're definitely not alone in your frustration with UR specs. While I like fountain encoders/decoders, there’s a lot room for simplification and efficiency, especially regarding the nested envelopes, redundant data headers and encoding.
BBQr is lightweight, has efficient encoding and compression, but it lacks lost-frame handling. If you're open to it, I can submit a PR to add BBQr support. It wouldn't add any UI/UX bloat, detecting and responding in the correct format can all be handled under the hood, similar to how Kern and Krux do it.
Longer term, Bitcoin would benefit from a new standard. Ideally, something like a BBQr with a modern, telecom-inspired data reconstruction algorithm to improve both performance and reliability.
There was a problem hiding this comment.
I'm all for adding support for widely used protocols, but I think in this instance we should focus on getting these two libraries in first, above everything else. With the musig/tapscript descriptor work as well as our upcoming internally planned features, we really have a lot on our plates at the moment...
|
Done: cUR:
Jade:
|
Perfect, this is exactly what I envisioned with the state based API :) |
Add cUR (https://github.com/odudex/cUR), a pure-C implementation of BCR-2020-005 UR encoding, pinned at the head of its main branch. No other changes - the source migration follows in the next commit.
Move the UR transport layer from the C++20 bc-ur library (and its C shim with placement-new sizing constants) to cUR, a pure-C implementation of BCR-2020-005, used as the UR transport envelope only - all payload CBOR remains TinyCBOR in bcur.c. - main/bcur.c and main/selfcheck.c moved to the cUR API: heap encoder/decoder handles, results borrowed from the decoder, fragments freed with free(). Encoder output is always uppercase. - collect_any_bcur() now replaces the decoder via qr_data->ctx on hard failure, and bcur_scan_qr() re-reads it after scanning. - selfcheck: a duplicate part is deduped before being counted, so the 'processed parts' expectation differs when the same part is presented twice. - Build with UR_ENVELOPE_ONLY to skip cUR's payload-type codecs; libjade links the same component's host static lib (bundled SHA-256, no mbedcrypto/wally dependencies). The esp32_bc-ur component is left in place (now unused) so it can be removed separately once the migration has settled.
Drive the animated-QR progress bar with ur_decoder_estimated_percent_complete_weighted() instead of the pure received/expected fragment counts. The weighted estimate also credits mixed fountain parts that have not yet resolved a pure fragment, so progress keeps moving where the count-based bar would stall at 'almost done' until the final reconstruction. Capped at 99% until the decode actually completes.
Add k_quirc (https://github.com/odudex/k_quirc), a rewritten ESP32-optimized QR decoder (bilinear/adaptive thresholding, span-based flood fill, SPIRAM-aware allocation), pinned at the head of its master branch. No other changes - the source migration follows in the next commit.
Rewrite the thin wrapper in main/qrscan.c to the k_quirc_* API, folding quirc_extract + quirc_decode into a single k_quirc_decode() and dropping the caller-allocated datastream scratch buffer (now managed internally). k_quirc_end() is called with find_inverted=false to preserve current behavior. Downstream callers use the qr_data_t / jade_camera_scan_qr / scan_qr wrapper API and are unaffected. Also update the three build references: main/CMakeLists.txt PRIV_REQUIRES, libjade include dirs, and the libjade.c amalgamated .c includes. The vendored esp32-quirc component is left in place (now unused) so it can be removed separately once the migration has settled.
|
cUR had a fix and improvements on recent push. |
|
@odudex I'm dubious about the cUR optimizations added TBH. e.g.:
IMO cUR is too young a project to be complicating the impl with micro-optimizations when you are still making high level changes like design optimizations and API cleanups, and still finding bugs. At the very least these changes should be (1) separate re-implementations of functions, not ifdef'd code complicating the existing functions (2) default to a reasonable size/speed tradeoff and (3) be gated by cpu defines only (so builders don't have to iterate the source files trying to determine which set of platform defines they need to set). I also think you might want to hold micro-optimizations back until the library has been completely tested/reviewed, although that last one is just IMO. Finally, please note that the github user greenaddress is no longer associated with Blockstream or Blockstream Green in any way. |
2 participants
This PR proposes, for your evaluation, replacing two libraries in the QR scan/display path:
components/esp32-quirc→ k_quirc submodule — QR decodingcomponents/esp32_bc-ur(C++) → cUR (C) submodule — BC-UR transport (bytewords, fountain codes, multi-part assembly)Both were originally developed for Kern and Krux projects. cUR is used envelope-only: all payload CBOR stays in TinyCBOR in
main/bcur.c, andmain/bcur.his unchanged, so no callers needed modification. A third small commit makes the libjadeESP_LOGmocks accept non-literal tags (k_quirc logs with aTAGvariable).Measured impact (esp32s3 dev config, vs master)
jade.binlibstdc++qr_qvga_*camera fixtures (desktop benchmark feeding the same 220×220 scan input to both decoders at-O2; per-image speedups 1.9–3.2×, decode parity 13/13).Compatibility and verification
UR:CRYPTO-PSBTparts, wire-format parity with bc-ur. Fountain fragments are additionally cross-validated in cUR's own test suite against foundation-ur-py output.processed_partsstays at 1 when the same part is presented twice (bc-ur counted 2).test_jade.pypasses via libjade, including the multi-partUR:CRYPTO-PSBTcamera-scan fixture.--psram --unamalgamated).Caveats
Motivation
If all goes well, give back from DIY community to the Jade project and at the same time get reviews to help harden these tools.