Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,20 @@ The most recent changes are listed first.

## [Unreleased]

### Changed

- `GetAddressUtxos` and `GetAddressUtxosStream` now pass `startHeight` and
`maxEntries` to the backend `getaddressutxos` RPC, instead of only applying
them to the reply. No backend implements the arguments yet, and a backend
that doesn't implement them ignores the extra JSON keys, so on its own this
changes nothing: the client-side filter stays, and correctness never depends
on the backend honoring them. It is the lightwalletd half of moving the
limits to where the work is done, which is what the unremediated part of
GHSA-x4m7-3gpp-xc36 needs -- today a request naming a single address with a
large UTXO set makes the node produce that entire set, however narrow a
height range or however few entries the client asked for. A request that
sets neither argument is serialized exactly as it was before.

### Fixed

- `GetTaddressBalance` now rejects an address list longer than the same 10,000
Expand Down
3 changes: 3 additions & 0 deletions common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,9 @@ type (
// zcashd rpc "getaddressutxos"
ZcashdRpcRequestGetaddressutxos struct {
Addresses []string `json:"addresses"`
// Optional; a backend that doesn't implement these ignores them.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LarryRuane do you think we should implement these at the lightclient-protocol level as well?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

StartHeight uint64 `json:"startHeight,omitempty"`
MaxEntries uint32 `json:"maxEntries,omitempty"`
}
ZcashdRpcReplyGetaddressutxos struct {
Address string
Expand Down
67 changes: 67 additions & 0 deletions frontend/frontend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,73 @@ type testgettx struct {
walletrpc.CompactTxStreamer_GetTaddressTransactionsServer
}

func TestGetAddressUtxosPushesDownLimits(t *testing.T) {
testT = t
defer resetGlobals()
lwd, _ := testsetup()

taddr := "t1" + strings.Repeat("a", 33)
utxo := func(height int) common.ZcashdRpcReplyGetaddressutxos {
return common.ZcashdRpcReplyGetaddressutxos{
Address: taddr,
Txid: "0788e4dc9973cd9a54e0f4d51ec96f4b8e6a8e0f8a1e1e9e4b2c2a1d0e0f0a0b",
OutputIndex: 0,
Script: "76a914000000000000000000000000000000000000000088ac",
Satoshis: 1000,
Height: height,
}
}

// Model a backend that doesn't implement the new arguments: it returns
// every utxo, in chain order, whatever the request asked for.
var sent json.RawMessage
common.RawRequest = func(ctx context.Context, method string, params []json.RawMessage) (json.RawMessage, error) {
if method != "getaddressutxos" {
testT.Fatal("unexpected method", method)
}
sent = params[0]
return json.Marshal([]common.ZcashdRpcReplyGetaddressutxos{
utxo(100), utxo(200), utxo(300), utxo(400),
})
}

reply, err := lwd.GetAddressUtxos(context.Background(), &walletrpc.GetAddressUtxosArg{
Addresses: []string{taddr},
StartHeight: 200,
MaxEntries: 2,
})
if err != nil {
t.Fatal("GetAddressUtxos failed:", err)
}
var req common.ZcashdRpcRequestGetaddressutxos
if err := json.Unmarshal(sent, &req); err != nil {
t.Fatal("could not unmarshal getaddressutxos request")
}
if req.StartHeight != 200 || req.MaxEntries != 2 {
t.Fatal("expected the limits to reach the backend, got:", string(sent))
}
// The backend ignored them, so the client-side filter must still hold.
heights := make([]uint64, 0)
for _, u := range reply.AddressUtxos {
heights = append(heights, u.Height)
}
if !reflect.DeepEqual(heights, []uint64{200, 300}) {
t.Fatal("expected utxos at heights 200 and 300, got:", heights)
}

// A request that sets neither limit is byte-for-byte what it was before
// the arguments existed, so an unpatched backend sees no change.
_, err = lwd.GetAddressUtxos(context.Background(), &walletrpc.GetAddressUtxosArg{
Addresses: []string{taddr},
})
if err != nil {
t.Fatal("GetAddressUtxos failed:", err)
}
if want := `{"addresses":["` + taddr + `"]}`; string(sent) != want {
t.Fatal("expected unset limits to be omitted, got:", string(sent))
}
}

func (tg *testgettx) Context() context.Context {
return context.Background()
}
Expand Down
6 changes: 5 additions & 1 deletion frontend/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -918,7 +918,9 @@ func getAddressUtxos(ctx context.Context, arg *walletrpc.GetAddressUtxosArg, f f
addresses = append(addresses, a)
}
addrList := &common.ZcashdRpcRequestGetaddressutxos{
Addresses: addresses,
Addresses: addresses,
StartHeight: arg.StartHeight,
MaxEntries: arg.MaxEntries,
}
param, err := json.Marshal(addrList)
if err != nil {
Expand Down Expand Up @@ -946,13 +948,15 @@ func getAddressUtxos(ctx context.Context, arg *walletrpc.GetAddressUtxosArg, f f
}
n := 0
for _, utxo := range utxosReply {
// Re-apply the limits; a backend that ignored them sent everything.
if uint64(utxo.Height) < arg.StartHeight {
continue
}
n++
if arg.MaxEntries > 0 && uint32(n) > arg.MaxEntries {
break
}

txidBigEndian, err := hex.DecodeString(utxo.Txid)
if err != nil {
return status.Errorf(codes.Internal,
Expand Down
Loading