[#83] add: gRPC parity test suite (Zainod vs. Lightwalletd)#84
[#83] add: gRPC parity test suite (Zainod vs. Lightwalletd)#84pacu wants to merge 4 commits intozcash:mainfrom
Conversation
git-subtree-dir: lightwallet-protocol git-subtree-split: 23f0768ea4471b63285f3c0e9b6fbb361674aa2b
Closes zcash#83 Runs Zainod and Lightwalletd side-by-side against the same Zebrad node and compares their CompactTxStreamer gRPC responses. Covers 21 test cases across 15 RPC methods and integrates with the existing BitcoinTestFramework and CI pipeline. - `lightwallet-protocol/` — canonical proto source via `git subtree` from `zcash/lightwallet-protocol` v0.4.0 - `qa/rpc-tests/test_framework/proto/` — generated Python gRPC stubs committed so CI needs only `grpcio` at runtime - `scripts/generate_proto.sh` — regenerates stubs after a protocol version bump and fixes flat imports to relative - `util.py` / `test_framework.py` — Lightwalletd process lifecycle (`lwd_grpc_port`, `write_lwd_conf`, `start_lightwalletd`, `wait_for_lwd_start`, teardown) - `qa/rpc-tests/grpc_comparison.py` — test file - `qa/zcash/grpc_comparison_tests.py` — convenience runner - CI: `lightwalletd-interop-request` dispatch trigger, `build-lightwalletd` job, artifact download in `test-rpc` - Docs: README and book updated with prerequisites and run instructions 1. **`vtx` in compact blocks** — For blocks containing only transparent transactions, Zainod returns empty `vtx`; Lightwalletd includes them. Block comparison currently covers header fields only. 2. **gRPC error codes on out-of-bounds requests** — Zainod returns `OUT_OF_RANGE`; Lightwalletd returns `INVALID_ARGUMENT`. Tests assert only that both sides raise an error. - `GetMempoolTx` / `GetMempoolStream` (need wallet integration) - Shielded transaction coverage (need Zallet or chain cache) - Completed subtree roots (need 2^16 outputs per tree) - `SendTransaction`, darkside mode Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a gRPC parity test suite that runs Zainod and Lightwalletd side-by-side against the same Zebrad node and compares CompactTxStreamer responses, plus vendored protocol sources and generated Python stubs to run in CI.
Changes:
- Added
grpc_comparison.pyRPC test and a convenience runner to execute it via the existing rpc-tests harness. - Added Lightwalletd lifecycle support to the Python test framework (ports, config generation, start/wait/stop/teardown).
- Vendored
lightwallet-protocolproto sources and checked in generated Python gRPC/protobuf stubs, plus a script to regenerate them.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/generate_proto.sh | Regenerates Python protobuf/gRPC stubs from the vendored proto subtree (with import-rewrite post-processing). |
| qa/zcash/grpc_comparison_tests.py | Convenience runner that invokes the rpc-tests harness for the new gRPC parity test. |
| qa/rpc-tests/test_framework/util.py | Adds Lightwalletd binary resolution, gRPC port allocation, config writing, and process start/wait/teardown helpers. |
| qa/rpc-tests/test_framework/test_framework.py | Wires Lightwalletd startup/teardown into the core BitcoinTestFramework lifecycle. |
| qa/rpc-tests/test_framework/proto/__init__.py | Declares the generated stubs as a package. |
| qa/rpc-tests/test_framework/proto/service_pb2.pyi | Generated typing stubs for service.proto. |
| qa/rpc-tests/test_framework/proto/service_pb2.py | Generated protobuf runtime code for service.proto. |
| qa/rpc-tests/test_framework/proto/service_pb2_grpc.py | Generated gRPC client/server code for CompactTxStreamer. |
| qa/rpc-tests/test_framework/proto/compact_formats_pb2.pyi | Generated typing stubs for compact_formats.proto. |
| qa/rpc-tests/test_framework/proto/compact_formats_pb2.py | Generated protobuf runtime code for compact_formats.proto. |
| qa/rpc-tests/test_framework/proto/compact_formats_pb2_grpc.py | Generated gRPC glue for compact_formats.proto. |
| qa/rpc-tests/grpc_comparison.py | New parity test suite covering multiple CompactTxStreamer methods and error paths. |
| qa/pull-tester/rpc-tests.py | Registers grpc_comparison.py in the rpc-tests runner. |
| lightwallet-protocol/walletrpc/service.proto | Vendored canonical proto definitions (subtree). |
| lightwallet-protocol/walletrpc/compact_formats.proto | Vendored canonical proto definitions (subtree). |
| lightwallet-protocol/LICENSE | Vendored license for the protocol subtree. |
| lightwallet-protocol/CHANGELOG.md | Vendored protocol changelog (includes v0.4.0 notes). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def setup_network(self, split=False): | ||
| self.wallets = [] # no wallets used; required for teardown | ||
| self.nodes = self.setup_nodes() | ||
| node = self.nodes[0] | ||
|
|
||
| # Mine 30 blocks so coinbase is mature and we have a chain for range queries. | ||
| node.generate(30) | ||
|
|
||
| # The default zebrad config mines coinbase to this regtest t-address. | ||
| # All mined coinbase UTXOs are at this address, giving us real t-addr | ||
| # data for GetTaddressTxids / GetTaddressBalance / GetAddressUtxos. | ||
| self.taddr = "tmSRd1r8gs77Ja67Fw1JcdoXytxsyrLTPJm" | ||
|
|
||
| # Use the coinbase txid from block 1 for GetTransaction tests. | ||
| self.txid = node.getblock("1")['tx'][0] | ||
|
|
||
| self.zainos = self.setup_indexers() | ||
| self.lwds = self.setup_lightwalletds() | ||
|
|
There was a problem hiding this comment.
setup_network() bypasses BitcoinTestFramework.prepare_chain(), but prepare_chain() explicitly mines up to 100 blocks when num_indexers > 0 (see qa/rpc-tests/test_framework/test_framework.py:81-85). With cache_behavior='clean' and only node.generate(30), Zainod may fail to start or behave unexpectedly on a too-short chain.
Suggestion: either call self.prepare_chain() before setup_indexers() / setup_lightwalletds(), or mine at least 100 blocks here (then mine any additional blocks you want for range-query coverage).
| import grpc | ||
|
|
||
| from test_framework.test_framework import BitcoinTestFramework | ||
| from test_framework.util import ( | ||
| assert_equal, | ||
| zaino_grpc_port, | ||
| lwd_grpc_port, | ||
| ) | ||
| from test_framework.proto import ( | ||
| compact_formats_pb2, | ||
| service_pb2, | ||
| service_pb2_grpc, | ||
| ) |
There was a problem hiding this comment.
This test (and the lightwalletd start-up wait loop in test_framework/util.py) imports grpc and protobuf-generated modules, but the repo's pyproject.toml/uv.lock currently don't include grpcio / protobuf dependencies. In addition, the checked-in generated stubs include hard version guards (e.g. service_pb2_grpc.py requires grpcio>=1.80.0 and service_pb2.py validates protobuf runtime 6.31.1).
To avoid CI/runtime failures, ensure the runtime dependency set pins/constraints grpcio and protobuf to versions compatible with the generated code (or regenerate stubs using the versions you intend to support).
| import compact_formats_pb2 as _compact_formats_pb2 | ||
| from google.protobuf.internal import containers as _containers | ||
| from google.protobuf.internal import enum_type_wrapper as _enum_type_wrapper | ||
| from google.protobuf import descriptor as _descriptor | ||
| from google.protobuf import message as _message | ||
| from collections.abc import Iterable as _Iterable, Mapping as _Mapping | ||
| from typing import ClassVar as _ClassVar, Optional as _Optional, Union as _Union |
There was a problem hiding this comment.
The generated type stub uses a flat import (import compact_formats_pb2 ...) which won't resolve when test_framework.proto is used as a package (it should be a relative import like from . import compact_formats_pb2 ..., matching service_pb2.py). This can break type checking and tooling for consumers of these stubs.
Suggestion: adjust the generation post-processing to also rewrite imports in service_pb2.pyi (and any other .pyi files that get flat imports).
scripts/generate_proto.sh
Outdated
| # Fix them to use relative imports. | ||
| sed -i 's/^import compact_formats_pb2 as/from . import compact_formats_pb2 as/' \ | ||
| "$PROTO_OUT/service_pb2.py" \ | ||
| "$PROTO_OUT/service_pb2_grpc.py" | ||
| sed -i 's/^import service_pb2 as/from . import service_pb2 as/' \ | ||
| "$PROTO_OUT/service_pb2_grpc.py" |
There was a problem hiding this comment.
The post-processing only rewrites imports in service_pb2.py / service_pb2_grpc.py, but --pyi_out also produces service_pb2.pyi that currently contains flat imports (see qa/rpc-tests/test_framework/proto/service_pb2.pyi:1).
Suggestion: extend the sed rewrite to include the generated .pyi files too (and consider making the rewrite logic robust to additional proto modules as the protocol grows).
| # Fix them to use relative imports. | |
| sed -i 's/^import compact_formats_pb2 as/from . import compact_formats_pb2 as/' \ | |
| "$PROTO_OUT/service_pb2.py" \ | |
| "$PROTO_OUT/service_pb2_grpc.py" | |
| sed -i 's/^import service_pb2 as/from . import service_pb2 as/' \ | |
| "$PROTO_OUT/service_pb2_grpc.py" | |
| # Fix them to use relative imports in all generated Python artifacts. | |
| for generated_file in \ | |
| "$PROTO_OUT/service_pb2.py" \ | |
| "$PROTO_OUT/service_pb2.pyi" \ | |
| "$PROTO_OUT/service_pb2_grpc.py" | |
| do | |
| for module_name in compact_formats_pb2 service_pb2 | |
| do | |
| sed -i "s/^import ${module_name} as/from . import ${module_name} as/" \ | |
| "$generated_file" | |
| done | |
| done |
| l_txs = _collect_stream(ls.GetTaddressTxids(req)) | ||
| assert_equal(len(z_txs), len(l_txs)) | ||
| for z_tx, l_tx in zip(z_txs, l_txs): | ||
| assert_equal(z_tx.data, l_tx.data) |
There was a problem hiding this comment.
test_get_taddress_txids_lower() only compares data and not height (unlike test_get_taddress_txids()), so a height mismatch could slip through while still passing.
Suggestion: also assert z_tx.height == l_tx.height for each streamed result here.
| assert_equal(z_tx.data, l_tx.data) | |
| assert_equal(z_tx.data, l_tx.data) | |
| assert_equal(z_tx.height, l_tx.height) |
| ) | ||
| z_txs = _collect_stream(zs.GetTaddressTxids(req)) | ||
| l_txs = _collect_stream(ls.GetTaddressTxids(req)) | ||
| assert_equal(len(z_txs), len(l_txs)) |
There was a problem hiding this comment.
test_get_taddress_txids_upper() currently only asserts that the two streams have the same length, but doesn't compare any per-transaction fields. That can miss real divergences where both sides return the same number of transactions but with different contents/order.
Suggestion: compare at least data and height for each element (and consider sorting deterministically if ordering isn't guaranteed).
| assert_equal(len(z_txs), len(l_txs)) | |
| assert_equal(len(z_txs), len(l_txs)) | |
| for z_tx, l_tx in zip(z_txs, l_txs): | |
| assert_equal(z_tx.data, l_tx.data) | |
| assert_equal(z_tx.height, l_tx.height) |
| assert_equal(z_u.index, l_u.index) | ||
| assert_equal(z_u.valueZat, l_u.valueZat) |
There was a problem hiding this comment.
test_get_address_utxos_stream() asserts a subset of fields (address, txid, index, valueZat) but omits script and height, even though test_get_address_utxos() verifies them. If the goal is parity across implementations, this can miss divergences in streamed UTXO details.
Suggestion: also assert script and height equality for each streamed element.
| assert_equal(z_u.index, l_u.index) | |
| assert_equal(z_u.valueZat, l_u.valueZat) | |
| assert_equal(z_u.index, l_u.index) | |
| assert_equal(z_u.script, l_u.script) | |
| assert_equal(z_u.valueZat, l_u.valueZat) | |
| assert_equal(z_u.height, l_u.height) |
qa/rpc-tests/grpc_comparison.py
Outdated
| zs = service_pb2_grpc.CompactTxStreamerStub(zainod_ch) | ||
| ls = service_pb2_grpc.CompactTxStreamerStub(lwd_ch) | ||
|
|
||
| deadline = time.time() + timeout | ||
| while time.time() < deadline: | ||
| try: | ||
| z_info = zs.GetLightdInfo(service_pb2.Empty(), timeout=5) | ||
| l_info = ls.GetLightdInfo(service_pb2.Empty(), timeout=5) | ||
| if (z_info.blockHeight >= expected_height and | ||
| l_info.blockHeight >= expected_height): | ||
| return | ||
| except grpc.RpcError: | ||
| pass | ||
| time.sleep(1) | ||
|
|
||
| raise Exception( | ||
| f"Indexers did not sync to height {expected_height} within {timeout}s" | ||
| ) |
There was a problem hiding this comment.
_wait_for_indexers() creates gRPC channels but never closes them. While the process will exit, leaving channels open can keep sockets/threads around longer than necessary and makes failures noisier.
Suggestion: wrap channel creation in a try/finally and call close() on both channels (or use context managers if supported).
| zs = service_pb2_grpc.CompactTxStreamerStub(zainod_ch) | |
| ls = service_pb2_grpc.CompactTxStreamerStub(lwd_ch) | |
| deadline = time.time() + timeout | |
| while time.time() < deadline: | |
| try: | |
| z_info = zs.GetLightdInfo(service_pb2.Empty(), timeout=5) | |
| l_info = ls.GetLightdInfo(service_pb2.Empty(), timeout=5) | |
| if (z_info.blockHeight >= expected_height and | |
| l_info.blockHeight >= expected_height): | |
| return | |
| except grpc.RpcError: | |
| pass | |
| time.sleep(1) | |
| raise Exception( | |
| f"Indexers did not sync to height {expected_height} within {timeout}s" | |
| ) | |
| try: | |
| zs = service_pb2_grpc.CompactTxStreamerStub(zainod_ch) | |
| ls = service_pb2_grpc.CompactTxStreamerStub(lwd_ch) | |
| deadline = time.time() + timeout | |
| while time.time() < deadline: | |
| try: | |
| z_info = zs.GetLightdInfo(service_pb2.Empty(), timeout=5) | |
| l_info = ls.GetLightdInfo(service_pb2.Empty(), timeout=5) | |
| if (z_info.blockHeight >= expected_height and | |
| l_info.blockHeight >= expected_height): | |
| return | |
| except grpc.RpcError: | |
| pass | |
| time.sleep(1) | |
| raise Exception( | |
| f"Indexers did not sync to height {expected_height} within {timeout}s" | |
| ) | |
| finally: | |
| zainod_ch.close() | |
| lwd_ch.close() |
- Fix flat import in service_pb2.pyi (relative import was missing, unlike the .py counterpart); extend generate_proto.sh to rewrite imports in .pyi files as well - Add missing height assertion in test_get_taddress_txids_lower - Add per-element data+height assertions in test_get_taddress_txids_upper (previously only checked stream length) - Add missing script and height assertions in test_get_address_utxos_stream to match the coverage in test_get_address_utxos - Fix gRPC channel leak in _wait_for_indexers: wrap channel lifecycle in try/finally and close both channels on exit - Pad chain to 100 blocks before starting indexers: Zainod requires a minimum of 100 blocks; the three mining phases only produce 36 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Addressed all Copilot review findings:
|
Summary
Adds a test suite that runs Zainod and Lightwalletd side-by-side against
the same Zebrad node and compares their
CompactTxStreamergRPC responses.Covers 21 test cases across 15 RPC methods. Integrates with the existing
BitcoinTestFrameworkand CI pipeline, and can be triggered from theLightwalletd repo via
repository_dispatch.Closes #83
Changes
New files
lightwallet-protocol/— canonical proto source viagit subtreefromzcash/lightwallet-protocolv0.4.0qa/rpc-tests/test_framework/proto/— generated Python gRPC stubs,committed so CI only needs
grpcioat runtime (notgrpcio-tools)scripts/generate_proto.sh— developer script to regenerate stubs aftera protocol version bump; fixes flat imports to relative automatically
qa/rpc-tests/grpc_comparison.py— test fileqa/zcash/grpc_comparison_tests.py— convenience runner(
uv run ./qa/zcash/grpc_comparison_tests.py)Modified files
util.py/test_framework.py— Lightwalletd process lifecycle:lwd_grpc_port,write_lwd_conf,start_lightwalletd,wait_for_lwd_start, teardownpyproject.toml— addedgrpcioandprotobufruntime dependenciesqa/pull-tester/rpc-tests.py— registeredgrpc_comparison.pyinNEW_SCRIPTS.github/workflows/ci.yml— addedlightwalletd-interop-requestdispatch trigger,
build-lightwalletdjob, artifact download andLIGHTWALLETDenv var intest-rpcREADME.mdanddoc/book/— prerequisites, run instructions, andwriting guide updated
RPC methods tested
GetLightdInfoGetLatestBlockGetBlockGetBlockNullifiersGetBlockRangeGetBlockRangeNullifiersGetTransactionGetTaddressTxidsGetTaddressBalanceGetTaddressBalanceStreamGetTreeStateGetLatestTreeStateGetSubtreeRootsGetAddressUtxosGetAddressUtxosStreamKnown divergences
Two behavioral differences between Zainod and Lightwalletd surfaced during
implementation. Both are worked around in the current tests but should be
investigated separately.
1.
vtxin compact blocksFor blocks containing only transparent transactions (in our test chain:
all blocks are coinbase-only), Zainod returns an empty
vtxwhileLightwalletd includes those transactions. Whether this is a bug, a protocol
interpretation difference, or expected behavior is unclear.
GetBlockandGetBlockRangetests currently compare header fields only (height, hash,prevHash, time, chainMetadata).
2. gRPC error codes on out-of-bounds requests
Zainod returns
OUT_OF_RANGE; Lightwalletd returnsINVALID_ARGUMENTforthe same out-of-bounds inputs. Out-of-bounds tests assert only that both
sides raise a gRPC error, not that the codes match.
Out of scope
GetMempoolTx/GetMempoolStream— require wallet integration tosubmit mempool transactions (TODO in test file)
cache with pre-existing shielded activity
GetSubtreeRootswith completed subtrees — each requires 2^16 outputs;not feasible in a clean regtest chain (both sides return empty stream;
agreement is asserted)
SendTransaction, darkside modeTest plan
./src/or via env vars (ZEBRAD,ZAINOD,LIGHTWALLETD)uv syncto pick upgrpcioandprotobufdependenciesuv run ./qa/zcash/grpc_comparison_tests.pypassesuv run ./qa/zcash/grpc_comparison_tests.py --nocleanuppasses and<tmpdir>/lwd0/lwd.logcontains no fatal errorsuv run ./qa/zcash/full_test_suite.pystill passesscripts/generate_proto.shproduces identical output to thecommitted stubs
grpc_comparison.pyin the
test-rpcshard output