lightning / bolts

BOLT: Basis of Lightning Technology (Lightning Network Specifications)
2.11k stars 491 forks source link

BOLT11 HRP encoded amount #324

Closed dcousens closed 6 years ago

dcousens commented 6 years ago

I understand the rationale for the HRP encoded amount was:

The amount is encoded into the human readable part, as it's fairly readable and a useful indicator of how much is being requested.

However, I disagree that this is beneficial in a non-development environment, in fact, I reason that it will hurt users.

With this bech32 encoded string potentially being copied by users between software, as with BIP21 URI's and Bitcoin addresses presently, it is reasonable to assume the same mistakes will occur.

As a user, reading:

lnbc20m1pvjluezhp58yjmdan79s6qqdhdzgynm4zwqd5d7xmw5fk98klysy043l2ahrqspp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqfppj3a24vwu6r8ejrss3axul8rxldph2q7z9kmrgvr7xlaqm47apw3d48zm203kzcq357a4ls9al2ea73r8jcceyjtya6fu5wzzpe50zrge6ulk4nvjcpxlekvmxl6qcs9j3tz0469gq5g658y

Compared to

lnbc20n1pvjluezhp58yjmdan79s6qqdhdzgynm4zwqd5d7xmw5fk98klysy043l2ahrqspp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqfppj3a24vwu6r8ejrss3axul8rxldph2q7z9kmrgvr7xlaqm47apw3d48zm203kzcq357a4ls9al2ea73r8jcceyjtya6fu5wzzpe50zrge6ulk4nvjcpxlekvmxl6qcs9j3tz0469gq5g658y

Is very difficult to differentiate, and so help me if I don't realise that n is an m, and I end up accidentally end up paying 1000000x more than expected...

Yes software should warn users, but in my opinion, the human-readable prefix with multiple possible denominations on a bech32 encoded string is probably not the best way to warn them.

Maybe only use 1 denomination (millisatoshis) in the HRP?

I may have misunderstood something.

edit: I understand the above mutation of the HRP would have changed the resultant bech32 encoding, that is irrelevant to my point though, which is users [mis]reading that and thinking they know what to expect.

junderw commented 6 years ago

I agree m and n are easily misread for each other.

perhaps milli micro nano pico would be better?

mruddy commented 6 years ago

If I read the documents correctly, the multiplier is optional and the separator is '1'. Therefore, any user that would read the amount from the human-readable-part would also have to realize that lnbc11... is really 1 and not 11. If the amount is specified without a multiplier, is that a specification in satoshis (BOLT 11 did not specify)? Or is a multiplier actually effectively mandatory due to the "funding_satoshis" limit being 2^24 satoshis?

From my own experience on the testnet, I doubt users will actually read the encoded invoice for this amount info. My starblocks blockacchino invoice starts "lntb15u1...". That is nearly unusable, and gibberish to many people. The long encoded strings are just difficult and unpleasant to look at. From a development perspective, I was expecting the invoice amount to be part of the "data" portion of the "bech32" (but not actually bech32 compliant) string.

cdecker commented 6 years ago

This took some discussing during the last meeting. Your concerns have also been raised during the drafting phase of BOLT11. In the end we decided to keep the amount in the HRP because it allows some powerusers to actually understand part of the request, without relying on tools to decode it. However, it is but the first defense against wrong spends, since a payment cannot be performed without actually decoding the entire request, showing the details to the user and then getting permission to perform the payment. Most users will just consider the request an opaque blob that has to be copy-pasted anyway.

In the end we decided that it is too small of a concern to break the feature freeze for the v1 spec.

mruddy commented 6 years ago

@cdecker Two quick questions on BOLT 11: 1) Do you think it would be possible to clarify in the BOLT that if the multiplier is not included, the amount value specifies an integer number of millisatoshis, instead of the ambiguous, "number in that currency"? 2) Have you all discussed limiting the length of the invoices to something like 1024 characters so that the bech32 guarantee of detecting up to 3 errors within a window of 1023 characters applies (1023 + the 1 separator char would be 1024)?

cdecker commented 6 years ago

@mruddy since we use milli, micro, and nano modifiers the natural unit if the multiplier is omitted would be bitcoins, not millisatoshis.

As for the maximal length of the request: that's a good point, I think we are aiming for QR code size anyway, so "don't create requests that are too large" is definitely a good hint :+1:

mruddy commented 6 years ago

@cdecker Yes, I was thinking satoshis or bitcoins, but the c-lightning code uses millisatoshis if the unit is not specified. So, there are at least 3 sensical interpretations and in my opinion, one should be specified in the BOLT. As to the max length, yes, it would help to bound it. I'm not trying to be pedantic, it's just that I was writing code to work with invoices and came across these questions.