GetTransaction does not correctly propagate the output of getrawtransaction calls.

[nuttycom]

It turns out that the GetTransaction method added to lightwalletd is horribly broken, because it does not correctly propagate the output of the getrawtransaction JSON-RPC method.

It says here "optionally includes the block height" (which runs into the gRPC problem that this gets aliased with the genesis block height 0 making the transactions look mined), but below says "block height if mined, or -1" (implying to users of the RPC that -1 means "unmined").

Meanwhile, getrawtransaction actually returns:

• A non-negative height for transactions mined in blocks in the current main chain.
• -1 for transactions mined in blocks that are not in the main chain (i.e. conflicted).
• Omitted for unmined transactions in the mempool.

This is further muddied by the fact that the Go parser is parsing the height field as an int, which is machine-dependent (and thus could differ from zcashd), and also I don't know how it handles omitted values (because it isn't to my knowledge a nullable type). So the unmined transactions might be getting their height fields coerced to some default in Go (0?) rather than being set as optional in the returned Protobuf (aka 0) - this needs further investigation.

RawTransaction was then further misused by the addition of the GetMempoolStream RPC. Transactions in the mempool should never be associated with the latest block height; if any height is associated with them, it should be the "mempool height" which is latest_height + 1 (which is the height at which the consensus rules are applied to the mempool transactions). So that RPC is broken as well.

This is going to be Not Fun™️ to fix.

Originally posted by @str4d in zcash/librustzcash#1473 (comment)

[LarryRuane]

So the unmined transactions might be getting their height fields coerced to some default in Go (0?) rather than being set as optional in the returned Protobuf (aka 0) - this needs further investigation

I looked into this using the debugger, and what happens is, the Go library function json.Unmarshal() deserializes the getrawtransaction 1 (1 for verbose) reply into an instance of:

	ZcashdRpcReplyGetrawtransaction struct {
		Hex    string
		Height int
	}

and since the "height" member is missing in the zcashd RPC reply for mempool transactions, it is set to zero. (I'm not sure if json.Unmarshal() is doing that or the Go language, it doesn't matter.) So it has nothing to do with protobuf (which really is getting a zero to serialize as part of the reply).

[nuttycom]

So it has nothing to do with protobuf (which really is getting a zero to serialize as part of the reply).

That's correct, in terms of the deserialization from the zcashd output. With respect to the protobuf part of this, the protobuf height field should really have been defined as int64, not uint64, but it's a bit late to change that now, so instead we are defining the field as an unsigned value that contains the twos-complement int64 representation, and as such the maximum value represents the -1 value returned from zcashd. Internally, clients should perform the conversion to a signed integer and check the sentinel cases before otherwise operating on the value.

Even though protobuf defines optional fields, it is not possible to test whether a value for the field was set or not, you can only test against the default value (which could be a valid value in the domain.) The only reason we're okay here is that we assume that wallets will never be querying for the genesis block's coinbase transaction, for which a height 0 is a valid value in the domain.