bcli: don't get upset at 0-value outputs

[darosior]

Calling bitcoind_getutxo() on testnet transaction https://blockstream.info/testnet/tx/eca4cefef2a98de5df67a6630c63af5a72ab57583e0f3bf066d976f28213f447?expand crashed my node.
EDIT: found another one https://blockstream.info/testnet/tx/21f95a5dd990e9caba09cbafe73514d9322bd8d28d70204dd2b5cce5c9c3f2a8 ^^

Fixes #3610

[darosior]

Maybe we should also filter out 0-value outputs in getfilteredblock ?
EDIT: cdecker answered on IRC:

Hm, shouldn't cause any issues at the moment, but eventually we want to support liquid assets (which'd have 0sats)
So it may be better not to filter them out

[rustyrussell]

We made a deliberate choice to expose all values in msat; older values can be deprecated at some stage. We were doing a lot of deprecating at the time, so we chose not to do it then.

Creating this function is dangerous; we should instead allow '0msat' to do what we expect in parse_amount_sat.

[darosior]

Ok, amended to modify parse_amount_sat() instead.

[rustyrussell]

No, that's wrong too!

parse_amount_sat() should accept msat amounts which end in 000, OR a literal 0. All it cares
about is that it's not rounding, and the "ends-in-000" test is a terrible way of doing that.

diff --git a/common/amount.c b/common/amount.c
index 7bb2e6d40..f5ae8dd62 100644
--- a/common/amount.c
+++ b/common/amount.c
@@ -181,6 +181,7 @@ bool parse_amount_msat(struct amount_msat *msat, const char *s, size_t slen)
  *  [0-9]+ => satoshi.
  *  [0-9]+sat => satoshi.
  *  [0-9]+000msat => satoshi.
+ *  0msat => 0 satoshi
  *  [0-9]+.[0-9]{1,8}btc => satoshi.
  */
 bool parse_amount_sat(struct amount_sat *sat, const char *s, size_t slen)
@@ -198,8 +199,11 @@ bool parse_amount_sat(struct amount_sat *sat, const char *s, size_t slen)
        if (!post_decimal_ptr && memeqstr(suffix_ptr, suffix_len, "sat"))
                return from_number(&sat->satoshis, s, whole_number_len, 0);
        if (!post_decimal_ptr && memeqstr(suffix_ptr, suffix_len, "msat")) {
-               if (!memends(s, whole_number_len, "000", strlen("000")))
+               if (!memends(s, whole_number_len, "000", strlen("000"))) {
+                       if (memeqstr(s, whole_number_len, "0"))
+                               return from_number(&sat->satoshis, s, whole_number_len, 0);
                        return false;
+               }
                return from_number(&sat->satoshis, s, whole_number_len - 3, 0);
        }
        if (post_decimal_ptr && memeqstr(suffix_ptr, suffix_len, "btc"))
diff --git a/common/test/run-amount.c b/common/test/run-amount.c
index 6b510277f..3b3e5596d 100644
--- a/common/test/run-amount.c
+++ b/common/test/run-amount.c
@@ -101,7 +101,7 @@ int main(void)
        PASS_SAT(&sat, "1000msat", 1);
        PASS_SAT(&sat, "1000000msat", 1000);
        PASS_SAT(&sat, "2100000000000000000msat", 2100000000000000ULL);
-       FAIL_SAT(&sat, "0msat");
+       PASS_SAT(&sat, "0msat", 0);
        FAIL_SAT(&sat, "100msat");
        FAIL_SAT(&sat, "2000000000000000999msat");
        FAIL_SAT(&sat, "-1000msat");
rusty@rusty-XPS-13-9370:~/devel/cvs/lightning (guilt/wumbo)$ git diff > /tmp/diff
rusty@rusty-XPS-13-9370:~/devel/cvs/lightning (guilt/wumbo)$ cat /tmp/diff 
diff --git a/common/amount.c b/common/amount.c
index 7bb2e6d40..f5ae8dd62 100644
--- a/common/amount.c
+++ b/common/amount.c
@@ -181,6 +181,7 @@ bool parse_amount_msat(struct amount_msat *msat, const char *s, size_t slen)
  *  [0-9]+ => satoshi.
  *  [0-9]+sat => satoshi.
  *  [0-9]+000msat => satoshi.
+ *  0msat => 0 satoshi
  *  [0-9]+.[0-9]{1,8}btc => satoshi.
  */
 bool parse_amount_sat(struct amount_sat *sat, const char *s, size_t slen)
@@ -198,8 +199,11 @@ bool parse_amount_sat(struct amount_sat *sat, const char *s, size_t slen)
 	if (!post_decimal_ptr && memeqstr(suffix_ptr, suffix_len, "sat"))
 		return from_number(&sat->satoshis, s, whole_number_len, 0);
 	if (!post_decimal_ptr && memeqstr(suffix_ptr, suffix_len, "msat")) {
-		if (!memends(s, whole_number_len, "000", strlen("000")))
+		if (!memends(s, whole_number_len, "000", strlen("000"))) {
+			if (memeqstr(s, whole_number_len, "0"))
+				return from_number(&sat->satoshis, s, whole_number_len, 0);
 			return false;
+		}
 		return from_number(&sat->satoshis, s, whole_number_len - 3, 0);
 	}
 	if (post_decimal_ptr && memeqstr(suffix_ptr, suffix_len, "btc"))
diff --git a/common/test/run-amount.c b/common/test/run-amount.c
index 6b510277f..3b3e5596d 100644
--- a/common/test/run-amount.c
+++ b/common/test/run-amount.c
@@ -101,7 +101,7 @@ int main(void)
 	PASS_SAT(&sat, "1000msat", 1);
 	PASS_SAT(&sat, "1000000msat", 1000);
 	PASS_SAT(&sat, "2100000000000000000msat", 2100000000000000ULL);
-	FAIL_SAT(&sat, "0msat");
+	PASS_SAT(&sat, "0msat", 0);
 	FAIL_SAT(&sat, "100msat");
 	FAIL_SAT(&sat, "2000000000000000999msat");
 	FAIL_SAT(&sat, "-1000msat");

[darosior]

Commited your patch..

fixes #3610

[kirelagin]

👍

Let’s get this merged? Seems to be quite a major issue, I’m not sure how it doesn’t show up more often.

[darosior]

Because you don't often see 0-value channel outputs on mainnet... ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ Le mardi, mars 31, 2020 12:41 AM, Kirill Elagin <notifications@github.com> a écrit :

…

👍 Let’s get this merged? Seems to be quite a major issue, I’m not sure how it doesn’t show up more often. — You are receiving this because you authored the thread. Reply to this email directly, [view it on GitHub](#3605 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3F6XFSOPJJ7G2AYGCFLRKENYXANCNFSM4LTMQYXA).

[darosior]

Let’s get this merged?

Btw, maybe not the best way to ask for progress.
Since you encountered this issue you could test it and report the result (I believe an ACK coming from someone who's not a contributor but ran in the issue solved by the patched is still welcome).
You would even be able to keep running the fix until we release it :-)

[kirelagin]

Oh yeah, we’ve been running it since yesterday, everything is good so far.

It’s just that this fix is an obvious improvement in that it definitely does not make things worse, and, as I said, I think the issue is quite severe (a crash), so I don’t see reasons not to merge it right away.

[rustyrussell]

Thanks everyone! This was my thinko in the first place, of course!