feat(rust/rbac-registration): RBAC stolen StakeAddress handling.
#631
Conversation
Mr-Leshiy
added
squad: gatekeepers
stanislav-tkach
left a comment
stanislav-tkach
left a comment
There was a problem hiding this comment.
I think the change is ok in general, but I don't quite get the idea with the updated URI set. As far as I can see, we are still not storing the full history and only the final state in Cip0134UriSet. In that case why do we want to store "taken" addresses separately? Why not store the resulting final state? For example currently we would have something like this:
x_uris: [A, B, C],
c_uris: [D, E],
taken_uris: [B, C, D],But it can be represented as
x_uris: [A],
c_uris: [E],Am I missing something?
| x_uris: UrisMap, | ||
| /// URIs from c509 certificates. | ||
| c_uris: UrisMap, | ||
| /// URIs which are taken by another certificates. |
There was a problem hiding this comment.
Nitpick: I think it would be better to say "by another chains".
| /// Returns a set of stake addresses by the given role. | ||
| #[must_use] | ||
| pub fn role_stake_addresses( |
There was a problem hiding this comment.
As for now, the behavior of role_stake_addresses and role_addresses functions is inconsistent with the one of the stake_addresses function. The latter one doesn't include taken addresses, but the former ones do.
We should probably add unit tests for these methods.
| /// URIs from c509 certificates. | ||
| c_uris: UrisMap, | ||
| /// URIs which are taken by another certificates. | ||
| taken_uris: HashSet<Cip0134Uri>, |
There was a problem hiding this comment.
As far as I can see, we can now mark as "taken" all address types, not just stake addresses. Is this desired behavior?
| /// Return the updated URIs set where the provided URIs were taken by other | ||
| /// registration chains. | ||
| /// | ||
| /// Updates the current URI set by removing the taken URIs from it. |
There was a problem hiding this comment.
I think the wording is slightly confusing: this code doesn't remove URIs from the set, it adds (marks?) such URIs as taken.
| /// Update the update by "taking" the given `StakeAddress` for the correspoding RBAC | ||
| /// chain's by the given `CatalystId`. |
There was a problem hiding this comment.
| /// Update the update by "taking" the given `StakeAddress` for the correspoding RBAC | |
| /// chain's by the given `CatalystId`. | |
| /// Update the chain by transferring the given `StakeAddress` to the chain with the given `CatalystId`. |
| fn take_stake_address_from_chain( | ||
| &mut self, | ||
| id: &CatalystId, | ||
| address: &StakeAddress, |
There was a problem hiding this comment.
Wouldn't it be better to pass a set (or an iterator) instead of calling this method multiple times?
| fn take_stake_address_from_chain( | ||
| &mut self, | ||
| id: &CatalystId, | ||
| address: &StakeAddress, | ||
| ) -> impl Future<Output = anyhow::Result<()>> + Send; |
There was a problem hiding this comment.
I'm not sure I understand the updated workflow. If we always need a Cip509 object to update a chain (even if it belongs to another chain) then do we still need to provide stake addresses here?
Also I think it would be better to have a pull request to catalyst-gateway to see the usage of the updated API.
| // Checks that a new registration doesn't contain a signing key that was used by any | ||
| // other chain. Returns a list of public keys in the registration. |
There was a problem hiding this comment.
| // Checks that a new registration doesn't contain a signing key that was used by any | |
| // other chain. Returns a list of public keys in the registration. | |
| // Checks that a new registration doesn't contain a signing key that was used by any | |
| // other chain. |
| for role in cip509.all_roles() { | ||
| if let Some(key) = cip509.signing_pk_for_role(role) { | ||
| if let Some(previous) = state.chain_catalyst_id_from_public_key(&key).await? { | ||
| if &previous != cat_id { | ||
| cip509.report().functional_validation( | ||
| &format!("An update to {cat_id} registration chain uses the same public key ({key:?}) as {previous} chain"), | ||
| "It isn't allowed to use role 0 signing (certificate subject public) key in different chains", | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
As far as I can see, the exact same code is used in the update function. Can we reuse it?
| if let Some(id) = state.chain_catalyst_id_from_stake_address(address).await? { | ||
| state.take_stake_address_from_chain(&id, address).await?; | ||
| } |
There was a problem hiding this comment.
Please add a comment that it is only allowed to take stake addresses from the existing chain if the new one uses a different signing key, but it is ok because all keys must be unique anyway.
|
Also I would like to see how we handle the caching on the |
Description
Processing scenario when another registration chain could "stole"
cip0134uriaddress, so it become not accessible for the current one after applying such registration.Related Issue(s)
Part of #578
Description of Changes
taken_uris: HashSet<Cip0134Uri>,andupdate_taken_urisforCip0134UriSetstruct. Also modify the originalupdatemethod, so it removes thetaken_uris, which enables the functionality of returning back "stolen" addresses.signing_pk_for_rolemethod for theCip509type.new,updatemethods for theRegistrationChaintype.Please confirm the following checks