Fix BIP 21 and BIP 72 ambiguities & mistakes

swansontec commented at 2:12 am on March 7, 2014: contributor

As mentioned in the bitcoin-development mailing list, I have discovered some ambiguities and mistakes while writing my own bip-21 parser. This pull request fixes the various issues.

BIP 21: Add a missing "&" rule to the ABNF grammar

The URI syntax uses an ampersand to separate query parameters,
but the ABNF grammar didn't reflect this fact.

Also, the type of grammar used here is "ABNF" and not "BNF".

9f3dc5d7a5

BIP 21: Mention RFC 3986 and character set 0ee7ff7bed

BIP 21: Precisely define the valid query characters c823eeb596

BIP 72: Reduce the amount of escaping needed

RFC 3986 obsoletes RFC 1738, which this BIP was wrongly referencing.
The new RFC requires far less escaping for query parameters.

820736896e

schildbach commented at 7:50 am on March 7, 2014: contributor

Is the ABNF grammar case sensitive? Because RFC 3986 is, afaik. This is relevant e.g. for the “bitcoin:” scheme, which could also be written as “BITCOIN:” or “BiTcOiN:”

laanwj commented at 7:50 am on March 7, 2014: member

See also #26

in bip-0021.mediawiki: in 820736896e outdated

38 (See also [[#Simpler syntax|a simpler representation of syntax]])
39 
40  bitcoinurn     = "bitcoin:" bitcoinaddress [ "?" bitcoinparams ]
41  bitcoinaddress = base58 *base58
42- bitcoinparams  = *bitcoinparam
43+ bitcoinparams  = bitcoinparam [ "&" bitcoinparams ]

sarchar commented at 8:08 am on March 7, 2014:

I know it’s pedantic, but RFC 3986 seems to imply that the query string can be empty, even if “?” is present. Thus,

0bitcoin:1JwSSubhmg6iPtRjtyqhUYYH7bZg3Lfy1T?

is a valid URI, according to RFC 3986, but would not match this grammar.

swansontec commented at 4:51 pm on March 7, 2014:

To fix this, we could add a second set of square brackets to bitcoinurn, like so:

0bitcoinurn     = "bitcoin:" bitcoinaddress [ "?" [bitcoinparams] ]

swansontec commented at 7:55 pm on March 7, 2014:

Also, the RFC only defines the general syntax of a URI, and the individual schemes are all a subset of that, each with their own restrictions. Therefore, it is perfectly valid for us to make an empty query string illegal in our scheme. It might be worthwhile to see what the reference client does with URI’s like this, and just match that behaviour.

laanwj commented at 7:06 am on March 8, 2014:

We accept empty query strings so just bitcoin:address?. Also the address can be empty in case of a payment request i.e. bitcoin:?r=.... But that’s a different BIP.

swansontec commented at 4:55 pm on March 7, 2014: contributor

@schildbach You are right, the scheme is case-insensitive, although the RFC strongly advises everyone to write it in lower case. On the other hand, we have to accept things like “BiTcOiN:”. I will add another patch to fix this.

schildbach commented at 5:01 pm on March 7, 2014: contributor

While you’re at it, parameter names are case insensitive as well afaik.

laanwj commented at 5:04 pm on March 7, 2014: member

The URI scheme identifier is case insensitive, however we only accept parameter names in lower-case.

schildbach commented at 5:10 pm on March 7, 2014: contributor

@laanwj I’m talking about RFC 3986 rather than current impls.

laanwj commented at 5:20 pm on March 7, 2014: member

I don’t think RFC 3986 says anything about the interpretation/comparison of parameter names. (see for example 6.2.2.1: The other generic syntax components [apart from scheme and host] are assumed to be case-sensitive unless specifically defined otherwise by the scheme)

swansontec commented at 0:55 am on March 11, 2014: contributor

@schildbach Ok, I have researched this a bit more, and yes, ABNF is case-insensitive. So, this means that both the scheme (good) and query parameter names (bad) are case-insensitive. The only way to make the query parameter names case-sensitive is to encode their ASCII values, like so:

0amountparam    = %x61.6d.6f.75.6e.74 "=" *digit [ "." *digit ]
1labelparam     = %x6c.61.62.65.6c "=" *qchar
2messageparam   = %x6d.65.73.73.61.67.65 "=" *qchar
3reqparam       = %x72.65.71 "-" qchar *qchar "=" *qchar

This is ugly, but conforms to what bitcoin-qt is doing.

swansontec commented at 1:26 am on March 11, 2014: contributor

Now that the grammar is free of syntax errors, it is possible to actually run it. There is an interactive ABNF tester at [http://coasttocoastresearch.com/interactiveapg]. The grammar from the BIP doesn’t define “digit”, “base58”, or “qchar”, so the following temporary definitions are still needed to make it work:

0digit  = %x30-39
1base58 = %x31-39 / %x41-48 / %x4A-4E / %x50-5A / %x61-6B / %x6D-7A
2qchar  = digit / %x41-5A / %x61-7A

laanwj commented at 7:33 am on March 13, 2014: member

Adding raw hex in the grammar description doesn’t make it easier to read for humans, which are the prime audience here. Maybe just add a mention that the parameter names are case sentitive.

BIP 21: Clarify case-sensitivity

The "bitcoin:" URI scheme needs to be case-insensitive according to the RFC,
but the query parameters should be case-sensitive according to the bitcoin-qt
client implementation.

6f15f98b12

BIP 21: Allow empty addresses and query parameters

Empty addresses should be valid for compatibility with  BIP 72.

Empty query parameters should also be valid, since they are harmless,
and clients already accept them.

With these changes, bitcoin:?& would be a valid (but useless) URI.

116129c687

swansontec commented at 8:01 am on March 13, 2014: contributor

Yes, @laanwj, I agree that this is harder to read. I have rebased the pull-request, reverting the change to the ABNF grammar and inserting a note instead.

laanwj commented at 12:13 pm on March 13, 2014: member

ACK

swansontec commented at 3:50 pm on March 18, 2014: contributor

There haven’t been any comments in a while. Perhaps this is ready to merge?

gavinandresen commented at 3:59 pm on March 18, 2014: contributor

ACK

laanwj referenced this in commit a3fe3270ac on Mar 18, 2014

laanwj merged this on Mar 18, 2014

laanwj closed this on Mar 18, 2014

real-or-random referenced this in commit 3a4f1688a6 on Aug 10, 2022

guggero referenced this in commit 309042716c on Sep 29, 2022

Fix BIP 21 and BIP 72 ambiguities & mistakes #30