-
Notifications
You must be signed in to change notification settings - Fork 4
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
Registry Spec #2
Conversation
/// @param node Address of the node. | ||
event NodeAdded(uint256 indexed nodeId, address indexed node); | ||
/// @notice This event MUST be emitted when a filter is added to the registry. | ||
event FilterAdded(address indexed filter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if additing filters is necessary?
Given that we talked about making this feature permissionless, it seems that the only real benefit here is that it might make it easier for applications/fontends to know and display available filters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Yeah we should keep it permissionless. Explicitly adding and emitting the event feels right somehow. What you point out is one scenario scenario.
You have any concrete thoughts on whether include/remove?
bool eligible; | ||
// Number of duties the node has not yet completed. | ||
// Incremented each time a duty is added, decremented each time a duty is completed. | ||
uint256 outstandingDuties; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a core component of the protocol that will dictate whether or not a node can currently be decomissioned.
This counter was my first attempt at solving for this, however it implies a lot of onchain state management, since it would need to be incremented/decemented for each node in each committee both when the committee is formed and when the committee's duties expire.
My guess is that there is a more storage efficient route to this, probably something like a nullifier proof to verify that a given node is not the list of nodes with currently active duties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In either case though, I don't think this can live in the filter contracts. It probably needs to be a core feature of the registry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, I took this comment as a major flaw in the direction of this PR. However, upon revisiting it, I think this is only partially the case.
One thing is certain: the commissioning and decommissioning of nodes do not belong in the filter logic. There’s no question about that, you comment is correct.
However, the Registry already maintains a state indicating whether a Cyphernode is active. For the registry, the only important factor is whether a node can be included in a committee selection. It doesn’t matter whether the node hasn’t been added yet, is serving too many duties, or has been slashed and its bond no longer suffices.
So, the relevant question for the registry is whether a node is fit to be included in an upcoming commission. This is a simple boolean value.
What seems to be missing is a new entity to handle the logic for staking, entering commissions, leaving commissions with prizes, and leaving commissions with slashes.
I'm going to push a couple more commits and summarize the current status of the phase design in a consolidated PR comment. I'll try to incorporate this idea there.
Let me know if you have any immediate feedback on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what you're suggesting here is essentially breaking the registry up into a multiple components, one that is the canonical source of truth for whether or not a given node is currently eligible to be selected for committee and one that is responsible for maintaining the list of eligible nodes (along with slashing conditions, etc)?
This seems reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears to me that ICommitteeCoordinator is actually acting as the registry, which seems to be conflating the role of the registry and the filers.
The registry is a core component to the protocol. Global node membership is not something that should be defined by the requestor. Rather, requestors should be able to define the filters that they want to apply to the global registry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put another way, the ICyphernodeRegistry
should handle node registration and decommissioning (currently permissioned by an owner address, in future permissionless via staking). Regardless of the filters applied, every node selected for a computation must be a member of the global list of eligible nodes to ensure that they are staked and the system maintains economic security.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is a good point, and it's what I meant when I mentioned in the DMs that I hadn't given any thought to the slashing logic and mechanism. Let me think it through
…at registry level
…nc process, we require the filter to post/publish the resulting key
Warning Review failedThe pull request is closed. WalkthroughThe changes introduce significant updates in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Enclave
participant CyphernodeRegistry
participant RegistryFilter
User ->> Enclave: request(filter, threshold, ...)
Enclave ->> CyphernodeRegistry: requestCommittee(e3Id, filter, threshold)
CyphernodeRegistry ->> RegistryFilter: requestCommittee(e3Id, threshold)
RegistryFilter -->> CyphernodeRegistry: success or failure
CyphernodeRegistry -->> Enclave: Emit CommitteeRequested
Enclave -->> User: Emit E3Requested
User ->> Enclave: publishInput(e3Id, input)
Enclave -->> Enclave: Accumulate Inputs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
This PR is a first attempt at specifying the interface for the cyphernode registry, along with its components. It incides mainly in the Committee Selection Pahse
Its main focus is the two-step asynchronous committee selection phase that utilizes a pluggable entity called
IRegistryFilter
.Key Points
IRegistryFilter
to initiate the selection of the committee.Security Considerations
Implementation Considerations
Cyphernode State
Summary by CodeRabbit
New Features
Bug Fixes
Tests