int64 -> uint64

[tim2CF]

Background

In many places of LND gRPC protobuf documents is used int64 type instead of uint64 or uint32, even if value will never be negative, and negative numbers don't make any sense in context. For example (just some of many)

dates

lnd/lnrpc/rpc.proto

Lines 2838 to 2842 in 3190437

	/// When this invoice was created
	int64 creation_date = 7;
	/// When this invoice was settled
	int64 settle_date = 8;

and money amounts

lnd/lnrpc/rpc.proto

Lines 2896 to 2904 in 3190437

	/**
	The amount that was accepted for this invoice, in satoshis. This will ONLY
	be set if this invoice has been settled. We provide this field as if the
	invoice was created with a zero value, then we need to record what amount
	was ultimately accepted. Additionally, it's possible that the sender paid
	MORE that was specified in the original invoice. So we'll record that here
	as well.
	*/
	int64 amt_paid_sat = 19;

lnd/lnrpc/rpc.proto

Line 2826 in 3190437

int64 value = 5;

I'm not expert in BOLT protocol, and I don't know many details. Maybe these values actually can be negative, I don't know. But if they can't - transition to uint64 or uint32 might be good improvement, and will reduce complexity everywhere including third-party applications which are using gRPC to communicate with LND.

Your environment

• grpc-1.23.0
• lnd version 0.10.0-beta
• Linux 4b3ce405f0cb 4.19.76-linuxkit Fix name typo in README #1 SMP Fri Apr 3 15:53:26 UTC 2020 x86_64 GNU/Linux
• Bitcoin Core Daemon version v0.18.1.0-gfa27a0760792b251585f2a70eccdd547f915b7e4

[brunoerg]

I agree.

For example:
// The number of confirmations for the Utxo
int64 confirmations = 6;

I do not think the number of confirmations for a UTXO can be a negative number.

I think the values that can be negatives are specified in the comments, like:
/* The number of blocks remaining until the current stage can be swept. Negative values indicate how many blocks have passed since becoming mature. */
int32 blocks_til_maturity = 5;

[cfromknecht]

@tim2CF definitely agree. this can be done pretty readily at the grpc-level, since it allows you to convert between the standard integral values (int32, uint32, int64, and uint64) without breaking compatibility at the wire level. the only downside to doing so would be that any client libraries will need to update the way they handle the differing native types compiled by grpc, but maybe that is an acceptable tradeoff.

fwiw, i personally would like to see more standardization wrt the types exposed on the rpc interface, so we should definitely investigate this. since it is a pseudo-breaking change, we would want to broadcast these changes with a little more lead time, but maybe something we can consider for 0.12?

[bartoli]

FYI, i think i will try to work on this. Probably starting with dates.

It should be OK to convert protobuf int32 to uint32 when we know they should always be positive. BUT, then i think protobuf could wrongly decode a negative value from an old client, as both types should have the same encoding, but it will not know which one was used.
Maybe, for a while (until clients are upgraded? depending on some thing indicating the version of client proto?), changing this should also imply adding checks in RPCs that the value is not bigger than the max signed int32?
Maybe a first step should even add checks that negative values are really not transmited currently?

[guggero]

Maybe a first step should even add checks that negative values are really not transmited currently?

I don't think that will give you much. Let's assume you change int64 confirmations = X to uint64 confirmations = X on the server level. But the client doesn't update and still thinks it's int64, then sends a -1.
That will on the server side now be interpreted as a very large value, since it's being interpreted differently.

Even though I agree this isn't super nice on the gRPC level, I'm not sure if looking into this is really worth the hassle. Especially since you cannot control what the client software version is using (thinking of all the wallet apps and other tools that use gRPC to connect to lnd). So I think we'd want some comprehensive tests to show that nothing breaks when someone uses a gRPC client (compiled for the different possible programming languages like Python, Java, Swift, JavaScript and so on) compiled against an old *.proto file against an updated lnd...

[bartoli]

Even though I agree this isn't super nice on the gRPC level, I'm not sure if looking into this is really worth the hassle.

Are you talking about that point only, or the issue itself?

[guggero]

I mean the general issue of not having the most optimal data type in our *.proto files. It's a bit annoying and ugly at times. But not breaking backward compatibility is just way more important. You'll probably find many inconsistencies and things that could be nicer, but we can't just change them without breaking older clients.

[bartoli]

I do agree it looks to be a good amount of work for little rewards.

And it might indeed break some clients. But i am actually more interested in making sure improper values could not abuse the server.
I might at least do a review of server side implementation of the RPCs to check there are no obvious security issues when improper values are passed