Governance MVP by gavofyork · Pull Request #42 · paritytech/substrate

gavofyork · 2018-01-21T21:38:06Z

NOTE: Grumbles addressed in #47 .

At this point "governance" is simply a qualified majority vote of the validators. This buys us the ability to do secure "decentralised" code upgrades. With this functionality, further (better) governance can be introduced in the PoC over time.

Still todo:

Tests
Docs
Additional internal functions.

…g and direct usage.

Also tests.

rphmeier · 2018-01-26T18:43:23Z

wasm-runtime/polkadot/src/support/storable.rs

+	}
+
 	/// Place the value in storage under `key`.
 	fn store(&self, key: &[u8]);


having this as an implicit function on the objects is a little weird. You end up with code like value.store("some_data") which reads a more like the value is having data added to it.

invocations like storage::store("some_key", &value) and storage::load("some_key") better indicate that implicit globals are being accessed.

currently every object that implements this trait has to manually perform storage, when really the only common functionality needed is serialization/deserialization. this will reduce duplication and make things cleaner in general

i'm happy with:

let count: usize = database::lookup_default(key); database::put(key, count + 1);

but i definitely don't think this is good:

let value_bytes = database::lookup_default(key); let count = <usize>::deserialise(value_bytes); let value_bytes = database::serialise(count + 1); database::store(key, &value_bytes);

rphmeier · 2018-01-27T18:44:46Z

wasm-runtime/polkadot/src/codec/slicable.rs

 		self.as_slice_then(|s| s.to_vec())
 	}
-	fn set_as_slice<F: FnOnce(&mut [u8]) -> bool>(set_slice: F) -> Option<Self>;
+	fn set_as_slice<F: Fn(&mut [u8], usize) -> bool>(set_slice: &F) -> Option<Self>;


It's more restrictive to take &F than F here because Fn is implemented for &F already.

see later comment.

rphmeier · 2018-01-27T18:45:23Z

