Initial validator sets pallet #187
Conversation
Since we aren't reusing keys across coins, there's no reason for it to be on-chain (as previously planned).
Ensures an even distribution of keys. While xxhash is breakable, these keys aren't manipulatable by users.
Also removes the randomness pallet which was only required by the contracts runtime. Does not remove the contracts folder yet so they can still be referred to while validator-sets is under development. Does remove them from Cargo.toml.
|
This needs tests and a further issue for session management (comprehensive to here, staking, and tendermint) made before merging can be considered. I'm requesting review now so we can also discuss what else to merge this, primarily as a multisig tracker, for protonet. |
| | DAI | Ethereum | 3 | | ||
| | Monero | Monero | 4 | | ||
| | Ether | Ethereum | 1 | | ||
| | DAI | Ethereum | 2 | |
There was a problem hiding this comment.
It may be work taking the ID to a string so that we can distinguish different chains within the ID as well, or we could also sign a separate chain ID.
There was a problem hiding this comment.
An even better thing would be to generate a random GUID and form consensus around this GUID per coin/token.
There was a problem hiding this comment.
Perhaps CAIP-19 specification can be used: https://github.com/ChainAgnostic/CAIPs/blob/master/CAIPs/caip-19.md. It describes how to identify assets in multi-chain applications.
There was a problem hiding this comment.
Absolute NACK around GUIDs. Alternatively to an integer, a string could be used. My objections are the inherent inefficiencies and the fact it's a protocol acknowledgement of what the coins are, rather than just another coin.
These inefficiencies aren't just minute. Bitcoin instructions are almost at the OP_RET size limit as-is. If integrators still are forced to a number scheme, I don't care to try to offer a nicety sporadically. I'd rather just have our lib define the constants as expected. And then for a native Substrate TX, I think this would bloat them 10%? As it adds ~12 bytes to each TX.
Alternatively, we can have a numeric ID for reference AND an on-chain string label, which is called upon request, not causing any inefficiencies. This on-chain label would be explicitly formatted so clients could get asset metadata without issue. CAIP-19 seems generally competent (URI scheme, chain ID, class, ID). My gripes are reuse of : (instead of a further /) and usage of SLIP-44 instead of a magic like self for the base.
So I do think the best move is an additional metadata section. My immediate question is should that be here, a new pallet entirely, and do we use CAIP-19 as-is, or a more LibP2P/Substrate esque eip155:1/native, eip155:1/erc20:0x6b175474e89094c44da98b954eedeac495271d0f...
There was a problem hiding this comment.
Note that Monero currently doesn't have a dedicated chain ID in CAIP system.
I've recently created an issue for that: ChainAgnostic/namespaces#41 (feedback is appreciated!).
There was a problem hiding this comment.
I prefer to use string to integers from a security perspective. As far as CAIP goes, the effort is indeed appreciated but I generally feel we're a bit early in the cross-chain pursuit to define fully backwards compatible standards that will account for the needs of various environments. I believe at one point we discussed defining our own steganography towards efficiency of data included on other chains. I'm a bit of an optimization maximalist and hope we can even be compatible if chains more restrictive of metadata arise.
Maybe we should keep this a simplistic structure for protonet, keep CAIP in mind for testnet+, and focus this discussion on implementing a distinction between tokens and native assets?
| Some(ValidatorSet { | ||
| bond: self.bond, | ||
| coins: BoundedVec::try_from(coins).unwrap(), | ||
| participants: BoundedVec::try_from(participants).unwrap(), |
There was a problem hiding this comment.
This adds immutability to participants?
There was a problem hiding this comment.
This is indexed by instance. It's intended to be immutable. Nothing here forces it to be though.
There was a problem hiding this comment.
Seems low-hanging and a good pattern to enforce immutability past this point yes? Once the set is compiled here it will not need to change unless a new session forms, yes?
There was a problem hiding this comment.
We can't force it to be immutable. All we can do is never write to it again. We can, however, introduce niceties for that, such as a struct we'd have to explicitly bypass. I'd be willing to add that in BUT we may not want such explicitly intended immutability. I'm unsure how removal due to slashes works under Parity's session pallet/how we'll end up handling it.
Move types such as Amount/Coin out of validator-sets. Will be expanded in the future.
commit e0a9e8825d6c22c797fb84e26ed6ef10136ca9c2 Author: Luke Parker <[email protected]> Date: Fri Jan 6 04:24:08 2023 -0500 Remove Scanner::address It either needed to return an Option, panic on misconfiguration, or return a distinct Scanner type based on burning bug immunity to offer this API properly. Panicking wouldn't be proper, and the Option<Address> would've been... awkward. The new register_subaddress function, maintaining the needed functionality, also provides further clarity on the intended side effect of the previously present Scanner::address function. commit 7359360ab2fc8c9255c6f58250c214252ce217a4 Author: Luke Parker <[email protected]> Date: Fri Jan 6 01:35:02 2023 -0500 fmt/clippy from last commit commit 80d912fc19cd268f3b019a9d9961a48b2c45e828 Author: Luke Parker <[email protected]> Date: Thu Jan 5 19:36:49 2023 -0500 Add Substrate "assets" pallet While over-engineered for our purposes, it's still usable. Also cleans the runtime a bit. commit 2ed2944b6598d75bdc3c995aaf39b717846207de Author: Luke Parker <[email protected]> Date: Wed Jan 4 23:09:58 2023 -0500 Remove the timestamp pallet It was needed for contracts, which has since been removed. We now no longer need it. commit 7fc1fc2dccecebe1d94cb7b4c00f2b5cb271c87b Author: Luke Parker <[email protected]> Date: Wed Jan 4 22:52:41 2023 -0500 Initial validator sets pallet (#187) * Initial work on a Validator Sets pallet * Update Validator Set docs per current discussions * Update validator-sets primitives and storage handling * Add validator set pallets to deny.toml * Remove Curve from primitives Since we aren't reusing keys across coins, there's no reason for it to be on-chain (as previously planned). * Update documentation on Validator Sets * Use Twox64Concat instead of Identity Ensures an even distribution of keys. While xxhash is breakable, these keys aren't manipulatable by users. * Add math ops on Amount and define a coin as 1e8 * Add validator-sets to the runtime and remove contracts Also removes the randomness pallet which was only required by the contracts runtime. Does not remove the contracts folder yet so they can still be referred to while validator-sets is under development. Does remove them from Cargo.toml. * Add vote function to validator-sets * Remove contracts folder * Create an event for the Validator Sets pallet * Remove old contracts crates from deny.toml * Remove line from staking branch * Remove staking from runtime * Correct VS Config in runtime * cargo update * Resolve a few PR comments on terminology * Create a serai-primitives crate Move types such as Amount/Coin out of validator-sets. Will be expanded in the future. * Fixes for last commit * Don't reserve set 0 * Further fixes * Add files meant for last commit * Remove Staking transfer commit 3309295911d22177bd68972d138aea2f8658eb5f Author: Luke Parker <[email protected]> Date: Wed Jan 4 06:17:00 2023 -0500 Reorder coins in README by market cap commit db5d19cad33ccf067d876b7f5b7cca47c228e2fc Author: Luke Parker <[email protected]> Date: Wed Jan 4 06:07:58 2023 -0500 Update README commit 606484d744b1c6cc408382994c77f1def25d3e7d Author: Luke Parker <[email protected]> Date: Wed Jan 4 03:17:36 2023 -0500 cargo update commit 3a319b2 Author: akildemir <[email protected]> Date: Wed Jan 4 16:26:25 2023 +0300 update address public API design commit d9fa88f Author: akildemir <[email protected]> Date: Mon Jan 2 13:35:06 2023 +0300 fix clippy error commit cc722e8 Merge: cafa9b3 eeca440 Author: akildemir <[email protected]> Date: Mon Jan 2 11:39:04 2023 +0300 Merge https://github.com/serai-dex/serai into develop commit cafa9b3 Author: akildemir <[email protected]> Date: Mon Jan 2 11:38:26 2023 +0300 fix build errors commit ce5b5f2 Merge: f502d67 49c4acf Author: akildemir <[email protected]> Date: Sun Jan 1 15:16:25 2023 +0300 Merge https://github.com/serai-dex/serai into develop commit f502d67 Author: akildemir <[email protected]> Date: Thu Dec 22 13:13:09 2022 +0300 fix pr issues commit 26ffb22 Author: akildemir <[email protected]> Date: Thu Dec 22 13:11:43 2022 +0300 remove extraneous rpc call commit 0e829f8 Author: akildemir <[email protected]> Date: Thu Dec 15 13:56:53 2022 +0300 add scan tests commit 5123c7f Author: akildemir <[email protected]> Date: Thu Dec 15 13:56:13 2022 +0300 add new address functions & comments
Only works for a static validator set yet does have multisig voting/tracking.