frontend: send startHeight and maxEntries to getaddressutxos - #592
Conversation
GetAddressUtxos takes a height range and an entry limit, but both are applied to the backend's reply, not to the request. lightwalletd asks the node for every utxo held by the named addresses, unmarshals all of it, and only then drops what falls outside the range or past the limit. The node's work, and lightwalletd's allocation, are set by the size of the address' utxo set alone; nothing the client asks for reduces either. That is the shape the remaining part of GHSA-x4m7-3gpp-xc36 describes: name one address with a large utxo set and set startHeight at the chain tip, and the request is a few dozen bytes, the reply is empty, and the node still produces the whole set. The 10,000-address cap added in 0.5.0 bounds a different dimension and does nothing here, since one address is enough. There is no return-bandwidth cost to throttle the caller with. Pass both values to the RPC so a backend can apply them where the work happens. The arguments are optional and omitted when unset, so a request that specifies neither is serialized exactly as before, and a backend that doesn't implement them ignores the extra keys -- verified against zebrad, which deserializes the request object with serde and so skips unknown fields. No capability probe or version gate is needed. Keep the reply-side filter permanently rather than making it conditional on backend support. It costs nothing once the backend has already narrowed the result, and it means correctness never depends on the backend honoring the arguments -- only performance does. That is what makes it safe to ship this half first, before any backend implements it. "Which entries" is well defined: service.proto already documents the reply as sorted by height, and zebra returns utxos in chain order (height, tx index, output index) and asserts it. A backend applying maxEntries takes the first N in that order, which is the same subset lightwalletd's filter would have kept. A backend must implement both arguments or neither: honoring maxEntries while ignoring startHeight would truncate before the range filter runs and silently return fewer entries than exist. That belongs in the RPC's specification. The test asserts both halves -- that the arguments reach the backend, and that the reply-side filter still produces the right utxos when the backend ignores them -- and that an unset pair is omitted from the JSON. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds pushdown of startHeight / maxEntries limits for getaddressutxos RPC requests while keeping client-side enforcement for compatibility and safety.
Changes:
- Extend
getaddressutxosrequest JSON to include optionalstartHeightandmaxEntries. - Re-apply server-side limits defensively on the client when backend ignores the new fields.
- Add a unit test verifying both request serialization and client-side filtering fallback.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| frontend/service.go | Sends startHeight/maxEntries to backend and re-enforces those limits on replies. |
| frontend/frontend_test.go | Adds coverage to ensure limits are pushed down and still enforced client-side if ignored. |
| common/common.go | Extends the RPC request struct with optional limit fields (omitempty). |
| CHANGELOG.md | Documents the behavior change and compatibility guarantees. |
Suppressed comments (1)
frontend/service.go:1
MaxEntriesenforcement is off-by-one: withnstarting at 0 and the break condition using>, the loop will process one extra UTXO beyondMaxEntries(e.g.,MaxEntries=2allows 3 entries). Change the comparison to>=(or adjust wherenis incremented) so the function never returns more thanMaxEntries.
// Copyright (c) 2019-2020 The Zcash developers
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
I just want to make it explicit that Copilot's suppressed comment above (off-by-one error) is incorrect, the logic works as it should. |
| // zcashd rpc "getaddressutxos" | ||
| ZcashdRpcRequestGetaddressutxos struct { | ||
| Addresses []string `json:"addresses"` | ||
| // Optional; a backend that doesn't implement these ignores them. |
There was a problem hiding this comment.
@LarryRuane do you think we should implement these at the lightclient-protocol level as well?
There was a problem hiding this comment.
Short answer: they're already there, nothing to add. GetAddressUtxosArg has had startHeight (field 2) and maxEntries (field 3) since GetAddressUtxos was defined. This PR doesn't add a parameter anywhere, it just stops throwing away the ones the client already sends — we forward them to the backend instead of only applying them to the backend's reply. No proto change needed, and service.proto is a symlink into the vendored lightwallet-protocol copy now anyway.
Everything below is a tangent. It's about the protocol, not this PR, and none of it blocks the merge — safe to skip.
Your question did make me look at how a client is meant to page through a large address, and I think the protocol is under-specified there. Resuming is by height only, and height isn't unique.
Say an address receives 5 outputs in one block at height 100, and a client pages with maxEntries=2:
startHeight=0, maxEntries=2 -> 2 entries, both at height 100
startHeight=100, maxEntries=2 -> the same 2 entries
startHeight=100, maxEntries=2 -> the same 2 entries ...
The client can't move to 101 without silently dropping the other three, and staying at 100 re-fetches the same prefix forever. Whenever a single height holds more entries than the page size there's no correct move. Multiple outputs to one address in one block is ordinary — mining payouts, batched sends — so this isn't exotic.
This is pre-existing and unaffected by this PR: lightwalletd does exactly the same thing today, and so does zebra's own gRPC server. All that changes here is where the filter runs.
There's a smaller related question. Does a reply with fewer entries than maxEntries mean there are no more? Clients have to assume yes, since it's their only stopping signal, but the proto never says so. I'm implementing the zebra side now and it has to go out of its way to make that true — a UTXO the finalized index returns can turn out to be spent by a block in the non-finalized chain, so a limited query has to over-fetch to compensate.
I think the real fix is a resume cursor over (height, tx index, output index) rather than a bare height. That's already the order results come back in, and it's a total order, so resuming is unambiguous and the "am I done" question answers itself. Happy to open an issue on lightwallet-protocol if you agree it's worth tracking.
GetAddressUtxostakes astartHeightand amaxEntries, but today both are applied to the backend's reply rather than to the request: lightwalletd asks the node for every utxo held by the named addresses, unmarshals all of it, and only then drops what falls outside the range or past the limit. The node's work, and lightwalletd's allocation, are set by the size of the address' utxo set alone — nothing the client asks for reduces either.That is the shape the remaining part of GHSA-x4m7-3gpp-xc36 describes. Name one address with a large utxo set and put
startHeightat the chain tip: the request is a few dozen bytes, the reply is empty, and the node still produces the whole set. The 10,000-address cap added in 0.5.0 bounds a different dimension and does nothing here, since one address is enough, and there is no return-bandwidth cost to throttle the caller with.This passes both values to the
getaddressutxosRPC so a backend can apply them where the work happens.Compatibility
The two arguments are optional and omitted when unset, so a request that specifies neither is serialized exactly as it was before this change, and a backend that doesn't implement them ignores the extra keys. Verified against zebrad, which deserializes the request object with serde and has no
deny_unknown_fields, so unknown keys are skipped. No capability probe or version gate is needed.The reply-side filter stays permanently rather than becoming conditional on backend support. It costs nothing once the backend has already narrowed the result, and it means correctness never depends on the backend honoring the arguments — only performance does. That is what makes it safe to merge this half first, before any backend implements it. This PR is inert on its own; the CHANGELOG entry is worded accordingly and does not claim a fix.
A requirement for the backend side
"Which entries" is well defined:
service.protoalready documents the reply as sorted by height, and zebra returns utxos in chain order (height, tx index, output index) and asserts it. A backend applyingmaxEntriestakes the first N in that order, which is the same subset lightwalletd's filter would have kept.A backend must implement both arguments or neither. Honoring
maxEntrieswhile ignoringstartHeightwould truncate before the range filter runs and silently return fewer entries than exist, and lightwalletd has no way to detect that. This belongs in the RPC's specification, and the zebrad-side change is where it needs to land.Testing
TestGetAddressUtxosPushesDownLimitsasserts all three claims: that the arguments reach the backend, that the reply-side filter still produces the right utxos when the backend ignores them, and that an unset pair is omitted from the serialized request. ThestartHeight/maxEntriesfilter had no test coverage before this.