Qr optimizations #417

nothingmuch · 2024-11-28T02:15:46Z

These commits:

encode fragment paramters as bech32 strings (with no checksum), with 2 character param names in the human readable part, and + as a delimiter
- expiry time parameter encoded as a little endian u32, in line with bitcoin header timestamp and unix time nLocktime encoding. EX hrp
- ohttp has OH hrp, same byte representation as previously encoded in base64
- receiver key has RK hrp, same byte representation as previously encoded in base64
use bech32 with no human readable part for the subdirectory IDs
uppercase the scheme and host of the URI
move the pj so it follows the pjos parameter

Once the # %-escaping bug is fixed (see #373), and as long as no path elements between the host and the subdirectory ID, the entire pj parameter of the bip21 URI can be encoded in a QR using the alphanumeric mode. Closes #389.

I'm not sure about the last commit, although it seems sufficiently motivated since there is no purpose to clearing these values it doesn't actually simplify the code that much.

Manually verified, with manual % escaping of #, using the qrencode CLI encoder alphanumeric mode is indeed used for everything following pj=.

coveralls · 2024-11-28T02:25:53Z

Pull Request Test Coverage Report for Build 12127285091

Details

177 of 201 (88.06%) changed or added relevant lines in 9 files are covered.
5 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.2%) to 61.786%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-directory/src/lib.rs	15	18	83.33%
payjoin/src/uri/error.rs	0	4	0.0%
payjoin/src/ohttp.rs	15	20	75.0%
payjoin/src/uri/url_ext.rs	53	58	91.38%
payjoin/src/uri/mod.rs	38	45	84.44%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/uri/mod.rs	1	86.97%
payjoin/src/uri/error.rs	1	3.85%
payjoin/src/uri/url_ext.rs	3	91.07%

Totals
Change from base Build 12086002692:	0.2%
Covered Lines:	2857
Relevant Lines:	4624

💛 - Coveralls

nothingmuch · 2024-11-28T02:26:59Z

Note that ./contrib/lint.sh doesn't pass for all commits, the bech32 module has dead code before later commits are added. I don't see much value in bisectability of linting checks at the sub-merge granularity. ./contrib/test.sh passes for all commits.

payjoin/src/uri/mod.rs

payjoin/src/uri/url_ext.rs

payjoin/src/receive/v2/mod.rs

payjoin/src/uri/mod.rs

DanGould

This LGTM, just hoping to clarify some things left in comments

payjoin/src/uri/mod.rs

payjoin/src/uri/url_ext.rs

payjoin/src/receive/v2/mod.rs

payjoin/src/ohttp.rs

DanGould · 2024-12-02T18:22:49Z

Ready to merge after one change. bitcoin-0.32.5 exposes bitcoin::bech32 and we should use that.

No checksum or human readable part is included, resulting in 13 uppercase characters, which is amenable to QR code alphanumeric mode. In the directory, Instead of parsing subdirectory IDs to obtain a byte array, simply treat them as strings again. This is simpler than introducing a dependency on the payjoin workspace crate in order to parse them back into the ShortId type, since this is likely to be a negligible optimization of the redis serialized key size, and does not affect the key space.

These changes are to facilitate more efficient QR encoding, by exploiting the alphanumeric encoding mode. Use `+` instead of `&` as a parameter delimiter, as only the former is in the QR alphanumeric character set. Instead of encoding keys and values using `=` as a delimiter (which also not in alphanumeric set), the key is the human readable part of a bech32 string with no checksum. This effectively makes the `1` a key/value delimiter. The expiry time is serialized as a bitcoin consensus u32 value (little endian), and also encoded as a bech32 string instead of a decimal value.

Also places the `pj` parameter last. Following this change and assuming the bip21 # escaping bug fix is merged, the alphanumeric encoding mode can be used for the entirety of the (% escaped) pj parameter.

Clearing of the values is not used anywhere, and this simplifies their type signatures. PjUriBuilder left unmodified, there is a ticket for it already.

DanGould

ACK 32004db

DanGould · 2024-12-02T21:16:57Z

payjoin/Cargo.toml

 io = ["reqwest/rustls-tls"]
 danger-local-https = ["io", "reqwest/rustls-tls", "rustls"]

 [dependencies]
-bitcoin = { version = "0.32.4", features = ["base64"] }
+bitcoin = { version = "0.32.5", features = ["base64"] }


This update is missing from payjoin-directory, but I don't think it matters since the specified [^]0.32.4 allows for 0.32.5

oops, i missed that. i think this should be addressed in #418

in fact, cargo udeps points out payjoin-directory doesn't even use its bitcoin dep

DanGould · 2024-12-02T21:21:48Z

payjoin/src/uri/error.rs

