From d17baf7dbc31bf913765efe9a25bc2e1dbf34140 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 13 Aug 2024 10:06:40 -0600 Subject: [PATCH] Document & test the conversion from `getrawtransaction` response to `RawTransaction` This also modifies the internal `common.ZcashdRpcReplyGetrawtransaction` type to ensure that the reinterpretation of the `-1` height value from a `getrawtransaction` response is not platform-dependent. --- common/common.go | 2 +- frontend/service.go | 52 +++++++++++++++++++++++++++++----------- frontend/service_test.go | 30 +++++++++++++++++++++++ walletrpc/service.proto | 29 ++++++++++++++++++---- 4 files changed, 93 insertions(+), 20 deletions(-) diff --git a/common/common.go b/common/common.go index 50f38b2b..3c8a5416 100644 --- a/common/common.go +++ b/common/common.go @@ -127,7 +127,7 @@ type ( // many more fields but these are the only ones we current need. ZcashdRpcReplyGetrawtransaction struct { Hex string - Height int + Height int64 } // zcashd rpc "getaddressbalance" diff --git a/frontend/service.go b/frontend/service.go index 30df3ed8..8e20bf01 100644 --- a/frontend/service.go +++ b/frontend/service.go @@ -307,6 +307,43 @@ func (s *lwdStreamer) GetLatestTreeState(ctx context.Context, in *walletrpc.Empt return s.GetTreeState(ctx, &walletrpc.BlockID{Height: uint64(latestHeight)}) } +// ParseRawTransaction converts between the JSON result of a `zcashd` +// `getrawtransaction` call and the `RawTransaction` protobuf type. +// +// Due to an error in the original protobuf definition, it is necessary to +// reinterpret the result of the `getrawtransaction` RPC call. Zcashd will +// return the int64 value `-1` for the height of transactions that appear in +// the block index, but which are not mined in the main chain. `service.proto` +// defines the height field of `RawTransaction` to be a `uint64`, and as such +// we must map the response from the zcashd RPC API to be representable within +// this space. Additionally, the `height` field will be absent for transactions +// in the mempool, resulting in the default value of `0` being set. Therefore, +// the meanings of the `Height` field of the `RawTransaction` type are as +// follows: +// +// * height 0: the transaction is in the mempool +// * height 0xffffffffffffffff: the transaction has been mined on a fork that +// is not currently the main chain +// * any other height: the transaction has been mined in the main chain at the +// given height +func ParseRawTransaction(message json.RawMessage) (*walletrpc.RawTransaction, error) { + // Many other fields are returned, but we need only these two. + var txinfo common.ZcashdRpcReplyGetrawtransaction + err := json.Unmarshal(message, &txinfo) + if err != nil { + return nil, err + } + txBytes, err := hex.DecodeString(txinfo.Hex) + if err != nil { + return nil, err + } + + return &walletrpc.RawTransaction{ + Data: txBytes, + Height: uint64(txinfo.Height), + }, nil +} + // GetTransaction returns the raw transaction bytes that are returned // by the zcashd 'getrawtransaction' RPC. func (s *lwdStreamer) GetTransaction(ctx context.Context, txf *walletrpc.TxFilter) (*walletrpc.RawTransaction, error) { @@ -328,20 +365,7 @@ func (s *lwdStreamer) GetTransaction(ctx context.Context, txf *walletrpc.TxFilte if rpcErr != nil { return nil, rpcErr } - // Many other fields are returned, but we need only these two. - var txinfo common.ZcashdRpcReplyGetrawtransaction - err = json.Unmarshal(result, &txinfo) - if err != nil { - return nil, err - } - txBytes, err := hex.DecodeString(txinfo.Hex) - if err != nil { - return nil, err - } - return &walletrpc.RawTransaction{ - Data: txBytes, - Height: uint64(txinfo.Height), - }, nil + return ParseRawTransaction(result) } if txf.Block != nil && txf.Block.Hash != nil { diff --git a/frontend/service_test.go b/frontend/service_test.go index 52661ebd..552b8acb 100644 --- a/frontend/service_test.go +++ b/frontend/service_test.go @@ -3,6 +3,7 @@ package frontend import ( "encoding/json" "testing" + "math" "github.com/zcash/lightwalletd/common" ) @@ -36,3 +37,32 @@ func TestZcashdRpcReplyUnmarshalling(t *testing.T) { } } +func TestParseRawTransaction(t *testing.T) { + rt0, err0 := ParseRawTransaction([]byte("{\"hex\": \"deadbeef\", \"height\": 123456}")) + if err0 != nil { + t.Fatal("Failed to parse raw transaction response with known height.") + } + if rt0.Height != 123456 { + t.Errorf("Unmarshalled incorrect height: got: %d, expected: 123456.", rt0.Height) + } + + rt1, err1 := ParseRawTransaction([]byte("{\"hex\": \"deadbeef\", \"height\": -1}")) + if err1 != nil { + t.Fatal("Failed to parse raw transaction response for a known tx not in the main chain.") + } + // We expect the int64 value `-1` to have been reinterpreted as a uint64 value in order + // to be representable as a uint64 in `RawTransaction`. The conversion from the twos-complement + // signed representation should map `-1` to `math.MaxUint64`. + if rt1.Height != math.MaxUint64 { + t.Errorf("Unmarshalled incorrect height: got: %d, want: 0x%X.", rt1.Height, uint64(math.MaxUint64)) + } + + rt2, err2 := ParseRawTransaction([]byte("{\"hex\": \"deadbeef\"}")) + if err2 != nil { + t.Fatal("Failed to parse raw transaction response for a tx in the mempool.") + } + if rt2.Height != 0 { + t.Errorf("Unmarshalled incorrect height: got: %d, expected: 0.", rt2.Height) + } +} + diff --git a/walletrpc/service.proto b/walletrpc/service.proto index a94e639f..2f7c9caf 100644 --- a/walletrpc/service.proto +++ b/walletrpc/service.proto @@ -31,12 +31,31 @@ message TxFilter { bytes hash = 3; // transaction ID (hash, txid) } -// RawTransaction contains the complete transaction data. It also optionally includes -// the block height in which the transaction was included, or, when returned -// by GetMempoolStream(), the latest block height. +// RawTransaction contains the complete transaction data. It also includes the +// height for the block in which the transaction was included in the main +// chain, if any (as detailed below). message RawTransaction { - bytes data = 1; // exact data returned by Zcash 'getrawtransaction' - uint64 height = 2; // height that the transaction was mined (or -1) + // The serialized representation of the Zcash transaction. + bytes data = 1; + // The height at which the transaction is mined, or a sentinel value. + // + // Due to an error in the original protobuf definition, it is necessary to + // reinterpret the result of the `getrawtransaction` RPC call. Zcashd will + // return the int64 value `-1` for the height of transactions that appear + // in the block index, but which are not mined in the main chain. Here, the + // height field of `RawTransaction` was erroneously created as a `uint64`, + // and as such we must map the response from the zcashd RPC API to be + // representable within this space. Additionally, the `height` field will + // be absent for transactions in the mempool, resulting in the default + // value of `0` being set. Therefore, the meanings of the `height` field of + // the `RawTransaction` type are as follows: + // + // * height 0: the transaction is in the mempool + // * height 0xffffffffffffffff: the transaction has been mined on a fork that + // is not currently the main chain + // * any other height: the transaction has been mined in the main chain at the + // given height + uint64 height = 2; } // A SendResponse encodes an error code and a string. It is currently used