`req-pj` behavior is described in the spec but not enforced in the code

[nothingmuch]

BIP 77 mentions using the BIP 21 req- prefix to compel the sender to use payjoin.

It looks like bip21/bitcoin_uri supports this handling of this, but the implied semantics in BIP 77 are that the sender should not fall back in case the receiver compelled payjoin usage, but this distinction is not exposed by the bip 21 parsing or PayjoinExtras struct, so the sender state machine doesn't know if pj or req-pj was used.

Secondly, on the theoretical/spec side of things, how should expiry be interpreted in the event req-pj was specified?

[shinghim]

This may be a stupid question, but from BIP-77:

A req-pj parameter key, as specified in BIP 21, may be used instead of pj to signal that Payjoin is required.

How should we be handling the case of a URI containing both pj and req-pj? Should we add another variant to InternalPjParseError similar to InternalPjParseError::DuplicateParams?

[nothingmuch]

How should we be handling the case of a URI containing both pj and req-pj? Should we add another variant to InternalPjParseError similar to InternalPjParseError::DuplicateParams?

Hmm, on the one hand that could be seen as a special case of DuplicateParams, since the req- prefix has no semantic difference, but maybe it's cleaner to add a variant given how DuplicateParams contains just a single &'static str, but even that seems sufficient if we have a special case for describing this occurrence as just a constant in the code, it seems excessive to make another variant or make the nested type a String just to more clearly report this.

[DanGould]

IMO a URI containing both param and req-param needs to be handled by the bip21/bitcoin_uri crate as a single param parameter.

[nothingmuch]

That would still raise an error, which would need to be mapped to the InternalPjParseError, so I think the question stands.

In the current API this would be handled as just two separate params, one with the req prefix and one without. Currently this means req-pj is entirely not understood by our code, as these very rudimentary tests demonstrate:

diff --git a/payjoin/src/uri/mod.rs b/payjoin/src/uri/mod.rs
index 08c1bf8..10a0730 100644
--- a/payjoin/src/uri/mod.rs
+++ b/payjoin/src/uri/mod.rs
@@ -295,6 +295,21 @@ mod tests {
         );
     }

+    #[test]
+    fn test_required() {
+        assert!(
+            Uri::try_from(
+                "bitcoin:12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX?amount=0.01\
+                   &pjos=0&req-pj=HTTPS://EXAMPLE.COM/\
+                   %23OH1QYPM5JXYNS754Y4R45QWE336QFX6ZR8DQGVQCULVZTV20TFVEYDMFQC"
+            )
+            .unwrap()
+            .extras
+            .pj_is_supported(),
+            "Uri expected a success with a well formatted pj extras, but it failed"
+        );
+    }
+
     #[test]
     fn test_pj_param_unknown() {
         use bitcoin_uri::de::DeserializationState as _;
@@ -340,6 +355,15 @@ mod tests {
         ));
     }

+    #[test]
+    fn test_pj_duplicate_req_params() {
+        let uri =
+            "bitcoin:12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX?req-pj=HTTPS://BAD.COM&pj=HTTPS://EXAMPLE.COM/\
+                   %23OH1QYPM5JXYNS754Y4R45QWE336QFX6ZR8DQGVQCULVZTV20TFVEYDMFQC";
+        let pjuri = Uri::try_from(uri);
+        assert!(matches!(pjuri, Ok(_)));
+    }
+
     #[test]
     fn test_serialize_pjos() {
         let uri = "bitcoin:12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX?pj=HTTPS://EXAMPLE.COM/%23OH1QYPM5JXYNS754Y4R45QWE336QFX6ZR8DQGVQCULVZTV20TFVEYDMFQC";

Also note that the required_understood test also does not actually check required behavior, i created payjoin/bitcoin_uri#7.

[shinghim]

IMO a URI containing both param and req-param needs to be handled by the bip21/bitcoin_uri crate as a single param parameter.

I wouldn't think anyone is using both param and req-param and agree it should be a single param parameter, but since this behavior isn't specified in BIP-21, I don't think we should enforce it in the bip21/bitcoin_uri crate but rather untangle it ourselves in rust-payjoin

[shinghim]

Actually im not sure it actually can be parsed as one param, since it could have different values, but I think sorting out how to handle that should be done in rust-payjoin

[nothingmuch]

but I think sorting out how to handle that should be done in rust-payjoin

Not sure I agree, the API already assumes parameters are deserialized into a struct, i.e. it's not just tokenizing them.

For example, if there's a duplicate amount or label, which are represented as Option<Param<'a>> then, the last one just takes precedence, so parsing is not lossless:

diff --git a/src/lib.rs b/src/lib.rs
index c47a0ec..9413387 100755
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -384,6 +384,20 @@ mod tests {
         assert_eq!(uri.to_string(), input);
     }

+    #[test]
+    fn address_with_duplicate_name() {
+        let input = "bitcoin:1andreas3batLhQa2FawWjeyjCqyBzypd?label=foo&label=bar";
+        let uri = input.parse::<Uri<'_, _>>().unwrap().require_network(bitcoin::Network::Bitcoin).unwrap();
+        let label: Cow<'_, str> = uri.label.clone().unwrap().try_into().unwrap();
+        assert_eq!(uri.address.to_string(), "1andreas3batLhQa2FawWjeyjCqyBzypd");
+        assert_eq!(label, "bar"); // FIXME "bar" passes, but it should probably be neither (error saying duplicate) or both values
+        assert!(uri.amount.is_none());
+        assert!(uri.message.is_none());
+
+        assert_eq!(uri.to_string(), input);
+    }
+
+
     #[allow(clippy::inconsistent_digit_grouping)] // Use sats/bitcoin when grouping.
     #[test]
     fn request_20_point_30_btc_to_luke_dash_jr() {

although the bip 21 & 321 specs are ambiguous about duplicate values, and query parameters are not specified at all in RFC 3986, duplicate parameter keys are usually allowed on the web at least when i was writing this stuff...

so arguably either we need to change the URI api to have strict implementation of BIP 21 that supports duplicate parameter keys and makes them all available (via an iterator of params i guess?) or we should make it stricter about duplicates.

i think i prefer the latter due to its simplicity, and since i can't think of a legitimate use case for duplicate param keys in bip 21/321, which implies that we can leave the rust payjoin crate largely agnostic to this and that it should be an error in bitcoin_uri

[shinghim]

or we should make it stricter about duplicates

I think i agree with you on this part, especially when there are duplicates of an identical param (like in your example with two label params). I think it is a bit different with param and req-param since they're not technically the same param, and I am just leaning more towards the side of giving the end user the freedom (or burden) to implement it themselves as part of the deserialization of their Extras. I don't feel extremely strong on this position though, and I think whichever path we take, the most important part is to document it

[nothingmuch]

fwiw the ser and de modules are pub mod, so they are part of the public API.

if the deserialization impl was generalized from Uri to a pub trait that Uri impls, that consumes a stream of Param values, then the low level API would already be supported through this hypothetical trait without completely redoing the high level API

having that supported would further justify high level ergonomic features like treating req- as a modifier, not part of the parameter name.

should we move this discussion to a new issue in the bitcoin_uri repo?

[shinghim]

Good idea, created https://github.com/payjoin/bitcoin_uri/issues/8

[arminsabouri]

Moving this to 1.1 milestone as this is a internal refactor.