Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UIP 4: Spend Backreferences #2

Merged
merged 15 commits into from
Nov 16, 2024
Merged

UIP 4: Spend Backreferences #2

merged 15 commits into from
Nov 16, 2024

Conversation

redshiftzero
Copy link
Member

Draft specification of Spend Backreferences to enable fast DAGSync. First discussed here: https://forum.penumbra.zone/t/uip-spend-backreferences/110

@hdevalence hdevalence changed the title UIP: Spend Backreferences UIP 4: Spend Backreferences Nov 8, 2024
@hdevalence
Copy link
Member

Assigned UIP number 4 to this UIP

uips/uip-4.md Outdated Show resolved Hide resolved
uips/uip-4.md Outdated Show resolved Hide resolved
uips/uip-4.md Outdated Show resolved Hide resolved
uips/uip-4.md Outdated Show resolved Hide resolved
uips/uip-4.md Outdated
k = BLAKE2b-512("Penumbra_Backref", ovk)
```

One advantage of using a single key is that we can scan all spends using this key without having to do key derivation each time.
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to clarify this relative to the OVK, my understanding of the design choices is:

  • Like the OVK, having a single symmetric key avoids the need to perform key exchange operations
  • Deriving the BRK from the OVK is cleaner and provides capability separation (in principle, it allows giving permission to see the transaction graph but no other details)

Does that seem right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Another reason is that it's a good idea to avoid having the same key serve multiple cryptographic purposes to avoid confusion attacks or other oddities.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is correct: I agree that two advantages of the brk is it is a key that has a single purpose that can be disclosed to show the transaction graph only.

Unpacking the situation for outgoing scanning, for each note, we first attempt to decrypt the OvkWrappedKey using a key derived from the OutgoingViewingKey and the other public fields (value commitment, note commitment, ephemeral public key) prior to doing DH key exchange so we don't need to do key exchange before we know the action is ours. This is also true of scanning with the BackReference Key.

The other uses of symmetric crypto in our transactions use unique keys and fixed nonces, the difference here with the BackReference Key is that we're not using a unique key per action, and instead are using a unique nonce per action.

Copy link
Member Author

Choose a reason for hiding this comment

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

added 3d6e495

uips/uip-4.md Outdated Show resolved Hide resolved
uips/uip-4.md Outdated Show resolved Hide resolved
uips/uip-4.md Outdated Show resolved Hide resolved
uips/uip-4.md Outdated Show resolved Hide resolved
uips/uip-4.md Outdated Show resolved Hide resolved

DAGSync is a graph-aware fast syncing algorithm. A client, upon detecting a single transaction involving them, can check that outputs visible to them are spent or not. If the output is unspent, then they have identified a live note they can potentially spend in the future, else if the output is unspent, they can continue the process forwards in the transaction graph, until they reach unspent notes.

The design of Penumbra currently does not allow traversal backwards through the transaction graph, only forwards. A `Spend` intentionally does not reveal the note being spent, only the nullifier that is revealed. By including on the `Spend` an encrypted reference back to the note commitment being spent, such that only the note owner can view it, we enable DAGSync clients to efficienctly reconstruct the transaction history both backwards and forwards.
Copy link
Member

Choose a reason for hiding this comment

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

I think there is a conceptual issue here that's worth discussing at some point in the UIP, namely, that note commitments uniquely identify note contents, but not note instances. Two identical notes can be created and added to the chain, and they will have the same note commitment, although they are different instances. (This is why nullifier derivation hashes the position of the note, and it's the same conceptual mismatch that creates the faerie gold attack).

In practice, note commitments are usually also commitments to note instances, because honest users should generate random rseed values, which causes the commitments to be unique. But this isn't guaranteed by the protocol, and we should pay attention to the implications.

My suspicion is that the note commitment is still the right thing to put in the backreference, but it's worth thinking through the implications and writing that up in the UIP. What are the consequences of non-unique backreferences? Should the backreference also include the position, so that it is unique? (I believe the position is supplied as part of the SpendPlan data to be able to derive a nullifier, so I think this wouldn't introduce any new data/plumbing).

For instance, is a tagging attack possible along the following lines:

  • User A sends user B a sequence of notes with identical note commitments
    • Maybe this happens over time so that user B spends each of them in turn (rather than having new ones displace old ones)
  • User B resyncs, queries for transaction IDs by note commitment
    • Something happens? Or not? Maybe the RPC returns all matching TxIds, maybe it just returns one of them and some data is missing?

After thinking about it a little (the time it took to write this comment) I don't think there's a real issue here, but probably good to cover in the writeup.

Copy link
Contributor

Choose a reason for hiding this comment

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

In terms of the graph, you might already have multiple backreferences in a given transaction, so if one backreference points to two transactions, or you have two backreferences pointing to two transactions, the syncing algorithm works the same way, so I don't think there's really an issue there.

I think we just need to make sure to be aware of the non-uniqueness, and not do some kind of clobbering accidentally.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the transaction ID by note commitment, I think the RPC should ideally be returning all matching TxIds, instead of the most recent one such that no data is "lost" - i.e. the Penumbra protocol already allows there to be duplicate note commitments and we should keep that in mind. User B would continue to DAGSync with the list of relevant TxIds they got for the entire transaction, and from there can recover their transaction history, so I think this is fine. It's definitely worth flagging this possibility in the UIP so I'll add some notes to the Rationale.

Copy link
Member Author

Choose a reason for hiding this comment

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

One implication is that an attacker could attempt to spam a target user with notes that have duplicate note commitments such that DAGSyncing takes slightly longer. Fees should mitigate that though.

Copy link
Member Author

Choose a reason for hiding this comment

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

added e04f488

@hdevalence
Copy link
Member

hdevalence commented Nov 8, 2024

This looks great!

Tracking a few discussion points to resolve:

@conorsch
Copy link
Contributor

@redshiftzero Mind rebasing yet again? There's a conflict with the cleanup in the README, from #5. And I just added some CI jobs (#7, #9) for deployment and linting. They should run post-rebase.

@redshiftzero redshiftzero force-pushed the uip-spend-backreference branch from 4d4b468 to ec01253 Compare November 12, 2024 22:03
Copy link

github-actions bot commented Nov 12, 2024

Visit the preview URL for this PR (updated for commit c3d5347):

https://penumbra-uips--pr2-uip-spend-backrefere-q4mm3ori.web.app

(expires Fri, 22 Nov 2024 23:49:16 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 54ff667eaf61c617385479f19496d509f9179622

@conorsch
Copy link
Contributor

This looks solid as is. There's still a draft implementation coming together in penumbra-zone/penumbra#4922, which could show surprises, but if so we'll address those when they crop up.

@redshiftzero redshiftzero force-pushed the uip-spend-backreference branch from f5af69a to c3d5347 Compare November 15, 2024 23:46
@redshiftzero redshiftzero merged commit e0ec642 into main Nov 16, 2024
3 checks passed
@redshiftzero redshiftzero deleted the uip-spend-backreference branch November 16, 2024 01:59
cronokirby pushed a commit to penumbra-zone/penumbra that referenced this pull request Nov 22, 2024
## Describe your changes

This implements UIP-4 as described in
penumbra-zone/UIPs#2

## Issue ticket number and link

penumbra-zone/UIPs#2

## Checklist before requesting a review

- [ ] I have added guiding text to explain how a reviewer should test
these changes.

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:
conorsch pushed a commit to penumbra-zone/penumbra that referenced this pull request Nov 22, 2024
## Describe your changes

This implements UIP-4 as described in
penumbra-zone/UIPs#2

## Issue ticket number and link

penumbra-zone/UIPs#2

## Checklist before requesting a review

- [ ] I have added guiding text to explain how a reviewer should test
these changes.

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants