-
Notifications
You must be signed in to change notification settings - Fork 106
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
Content Node Chain of Trust [Phase 2] - automatic node self-registration on URSM (And new Content Node Init pattern) #1232
Conversation
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 PR looks great. had one comment about while true. i'm basically ready to approve if you change that or feel we shouldn't change that
@dmanjunath @hareeshnagaraj addressed all comments pls re-review |
@@ -230,8 +255,29 @@ const verifyUrsmContentNodes = async (executeOne) => { | |||
) | |||
} | |||
logger.info(`Found UserReplicaSetManager and ServiceProviderFactory match for spID=${spID}, delegateWallet=${delegateWalletFromChain}, ownerWallet=${ownerWalletFromChain}`) | |||
|
|||
/** | |||
* Confirm nodes with spIDs 1-3 are correctly configured as bootstrappers and any additional nodes were registered with proposals from registered nodes |
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.
just to clarify, we know the order here because ursm content nodes are ordered by ID in the route from disc prov and the requesting node aggregates the proposer responses in said order?
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.
nah we don't know the order in which the requesting node aggregates proposer responses. this code works bc it makes no assumptions on the order of those responses
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.
Changes look great, love the code quality and thoroughness here - comments, testing, PR description 💯
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.
LGTM!
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.
YESSSSSSS
Description
What is the purpose of this PR? What is the current behavior? New behavior? Relevant links (e.g. Trello) and/or information pertaining to PR?
Linear card
Notion doc
The PR for Phase 1 of this task is #1260
This is Phase 2 of chain of trust authentication flow for content nodes to register with new UserReplicaSetManager contract (URSM) in order to be eligible to be added to user's replica sets.
This PR "activates" the code from Phase 1, by adding a CN service that automatically registers the node against URSM on startup, including requesting signatures from registered nodes using the code from Phase 1 above.
Included:
URSMRegistrationManager
service, responsible for registering the node. This service is entirely backwards compatible, working before and after contract is deployedspID
) out ofSnapbackSM
service for better logical separation of concerns. Now handled byServiceRegistry
ursm_request_for_signature
routeSnapbackSM
integration test fixTests
List the manual tests and repro instructions to verify that this PR works as anticipated. Include log analysis if possible.
❗ If this change impacts clients, make sure that you have tested the clients ❗
Testing steps:
Logs below show CN4 repeatedly trying to register on URSM until it succeeds
❗ Reminder 💡❗:
If this PR touches a critical flow (such as Indexing, Uploads, Gateway or the Filesystem), make sure to add the
requires-special-attention
label. Add relevant labels as necessary.