@@ -40,7 +42,8 @@ impl std::error::Error for ParseReceiverPubkeyError {

        match &self {
            MissingPubkey => None,
-            PubkeyNotBase64(error) => Some(error),
+            InvalidHrp(_) => None,


Why not pass the source for this one?

The value is an invalid and untrusted input (but is at least a valid HRP per BIP 173), it doesn't impl std::error::Error so it's not a wrapped error but an error with the offending value included in it

These commits: 1. encode fragment paramters as bech32 strings (with no checksum), with 2 character param names in the human readable part, and `+` as a delimiter - expiry time parameter encoded as a little endian u32, in line with bitcoin header timestamp and unix time nLocktime encoding. `EX` hrp - ohttp has `OH` hrp, same byte representation as previously encoded in base64 - receiver key has `RK` hrp, same byte representation as previously encoded in base64 2. use bech32 with no human readable part for the subdirectory IDs 3. uppercase the scheme and host of the URI 4. move the pj so it follows the pjos parameter Once the `#` %-escaping bug is fixed (see #373), and as long as no path elements between the host and the subdirectory ID, the entire `pj` parameter of the bip21 URI can be encoded in a QR using the alphanumeric mode. Closes #389. The last commit seems sufficiently motivated. Since there is no purpose to clearing these values it doesn't actually simplify the code that much. Manually verified, with manual % escaping of `#`, using the `qrencode` CLI encoder alphanumeric mode is indeed used for everything following `pj=`.

nothingmuch marked this pull request as draft November 28, 2024 02:20

nothingmuch force-pushed the qr-optimizations branch from d4a1c8d to 60fcaae Compare November 28, 2024 02:22

nothingmuch marked this pull request as ready for review November 28, 2024 02:27

nothingmuch requested a review from DanGould November 28, 2024 02:27

nothingmuch commented Nov 28, 2024

View reviewed changes

payjoin/src/uri/mod.rs Show resolved Hide resolved

payjoin/src/uri/url_ext.rs Show resolved Hide resolved

payjoin/src/receive/v2/mod.rs Show resolved Hide resolved

payjoin/src/uri/mod.rs Outdated Show resolved Hide resolved

DanGould approved these changes Dec 1, 2024

View reviewed changes

payjoin/src/uri/mod.rs Outdated Show resolved Hide resolved

payjoin/src/uri/mod.rs Show resolved Hide resolved

payjoin/src/uri/url_ext.rs Show resolved Hide resolved

payjoin/src/receive/v2/mod.rs Show resolved Hide resolved

payjoin/src/ohttp.rs Outdated Show resolved Hide resolved

nothingmuch force-pushed the qr-optimizations branch 2 times, most recently from 77393fc to e8220af Compare December 2, 2024 03:08

DanGould mentioned this pull request Dec 2, 2024

Bump payjoin version to 0.21.0 #415

Merged

nothingmuch added 6 commits December 2, 2024 20:27

bump rust-bitcoin to 0.32.5 for bech32 re-export

6c4e40b

add NoChecksum bech32 helpers in their own mod

fa8475f

Uppercase scheme and host in pj uri

b26ae16

Also places the `pj` parameter last. Following this change and assuming the bip21 # escaping bug fix is merged, the alphanumeric encoding mode can be used for the entirety of the (% escaped) pj parameter.

Remove Option wrapper from fragment param setters

32004db

Clearing of the values is not used anywhere, and this simplifies their type signatures. PjUriBuilder left unmodified, there is a ticket for it already.

nothingmuch force-pushed the qr-optimizations branch from e8220af to 32004db Compare December 2, 2024 20:19

DanGould approved these changes Dec 2, 2024

View reviewed changes

DanGould merged commit c54870d into payjoin:master Dec 2, 2024
6 checks passed

nothingmuch mentioned this pull request Dec 2, 2024

Expose new shared payjoin::directory types #418

Open

DanGould mentioned this pull request Dec 3, 2024

Release 0.21.0 #390

Open

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Qr optimizations #417

Qr optimizations #417

nothingmuch commented Nov 28, 2024

coveralls commented Nov 28, 2024 •

edited

Loading

nothingmuch commented Nov 28, 2024

DanGould left a comment

DanGould commented Dec 2, 2024

DanGould left a comment

DanGould Dec 2, 2024

nothingmuch Dec 2, 2024

nothingmuch Dec 2, 2024

DanGould Dec 2, 2024

nothingmuch Dec 2, 2024

Qr optimizations #417

Qr optimizations #417

Conversation

nothingmuch commented Nov 28, 2024

coveralls commented Nov 28, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12127285091

Details

💛 - Coveralls

nothingmuch commented Nov 28, 2024

DanGould left a comment

Choose a reason for hiding this comment

DanGould commented Dec 2, 2024

DanGould left a comment

Choose a reason for hiding this comment

DanGould Dec 2, 2024

Choose a reason for hiding this comment

nothingmuch Dec 2, 2024

Choose a reason for hiding this comment

nothingmuch Dec 2, 2024

Choose a reason for hiding this comment

DanGould Dec 2, 2024

Choose a reason for hiding this comment

nothingmuch Dec 2, 2024

Choose a reason for hiding this comment

coveralls commented Nov 28, 2024 •

edited

Loading