Remove `without_storage_info` for the `node-authorization` pallet #11621

koute · 2022-06-08T07:41:19Z

This PR changes the logic in the following way:

The well known nodes inserted during genesis are now bound by the MaxPeerIdLength and MaxWellKnownNodes constants. (Previously this wasn't enforced.)
The reset_well_known_nodes now enforces that the peer IDs passed in the nodes are bound by MaxPeerIdLength. (Previously this wasn't enforced.)
The reset_well_known_nodes now enforces that the number of nodes is bound by exactly MaxWellKnownNodes. (Previously it enforced a limit of MaxWellKnownNodes - 1, which I assume was an off-by-one error?)
The set inside of the AdditionalConnections is now bound by the MaxAdditionalConnections constant. (Previously it was uncapped.)
The add_connections and remove_connections now enforce that the peer IDs passed in the connections are bound by MaxPeerIdLength. (Previously this wasn't enforced.)
The offchain_worker hook now enforces that the state.peer_id is now at most MaxPeerIdLength long. (Previously this wasn't enforced.)

Marking as D5-nicetohaveaudit since this does change the logic.

Since this enforces extra bounds which weren't previously enforced do we need to do anything extra here?

KiChjang · 2022-06-08T12:57:39Z

frame/node-authorization/src/lib.rs

+	#[derive(Encode, Decode, RuntimeDebug, TypeInfo, MaxEncodedLen)]
+	#[codec(mel_bound())]
+	#[scale_info(skip_type_params(T))]
+	pub struct BoundedPeerId<T: Config>(BoundedVec<u8, T::MaxPeerIdLength>);


I think we should really try to give an upper bound on PeerId itself, rather than creating a container type around it to sidestep the issue. @shawntabrizi How exactly should we make primitive types bounded?

stale · 2022-07-16T08:46:29Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

koute · 2022-07-16T09:22:19Z

not stale

stale · 2022-08-15T09:40:27Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

koute · 2022-08-15T09:41:15Z

not stale

stale · 2022-09-14T10:34:59Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

koute · 2022-09-14T12:01:32Z

not stale

…_authorization

stale · 2022-10-14T17:45:35Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

koute · 2022-10-15T10:10:58Z

not stale

ggwpez · 2022-10-17T13:24:31Z

frame/node-authorization/src/lib.rs

+	impl<T: Config> PartialEq for BoundedPeerId<T> {
+		fn eq(&self, rhs: &Self) -> bool {
+			self.0.eq(&rhs.0)
+		}
+	}


Suggested change

impl<T: Config> PartialEq for BoundedPeerId<T> {

fn eq(&self, rhs: &Self) -> bool {

self.0.eq(&rhs.0)

}

}

There are PartialEqNoBound, CloneNoBound and a few others.

ggwpez · 2022-10-17T13:38:52Z

frame/node-authorization/src/lib.rs

 			let owner = Owners::<T>::get(&node).ok_or(Error::<T>::NotClaimed)?;
 			ensure!(owner == sender, Error::<T>::NotOwner);

-			let mut nodes = AdditionalConnections::<T>::get(&node);
-
+			let mut nodes: BTreeSet<_> = AdditionalConnections::<T>::get(&node).into();


Nit:

Suggested change

let mut nodes: BTreeSet<_> = AdditionalConnections::<T>::get(&node).into();

let mut nodes = AdditionalConnections::<T>::get(&node);

then you can try_insert below and dont have to bound it again.

ggwpez · 2022-10-17T13:44:26Z

frame/node-authorization/src/lib.rs

+		for (peer_id, account_id) in nodes {
+			let peer_id = BoundedPeerId::<T>::try_from(peer_id.clone())?;
+			peer_ids.try_insert(peer_id.clone()).map_err(|_| Error::<T>::TooManyNodes)?;
+			owners.push((peer_id, account_id));


Suggested change

owners.push((peer_id, account_id));

Owners::<T>::insert(peer_id, account_id);

and remove the owners vector?

stale · 2022-11-17T01:45:28Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

koute · 2022-11-17T02:07:59Z

not stale

stale · 2022-12-17T02:21:32Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

…_authorization

stale · 2023-02-09T10:22:31Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2023-03-12T13:06:39Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

juangirini · 2023-06-09T12:26:17Z

@koute do you feel like taking this over again? We will review it 😇

koute · 2023-06-09T12:34:59Z

@koute do you feel like taking this over again? We will review it

Sure I can. Could I get some feedback as to how this should be done? Is the way I did it okay, or does anything else has to change? (Besides @ggwpez 's comments which I'll address.)

juangirini · 2023-06-09T13:53:02Z

I think this is pretty much ready. @ggwpez I loop you in as you are more familiar with it to answer koute

Remove without_storage_info for the node-authorization pallet

e78f25d

koute requested review from shawntabrizi, KiChjang and ggwpez June 8, 2022 07:41

shawntabrizi mentioned this pull request Aug 24, 2023

[FRAME Core] Remove without_storage_info on pallets paritytech/polkadot-sdk#323

Open

40 tasks

KiChjang reviewed Jun 8, 2022

View reviewed changes

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 16, 2022

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 16, 2022

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Aug 15, 2022

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Aug 15, 2022

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 14, 2022

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 14, 2022

Merge remote-tracking branch 'origin/master' into master_bounded_node…

13a9c23

…_authorization

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Oct 14, 2022

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Oct 15, 2022

ggwpez approved these changes Oct 17, 2022

View reviewed changes

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Nov 17, 2022

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Nov 17, 2022

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Dec 17, 2022

stale bot closed this Dec 31, 2022

Merge remote-tracking branch 'origin/master' into master_bounded_node…

6f8f2ed

…_authorization

koute reopened this Jan 10, 2023

stale bot removed A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. labels Jan 10, 2023

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Feb 9, 2023

koute removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Feb 10, 2023

stale bot added the A3-stale label Mar 12, 2023

stale bot closed this Mar 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `without_storage_info` for the `node-authorization` pallet #11621

Remove `without_storage_info` for the `node-authorization` pallet #11621

koute commented Jun 8, 2022 •

edited

Loading

KiChjang Jun 8, 2022

stale bot commented Jul 16, 2022

koute commented Jul 16, 2022

stale bot commented Aug 15, 2022

koute commented Aug 15, 2022

stale bot commented Sep 14, 2022

koute commented Sep 14, 2022

stale bot commented Oct 14, 2022

koute commented Oct 15, 2022

ggwpez Oct 17, 2022

ggwpez Oct 17, 2022

ggwpez Oct 17, 2022

stale bot commented Nov 17, 2022

koute commented Nov 17, 2022

stale bot commented Dec 17, 2022

stale bot commented Feb 9, 2023

stale bot commented Mar 12, 2023

juangirini commented Jun 9, 2023

koute commented Jun 9, 2023

juangirini commented Jun 9, 2023

	let mut nodes: BTreeSet<_> = AdditionalConnections::<T>::get(&node).into();
	let mut nodes = AdditionalConnections::<T>::get(&node);

	owners.push((peer_id, account_id));
	Owners::<T>::insert(peer_id, account_id);

Remove without_storage_info for the node-authorization pallet #11621

Remove without_storage_info for the node-authorization pallet #11621

Conversation

koute commented Jun 8, 2022 • edited Loading

KiChjang Jun 8, 2022

Choose a reason for hiding this comment

stale bot commented Jul 16, 2022

koute commented Jul 16, 2022

stale bot commented Aug 15, 2022

koute commented Aug 15, 2022

stale bot commented Sep 14, 2022

koute commented Sep 14, 2022

stale bot commented Oct 14, 2022

koute commented Oct 15, 2022

ggwpez Oct 17, 2022

Choose a reason for hiding this comment

ggwpez Oct 17, 2022

Choose a reason for hiding this comment

ggwpez Oct 17, 2022

Choose a reason for hiding this comment

stale bot commented Nov 17, 2022

koute commented Nov 17, 2022

stale bot commented Dec 17, 2022

stale bot commented Feb 9, 2023

stale bot commented Mar 12, 2023

juangirini commented Jun 9, 2023

koute commented Jun 9, 2023

juangirini commented Jun 9, 2023

Remove `without_storage_info` for the `node-authorization` pallet #11621

Remove `without_storage_info` for the `node-authorization` pallet #11621

koute commented Jun 8, 2022 •

edited

Loading