wasm-runtime/polkadot/src/codec/slicable.rs

 		let mut result: T = unsafe { mem::zeroed() };
 		let result_slice = unsafe {
 			let ptr = &mut result as *mut _ as *mut u8;
 			slice::from_raw_parts_mut(ptr, size)


if size is zero then this is UB

the size is never zero - EndianSensitive is only implemented for "real" types. if there's a static check to ensure this is the case, great. Or some other way of preventing external code from using EndianSensitive. For now i'll make a runtime-assert.

rphmeier · 2018-01-27T18:46:29Z

wasm-runtime/polkadot/src/codec/slicable.rs

+		u32::set_as_slice(fill_slice).and_then(|len| {
+			let mut v = Vec::with_capacity(len as usize);
+			unsafe { v.set_len(len as usize); }
+			if fill_slice(&mut v, 4) {


better to just zero the vector:

it's 4 bytes

fill_slice might cause undefined behavior reading from uninitialized memory

unsafe code is bad

is this a bottleneck?

it's not 4 bytes. the 4 is the four byte offset; the vector could be huge e.g. a multi-MB block.

rphmeier · 2018-01-27T18:48:14Z

wasm-runtime/polkadot/src/runtime/governance.rs

+/// Proposal is by the `transactor` and will automatically count as an approval. Transactor must
+/// be a current validator. It is illegal to propose when there is already a proposal in effect.
+pub fn propose(validator: &AccountID, proposal: &Proposal) {
+	if Proposal::lookup(b"gov:pro").is_some() {


i'm in favor of extracting out all storage keys to useful constants.

yeah - i'm planning to explore a better storage keying mechanism in later PoCs.

rphmeier · 2018-01-27T18:48:46Z

wasm-runtime/polkadot/src/runtime/governance.rs

+/// Propose a sensitive action to be taken. Any action that is enactable by `Proposal` is valid.
+/// Proposal is by the `transactor` and will automatically count as an approval. Transactor must
+/// be a current validator. It is illegal to propose when there is already a proposal in effect.
+pub fn propose(validator: &AccountID, proposal: &Proposal) {


is proposal here used to mean something like a referendum? in the consensus layer we also have the term "proposal" used to mean something else.

it's meant to be "proposal for some arbitrary action, embodied as a privileged call".

rphmeier · 2018-01-27T18:50:42Z

wasm-runtime/polkadot/src/runtime/governance.rs

+/// than once for any validator in an era.
+pub fn approve(validator: &AccountID, era_index: BlockNumber) {
+	if era_index != staking::current_era() {
+		panic!("approval vote applied on non-current era.")


is panic being used as the signal for block processing failure? seems dangerous if we want to run it in a native context.

It would be better to propagate errors up using something like error_chain and use the bail! macro.

disagree vehemently. that would add a tonne of noise for no reason. within the context of the wasm runtime, a panic is always tantamount to block failure. native has a panic handler. unless there are cirsumstances where an intentional panic in the runtime would cause the main node to panic, then i see no reason to add it.

rphmeier · 2018-01-27T18:57:11Z

wasm-runtime/polkadot/src/support/proposal.rs

+	/// Derive `Some` value from a `u8`, or `None` if it's invalid.
+	pub fn from_u8(value: u8) -> Option<InternalFunction> {
+		match value {
+			x if x == InternalFunction::SystemSetCode as u8 => Some(InternalFunction::SystemSetCode),


since the enum variants aren't strictly defined it seems easy for these to change in a non backwards-compatible way.

fixed in latest.

rphmeier · 2018-01-27T18:57:24Z

wasm-runtime/polkadot/src/support/proposal.rs

+}
+
+impl Slicable for Proposal {
+	fn set_as_slice<F: Fn(&mut[u8], usize) -> bool>(fill_slice: &F) -> Option<Self> {


same here -- prefer F over &F

couldn't make it work as i need to recurse the argument into itself multiple times in at least one implementation.

rphmeier · 2018-01-27T18:58:00Z

wasm-runtime/polkadot/src/support/proposal.rs

+		let mut params = StreamReader::new(&self.input_data);
+		match self.function {
+			InternalFunction::SystemSetCode => {
+				let code: Vec<u8> = params.read().unwrap();


panic proofs are missing in this match

no need as this is runtime code. if anything goes wrong, the block is invalid and it'll be by the panic handler.

rphmeier · 2018-01-27T18:58:44Z

wasm-runtime/polkadot/src/support/storable.rs

 	/// Lookup `Some` value in storage and deserialise; `None` if it's not there.
-	fn lookup(_key: &[u8]) -> Option<Self> where Self: Sized { unimplemented!() }
+	fn lookup(_key: &[u8]) -> Option<Self> where Self: Sized {
+		unimplemented!()


having a default trait function be unimplemented! is strange. why not just leave it unimplemented?

👍 We had that in RPC some time ago and it caused some of the RPC methods to be unintentionally unimplemented because of that.

because this is not always implementable (i.e. when Self is not Sized). in that case, i can't leave it unimplemented but also can't implement it. having a default implementation is the only way around this i found. happy to change if there's a cleaner way.

(point is moot now with the new storage API)

rphmeier · 2018-01-27T18:59:21Z

wasm-runtime/polkadot/src/support/storable.rs

+
+	/// Retrives and returns the serialised value of a key from storage, removing it immediately.
+	fn take(key: &[u8]) -> Option<Self> where Self: Sized {
+		if let Some(value) = Self::lookup(key) {


Using Self in this way is pretty unidiomatic. A trait should provide the core functionality and then a module can expose free functions to leverage it.

storage API refactor can happen in later PR.

rphmeier

A few issues in unsafe code -- also worth addressing the same issue in Slicable::as_slice_then in this PR.

Style grumbles:

Usage of &F where F: Fn
The whole Storable interface is very awkward. I would suggest constructing our strongly-typed storage interface somewhat differently using the combination of three parts:

a utility for putting/getting raw bytes (storage::put_raw, storage::get_raw)
traits for serialization and deserialization
utilities for putting/getting strongly-typed data (storage::put<T: Serializable>, storage::get<T: Deserializable>)

As it stands, every type we want to put into and get from storage has to have the code which actually reads and writes it duplicated, complicating their implementations. It also improves readability IMO -- compare storage::put(b"key", my_value_of_any_type) vs. my_value.store(b"key"). The second option looks like we are just constructing an in-memory map, not like the value is being written to persistent global storage.

The governance logic looks fine.

tomusdrw · 2018-01-27T20:19:49Z

executor/src/wasm_executor.rs

+	ext_get_storage_into(key_data: *const u8, key_len: u32, value_data: *mut u8, value_len: u32, value_offset: u32) -> u32 => {
 		if let Ok(key) = this.memory.get(key_data, key_len as usize) {
 			if let Ok(value) = this.ext.storage(&key) {
+				let value = &value[value_offset as usize..];


Won't it panic if value.len() <= value_offset?

yup, and that's fine. panicking is an indication that the block is invalid.

tomusdrw · 2018-01-27T20:28:01Z

wasm-runtime/polkadot/src/runtime/governance.rs

+/// Approve the current era's proposal. Transactor must be a validator. This may not be done more
+/// than once for any validator in an era.
+pub fn approve(validator: &AccountID, era_index: BlockNumber) {
+	if era_index != staking::current_era() {


Sorry to chime in so late, first time reviewing any contract, but IMHO relying on some globally accessible APIs (especially if they can do alterations like u32::store(&str)) seem super bug-prone for me.
It also makes the code hard to analyze and encourages hacky changes instead of clean code design (or rather it doesn't enforce good design).

In general, I agree. But it might make sense for these specific externalities if you compare them to e.g. filesystem or network APIs which also rely on some global context. I just think we need sane wrappers. Things like true.store(b"foo") don't indicate that global state is being leaned on enough.

happy to try some other API, but in general i'd like to keep the storage API lean - perhaps not quite as lean as it is in solidity, but preferably not far off.

tomusdrw · 2018-01-27T20:28:46Z

wasm-runtime/polkadot/src/runtime/governance.rs

+/// validator. `1000` would require the approval of all validators; `667` would require two-thirds
+/// (or there abouts) of validators.
+pub fn set_approval_ppm_required(ppm: u32) {
+	ppm.store(b"gov:apr");


That looks super hacky, why there is no context object at least? IMHO it's super nice in rust that you see if by calling a function you're gonna modify some other object that is borrowed to it.

well, modifying storage is a bare function. i could create a dummy context and then pointlessly hand it around from function to function, but it makes little sense to add all of that noise.

tomusdrw · 2018-01-27T20:31:45Z

wasm-runtime/polkadot/src/runtime/governance.rs

+		kill(b"gov:pro");
+		let approvals_required = approvals_required();
+		let approved = session::validators().into_iter()
+			.filter_map(|v| bool::take(&v.to_keyed_vec(b"gov:app:")))


+1 for constans, gov:apr vs gov:app: is quite difficult to even distinguish visually. also what's app ro apr? why not full words?

sure - it's a fairly big job though so i'd prefer to leave it for a later PR.

tomusdrw · 2018-01-27T20:32:43Z

wasm-runtime/polkadot/src/runtime/governance.rs

+	use testing::{one, two, TestExternalities};
+	use primitives::AccountID;
+	use proposal::InternalFunction;
+	use runtime::{staking, session};


Already imported via super::* don't need to repeat the imports in submodules.

tomusdrw · 2018-01-27T20:35:13Z

wasm-runtime/polkadot/src/runtime/staking.rs

 impl StorageVec for IntentionStorageVec {
 	type Item = AccountID;
-	const PREFIX: &'static[u8] = b"ses:wil:";
+	const PREFIX: &'static[u8] = b"sta:wil:";


ses? wil? sta = staking? Another +1 for contants.

i'm planning on abstracting all this in the not-too-distant future to readable static values, but would prefer to leave it to another PR

tomusdrw · 2018-01-27T20:35:53Z

wasm-runtime/polkadot/src/runtime/staking.rs

-pub fn bondage(who: &AccountID) -> Bondage {
-	Storable::lookup_default(&who.to_keyed_vec(b"sta:bon:"))
-}
+// TRANSACTION API


Why not use modules instead of formatting comments?

done in the latest PR

tomusdrw · 2018-01-27T20:37:44Z

wasm-runtime/polkadot/src/runtime/staking.rs

+// PRIVILEDGED API
+
+/// Set the number of sessions in an era.
+pub fn set_sessions_per_era(new: BlockNumber) {


priviledged::set_sessions_per_era() would imho look nicer and would made it more visible to see that it's a priviledged API (especially at call site) than the comment above.

done in the latest PR.

tomusdrw · 2018-01-27T20:52:38Z

wasm-runtime/polkadot/src/support/function.rs

 			x if x == Function::StakingTransfer as u8 => Some(Function::StakingTransfer),
 			x if x == Function::SessionSetKey as u8 => Some(Function::SessionSetKey),
 			x if x == Function::TimestampSet as u8 => Some(Function::TimestampSet),
+			x if x == Function::GovernancePropose as u8 => Some(Function::GovernancePropose),


Why not

let functions = [Function::StakingStake, ...]; if value as usize < functions.len() { Some(functions[value as usize]); } else { None }

doesn't it need #[repr(u8)] anyway for that to be safe?

yes (done in latest PR)

tomusdrw · 2018-01-27T20:56:18Z

wasm-runtime/polkadot/src/support/storable.rs

 	/// Lookup `Some` value in storage and deserialise; `None` if it's not there.
-	fn lookup(_key: &[u8]) -> Option<Self> where Self: Sized { unimplemented!() }
+	fn lookup(_key: &[u8]) -> Option<Self> where Self: Sized {
+		unimplemented!()


👍 We had that in RPC some time ago and it caused some of the RPC methods to be unintentionally unimplemented because of that.

gavofyork · 2018-01-28T16:14:41Z

closed in favour of #47 where all major grumbles have been resolved.

…reate_only_as_root Feature/tokens mint and create only as root

* Add clean consensus * Fix build wasm error * Evaluate block every 1s * Fix bft consensus about time poll and 2s evaluate block

Add CI badges to README

…runtime-upgrade Release/second runtime upgrade

gavofyork added 30 commits January 8, 2018 17:13

Introduce basic skeleton for Polkador runtime.

36c171c

Clean up the runtime skeleton.

f79b7bb

Make initial runtime skeleton compile.

b48e053

Compile polkadot-runtime both for Wasm ad native, allowing for testin…

4e25b24

…g and direct usage.

More fleshing out on runtime.

b28402c

Update native support.

bac50a4

Fix warning.

9a4360f

Update gitignore

0890e72

Update path.

4cf5fd1

Fix path.

12b15fd

Remove accidentally committed files.

47260c2

Add wasm binaries.

800eb20

Fix test.

10eaefe

Native storage support API.

24ba809

Add environmental module

36a49ff

Add native environment to make native source-code compatible with wasm.

50e7222

Also tests.

Finish up & polish environment stuff.

3e17caa

Avoid using reentrancy issues.

e807c9a

Add some docs and a test.

402ae29

Remove unneeded function.

762826b

Documentation

7e969c8

Tweak docs

64b0670

Remove TODOs.

d34f62a

Balance transfers + util methods.

89f35e3

Rejig tests and ensure authorities are addressed consistently.

5e42240

Add marshaller for xfer function

d97e2d6

Transaction dispatch test.

26ccc14

Minor fix.

7258e1f

Add test for ser/de transaction.

c02e335

Add ser/de for header.

28fa761

gavofyork added 5 commits January 23, 2018 14:43

Merge remote-tracking branch 'origin/master' into governance

817902d

Add the missing internal functions.

ac9d4ae

Merge remote-tracking branch 'origin/master' into governance

9ea6011

Merge branch 'master' into governance

eb31ef6

Merge branch 'master' into governance

694204a

gavofyork mentioned this pull request Jan 23, 2018

Native executor #47

Merged

1 task

gavofyork added 2 commits January 23, 2018 20:52

Merge branch 'master' into governance

1073332

Ensure bool actually is endian-insensitive.

f27c8eb

rphmeier reviewed Jan 26, 2018

View reviewed changes

rphmeier reviewed Jan 27, 2018

View reviewed changes

rphmeier suggested changes Jan 27, 2018

View reviewed changes

tomusdrw reviewed Jan 27, 2018

View reviewed changes

gavofyork closed this Jan 28, 2018

gavofyork deleted the governance branch January 30, 2018 09:58

iStrike7 referenced this pull request in gasp-xyz/substrate Aug 3, 2021

Merge pull request #42 from mangata-finance/feature/tokens_mint_and_c…

e45bab9

…reate_only_as_root Feature/tokens mint and create only as root

liuchengxu referenced this pull request in autonomys/substrate Jun 3, 2022

Merge pull request #42 from liuchengxu/add-ci-badge

7b79bed

Add CI badges to README

imstar15 pushed a commit to imstar15/substrate that referenced this pull request Jan 3, 2023

Merge pull request paritytech#42 from mangata-finance/release/second-…

c0fb07c

…runtime-upgrade Release/second runtime upgrade

helin6 pushed a commit to boolnetwork/substrate that referenced this pull request Jul 25, 2023

update version 0.2 -> 0.3 (paritytech#42)

40a8ed3

Conversation

gavofyork commented Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier Jan 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavofyork commented Jan 21, 2018 •

edited

Loading

rphmeier Jan 26, 2018 •

edited

Loading

rphmeier Jan 27, 2018 •

edited

Loading

rphmeier left a comment •

edited

Loading