From 8866a0be4c557f3c651ff0846d4aa6ff51141ab6 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 12 Aug 2024 19:12:22 -0600 Subject: [PATCH 1/4] Add a test that demonstrates unmarshalling behavior for `getrawtransaction` results. --- common/common_test.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/common/common_test.go b/common/common_test.go index 9fc19f32..a680dd87 100644 --- a/common/common_test.go +++ b/common/common_test.go @@ -710,3 +710,33 @@ func TestMempoolStream(t *testing.T) { sleepCount = 0 sleepDuration = 0 } + +func TestZcashdRpcReplyUnmarshalling(t *testing.T) { + var txinfo0 ZcashdRpcReplyGetrawtransaction + err0 := json.Unmarshal([]byte("{\"hex\": \"deadbeef\", \"height\": 123456}"), &txinfo0) + if err0 != nil { + t.Fatal("Failed to unmarshal tx with known height.") + } + if txinfo0.Height != 123456 { + t.Errorf("Unmarshalled incorrect height: got: %d, want: 123456.", txinfo0.Height) + } + + var txinfo1 ZcashdRpcReplyGetrawtransaction + err1 := json.Unmarshal([]byte("{\"hex\": \"deadbeef\", \"height\": -1}"), &txinfo1) + if err1 != nil { + t.Fatal("failed to unmarshal tx not in main chain") + } + if txinfo1.Height != -1 { + t.Errorf("Unmarshalled incorrect height: got: %d, want: -1.", txinfo1.Height) + } + + var txinfo2 ZcashdRpcReplyGetrawtransaction + err2 := json.Unmarshal([]byte("{\"hex\": \"deadbeef\"}"), &txinfo2) + if err2 != nil { + t.Fatal("failed to unmarshal reply lacking height data") + } + if txinfo2.Height != 0 { + t.Errorf("Unmarshalled incorrect height: got: %d, want: 0.", txinfo2.Height) + } +} + From 0f1ed05c4152cc831b69beb172eca1e142e78229 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 13 Aug 2024 10:06:40 -0600 Subject: [PATCH 2/4] 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. --- CHANGELOG.md | 20 ++++++++++++++++++++ common/common.go | 39 ++++++++++++++++++++++++++++++++++++++- common/common_test.go | 29 +++++++++++++++++++++++++++++ frontend/service.go | 27 ++++++--------------------- walletrpc/service.proto | 29 ++++++++++++++++++++++++----- 5 files changed, 117 insertions(+), 27 deletions(-) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 00000000..fd86bc82 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,20 @@ +# Changelog +All notable changes to this library will be documented in this file. + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), +and this library adheres to Rust's notion of +[Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +## [Unreleased] + +### Fixed + +- Parsing of `getrawtransaction` results is now platform-independent. + Previously, values of `-1` returned for the transaction height would + be converted to different `RawTransaction.Height` values depending + upon whether `lightwalletd` was being run on a 32-bit or 64-bit + platform. + +## [Prior Releases] + +This changelog was not created until after the release of v0.4.17 diff --git a/common/common.go b/common/common.go index 50f38b2b..82b0155a 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" @@ -522,6 +522,43 @@ func GetBlockRange(cache *BlockCache, blockOut chan<- *walletrpc.CompactBlock, e errOut <- nil } +// 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 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 +} + func displayHash(hash []byte) string { return hex.EncodeToString(parser.Reverse(hash)) } diff --git a/common/common_test.go b/common/common_test.go index a680dd87..f69d898d 100644 --- a/common/common_test.go +++ b/common/common_test.go @@ -9,6 +9,7 @@ import ( "encoding/json" "errors" "fmt" + "math" "os" "strings" "testing" @@ -740,3 +741,31 @@ 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/frontend/service.go b/frontend/service.go index 30df3ed8..991b1d1e 100644 --- a/frontend/service.go +++ b/frontend/service.go @@ -314,34 +314,19 @@ func (s *lwdStreamer) GetTransaction(ctx context.Context, txf *walletrpc.TxFilte if len(txf.Hash) != 32 { return nil, errors.New("transaction ID has invalid length") } - leHashStringJSON, err := json.Marshal(hex.EncodeToString(parser.Reverse(txf.Hash))) + txidJSON, err := json.Marshal(hex.EncodeToString(parser.Reverse(txf.Hash))) if err != nil { return nil, err } - params := []json.RawMessage{ - leHashStringJSON, - json.RawMessage("1"), - } - result, rpcErr := common.RawRequest("getrawtransaction", params) - // For some reason, the error responses are not JSON + params := []json.RawMessage{txidJSON, json.RawMessage("1")} + result, rpcErr := common.RawRequest("getrawtransaction", params) if rpcErr != nil { + // For some reason, the error responses are not JSON 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 common.ParseRawTransaction(result) } if txf.Block != nil && txf.Block.Hash != nil { 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 From 56fe52a11a1c1e2dad2af62e471a76b9de31d376 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 13 Aug 2024 11:51:14 -0600 Subject: [PATCH 3/4] Uniformly return height 0 for mempool `RawTransaction` results. --- CHANGELOG.md | 8 ++++++++ common/common_test.go | 12 ++++++------ common/mempool.go | 25 ++++++++----------------- 3 files changed, 22 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd86bc82..be064197 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this library adheres to Rust's notion of ## [Unreleased] +### Changed + +- The `RawTransaction` values returned from a call to `GetMempoolStream` + now report a `Height` value of `0`, in order to be consistent with + the results of calls to `GetTransaction`. See the documentation of + `RawTransaction` in `walletrpc/service.proto` for more details on + the semantics of this field. + ### Fixed - Parsing of `getrawtransaction` results is now platform-independent. diff --git a/common/common_test.go b/common/common_test.go index f69d898d..ee67a981 100644 --- a/common/common_test.go +++ b/common/common_test.go @@ -605,7 +605,7 @@ func mempoolStub(method string, params []json.RawMessage) (json.RawMessage, erro if txid != "mempooltxid-1" { testT.Fatal("unexpected txid") } - r, _ := json.Marshal("aabb") + r, _ := json.Marshal(map[string]string{"hex":"aabb"}) return r, nil case 5: // Simulate that still no new block has arrived ... @@ -637,7 +637,7 @@ func mempoolStub(method string, params []json.RawMessage) (json.RawMessage, erro if txid != "mempooltxid-2" { testT.Fatal("unexpected txid") } - r, _ := json.Marshal("ccdd") + r, _ := json.Marshal(map[string]string{"hex":"ccdd"}) return r, nil case 8: // A new block arrives, this will cause these two tx to be returned @@ -669,7 +669,7 @@ func TestMempoolStream(t *testing.T) { return nil }) if err != nil { - t.Fatal("GetMempool failed") + t.Errorf("GetMempool failed: %v", err) } // This should return two transactions. @@ -678,7 +678,7 @@ func TestMempoolStream(t *testing.T) { return nil }) if err != nil { - t.Fatal("GetMempool failed") + t.Errorf("GetMempool failed: %v", err) } if len(replies) != 2 { t.Fatal("unexpected number of tx") @@ -688,13 +688,13 @@ func TestMempoolStream(t *testing.T) { if !bytes.Equal([]byte(replies[0].GetData()), []byte{0xaa, 0xbb}) { t.Fatal("unexpected tx contents") } - if replies[0].GetHeight() != 200 { + if replies[0].GetHeight() != 0 { t.Fatal("unexpected tx height") } if !bytes.Equal([]byte(replies[1].GetData()), []byte{0xcc, 0xdd}) { t.Fatal("unexpected tx contents") } - if replies[1].GetHeight() != 200 { + if replies[1].GetHeight() != 0 { t.Fatal("unexpected tx height") } diff --git a/common/mempool.go b/common/mempool.go index 241e0520..e641b8c6 100644 --- a/common/mempool.go +++ b/common/mempool.go @@ -1,7 +1,6 @@ package common import ( - "encoding/hex" "encoding/json" "sync" "time" @@ -105,35 +104,27 @@ func refreshMempoolTxns() error { // We've already fetched this transaction continue } - g_txidSeen[txid(txidstr)] = struct{}{} + // We haven't fetched this transaction already. + g_txidSeen[txid(txidstr)] = struct{}{} txidJSON, err := json.Marshal(txidstr) if err != nil { return err } - // The "0" is because we only need the raw hex, which is returned as - // just a hex string, and not even a json string (with quotes). - params := []json.RawMessage{txidJSON, json.RawMessage("0")} + + params := []json.RawMessage{txidJSON, json.RawMessage("1")} result, rpcErr := RawRequest("getrawtransaction", params) if rpcErr != nil { // Not an error; mempool transactions can disappear continue } - // strip the quotes - var txStr string - err = json.Unmarshal(result, &txStr) - if err != nil { - return err - } - txBytes, err := hex.DecodeString(txStr) + + rawtx, err := ParseRawTransaction(result) if err != nil { return err } - newRtx := &walletrpc.RawTransaction{ - Data: txBytes, - Height: uint64(g_lastBlockChainInfo.Blocks), - } - g_txList = append(g_txList, newRtx) + + g_txList = append(g_txList, rawtx) } return nil } From 90116a7c55256e597c9a86b31ed942aebaf33699 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 13 Aug 2024 16:20:06 -0600 Subject: [PATCH 4/4] Filter out mined transactions in `refreshMempoolTxns` --- common/mempool.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/common/mempool.go b/common/mempool.go index e641b8c6..e52082c5 100644 --- a/common/mempool.go +++ b/common/mempool.go @@ -124,6 +124,12 @@ func refreshMempoolTxns() error { return err } + // Skip any transaction that has been mined since the list of txids + // was retrieved. + if (rawtx.Height != 0) { + continue; + } + g_txList = append(g_txList, rawtx) } return nil