Use dedicated type for error codes

[vasild]

Before this patch we used int for error codes. The problem with
int is that we try to pass it to/from wire and the size of int is
not defined by the standard. So a sender with 4-byte int would write
4 bytes to the wire and a receiver with 2-byte int (for example) would
read just 2 bytes from the wire.

To resolve this:

• Introduce an error code type with a known size:
  typedef int32_t errcode_t.

• Change all error code macros to constants of type errcode_t.
  Constants also play better with gdb - it would visualize the name of
  the constant instead of the numeric value.

• Change all functions that take error codes to take the new type
  errcode_t instead of int.

• Introduce towire / fromwire functions to send / receive the newly added
  type errcode_t and use it instead of towire_int().

In addition:

• Remove the now unneeded towire_int().

• Replace a hardcoded error code -2 with a new constant
  INVOICE_EXPIRED_DURING_WAIT (903).

Changelog-Changed: The waitinvoice command would now return error code 903 to designate that the invoice expired during wait, instead of the previous -2

[ZmnSCPxj]

Style checker is complaining (make check-includes):

Include(s) from common/json.h duplicated in common/json.c:
#include <ccan/short_types/short_types.h>
#include <common/errcode.h>

Include(s) from lightningd/jsonrpc.h duplicated in lightningd/jsonrpc.c:
#include <common/errcode.h>

Include(s) from lightningd/notification.h duplicated in lightningd/notification.c:
#include <common/errcode.h>

Include(s) from lightningd/pay.h duplicated in lightningd/pay.c:
#include <common/errcode.h>

Include(s) from plugins/libplugin.h duplicated in plugins/libplugin.c:
#include <common/errcode.h>

[rustyrussell]

Hmm, this is a readability improvement (though I generally stick with #define for constants), but note that the compiler won't see a difference between errcode_t and int, because typedefs are completely fungible.

I couldn't understand how we ever got to/fromwire_int until I traced the git history and saw you introduced it yourself in an earlier patch! :)

Ack, pending check fixes.

[ZmnSCPxj]

but note that the compiler won't see a difference between errcode_t and int, because typedefs are completely fungible.

inb4 typedef struct { s32 value; } errcode_t;

[ZmnSCPxj]

zmnscpxj@zmnscpxj-server-2020:~/dvpt/tmp$ cat tmp.c
typedef struct { int value; } errcode_t;

static const errcode_t FOO = {42};
static const errcode_t BAR = {0};

int main() {
        return FOO;
}
zmnscpxj@zmnscpxj-server-2020:~/dvpt/tmp$ gcc -O1 -g tmp.c
tmp.c: In function ‘main’:
tmp.c:7:9: error: incompatible types when returning type ‘errcode_t’ {aka ‘const struct <anonymous>’} but ‘int’ was expected
    7 |  return FOO;
      |         ^~~
zmnscpxj@zmnscpxj-server-2020:~/dvpt/tmp$ ed tmp.c
140
/return FOO/
        return FOO;
s/FOO/FOO.value/
wq
146
zmnscpxj@zmnscpxj-server-2020:~/dvpt/tmp$ cat tmp.c
typedef struct { int value; } errcode_t;

static const errcode_t FOO = {42};
static const errcode_t BAR = {0};

int main() {
        return FOO.value;
}
zmnscpxj@zmnscpxj-server-2020:~/dvpt/tmp$ gcc -O1 -g tmp.c
zmnscpxj@zmnscpxj-server-2020:~/dvpt/tmp$ gdb ./a.out 
GNU gdb (Ubuntu 8.3-0ubuntu1) 8.3
Copyright (C) 2019 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
    <http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from ./a.out...
(gdb) p FOO
$1 = {value = 42}
(gdb) p &FOO
Can't take address of "FOO" which isn't an lvalue.
(gdb) p BAR
$2 = {value = 0}
(gdb) p &BAR
Can't take address of "BAR" which isn't an lvalue.
(gdb) quit

In some modern ABIs as well, a struct than can fit in a machine word can be passed as-is inside a register, just like an int would, if passed by value rather than via pointer. Though not all modern ABIs allow this.

[vasild]

Hmm, this is a readability improvement (though I generally stick with #define for constants), but note that the compiler won't see a difference between errcode_t and int, because typedefs are completely fungible.

The main reason I used constant variables over macros is to ensure that they are of type errcode_t and not the default int. Now I realize this can also be achieved by #define FOO (errcode_t)123 (yuck!). Plus some minor benefits for gdb.

I couldn't understand how we ever got to/fromwire_int until I traced the git history and saw you introduced it yourself in an earlier patch! :)

I couldn't understand either how did I end up authoring towire_int() which is contradictory by itself - send_in_a_portable_way__a_type_that_cannot_be_sent_in_portable_way(). I guess everybody got fooled by the idea initially. Then I realized it has some problems, but it was the way with least resistance and was ok, given the current PR was coming.

Btw there is also towire_double() which should be ditched.

Ack, pending check fixes.

Thanks!

[ZmnSCPxj]

In commit message:

Changelog-Changed: The invoice command would now return error code 903 to designate that the invoice expired during wait, instead of the previous -2

It is the waitinvoice command which errors if an invoice expired during wait, not invoice.

[ZmnSCPxj]

Probably also need to update: https://github.com/ElementsProject/lightning/blame/602b81fef2f47834aa815d723d0f210af80062ce/doc/lightning-waitinvoice.7.md#L24-L26

[vasild]

Done in e3879d9.

[vasild]

It is the waitinvoice command...

Fixed in e3879d9.

[ZmnSCPxj]

ACK e3879d9