Skip to content

PDP service contract whitelist #545

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

Open
wants to merge 2 commits into
base: synapse
Choose a base branch
from

Conversation

ZenGround0
Copy link
Collaborator

This provides some protection for curio PDP operators running publically accessible PDP services. Only the pandora PDP service contract is allowed upon proofset creation over the publically available PDP service.

The whitelist is network dependent -- only calibnet specified as pandora only launched on calibnet.

@ZenGround0
Copy link
Collaborator Author

cc @rvagg


// Check if the recordkeeper is in the whitelist
for _, allowed := range allowedList {
if recordKeeper == allowed {
Copy link
Member

Choose a reason for hiding this comment

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

Do you know if common.Address is going to be happy with case-insensitive forms of the address? Should we just lower-case all addresses to avoid any issues with case sensitivity and not demand the user provide the correct validation form?

Copy link
Collaborator Author

@ZenGround0 ZenGround0 Jun 24, 2025

Choose a reason for hiding this comment

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

Good question. After looking into it the answer is that case insensitivity is handled correctly.

Tracing through common.Address everything relies in a straightforward way on golang stdlib hex encode decode. Stdlib reverse lookup from ascii code to hex value is here: https://cs.opensource.google/go/go/+/refs/tags/go1.24.4:src/encoding/hex/hex.go;l=25 and you can see capital letters are treated correctly.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

fair enough if this is only for the synapse branch, but maybe it should be a config option rather than defined in code?

also go fmt apparently not right in here somewhere

@ZenGround0
Copy link
Collaborator Author

fair enough if this is only for the synapse branch, but maybe it should be a config option rather than defined in code?

I see where you're coming from but since it only applies to the public service I am ok with leaving it as a build constant like the existing verifier. Merging synapse back to main will require some conceptual merging of the public service construct and we can revisit this question then as part of that.

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.

2 participants