[Minotaur] Refactoring of Ariadne towards handing ETH stake #67

aslesarenko · 2024-08-30T15:54:00Z

Description

In this PR:

refactoring necessary to add ETH stake registrations into virtual stake computation and computing combined list of valid trustless candidates from both Cardano and Ethereum.
No tests (and test vectors) has been changed (other than adapted to new data structures, and added default values).
All tests pass
Main goal is to extend DParameter, CandidateRegistrations, RegisteredData so that select_authorities have all the necessary information. All the other changes are motivated by this.
added // TODO ETH: ... in places where further work is needed to hangle stake from Ethereum
no changes in db, follower, inherents, storage etc.

Checklist

Commit sequence broadly makes sense and commits have useful messages.
New tests are added if needed and existing tests are updated.
Relevant logging and metrics added
CI passes. See note on CI.
Any changes are noted in the changelog.md for affected crate
Self-reviewed the diff

…validation of registration signatures

LGLO · 2024-09-02T06:35:12Z

primitives/domain/src/lib.rs

@@ -187,11 +217,19 @@ pub const MAX_ASSET_NAME_LEN: u32 = 32;
 pub struct AssetName(pub BoundedVec<u8, ConstU32<MAX_ASSET_NAME_LEN>>);

 const MAINCHAIN_PUBLIC_KEY_LEN: usize = 32;
+const ETH_PUBLIC_KEY_LEN: usize = 32; // TODO ETH: is this correct length?


Yes and no.
Story: historically this project started as Rust version of a Ethereum client that was able to observe Cardano. From this you can see SidechainPubKey. It is 33 bytes, because in this project we have always used the parity bit (hence all keys in hex start with 02 or 03). It is another complication for Minotaur, how to deal with existence of these keys in the project (which are user facing) and not make it very complicated for the user. Should already existing keys be used to get ETH stake address?

From Minotaur point of view any key is an opaque type with some trait(s) as constraints (e.g. Key: MainchainKey).

From user point of view, I would prefer to have clear distinction between Cardano, Ethereum and PC keys/addresses so that it is clear where each type of key should be used.

For example, if I want to run PC node and restake both my ADA and ETH, then I already have and understand the difference between Cardano and Ethereum keys/addresses. Then it is very naturally for me to learn about PC keys/addresses, how they look like and how to create and use them.

If SidechainPubKey has Ethereum legacy, this is probably has to be fixed sooner or later as technical debt.

I agree with fixing technical debt. I also do not want to add new one. I only try to raise awareness that:

ECDSA public key can have different lengths

For the "same lenght" one can one have version with and without a parity bit

The parity bit is written as extra byte

There are many ways of encoding a bit in a byte

This project already has one ECDSA public key in use. This public key is 33 bytes long. 32 bytes for a key and 1 byte that encodes parity bit as either 02 or 03.

This whole message is not trying me to forbid adding 32 bytes ECDSA public key w/o parity bit, I'm only saying that it will probably be confusing (added to already confusing Substrate layers over cryptography).

Ah, that is important context which I didn't consider at all here.
Then we can make it 33 bytes or even define it as

pub struct EthPublicKey(pub Vec<u8>);

From Minotaur's perspective it is not important at all, so please let me know how it should be defined and I will change it.

LGLO · 2024-09-02T06:37:23Z

mainchain-follower/db-sync-follower/src/candidates/mod.rs

+			.get_registered_candidates(epoch, committee_candidate_address)
+			.await?;
+
+		// TODO ETH: get registered candidates for ETH


D-param lives on Cardano, Ada candidates also live on Cardano. Cardano = Mainchain. We can rename Mainchain to Cardano (I still doubt we should), but we cannot have two Mainchains for sure. My point is that we perhaps should not get ETH candidates from Cardano (Mainnet)

Yeah, somewhat misleading description, fixed. It make sense to continue calling Cardano as Mainchain since its role is special for governance. On the other hand, from virtual stake calculation point of view Cardano's role is symmetric to Ethereum.

👍 My understanding is: for Minotaur there will be many sources of stake that are equal. But Cardano is the source of DParam. Is it correct?

Yes, besides ETH, there are talks to add PC's native token. But governance will be done on Cardano, so it is to some degree the "main" chain.

we will need to consider cases where other chains are the "main chain" as well as where the main chain is the partner chain itself. not sure it needs to be concerned with right now.

LGLO · 2024-09-02T06:39:52Z

primitives/domain/src/lib.rs

@@ -460,7 +518,7 @@ pub struct CommitteeHash(pub Vec<u8>);
 /// Note: A Registration Transaction is called by a user on Cardano to register themselves as a Sidechain Authority Candidate
 #[derive(Debug, Clone, Encode, Decode, PartialEq, Eq, TypeInfo)]
 #[cfg_attr(feature = "serde", derive(Serialize))]
-pub struct RegistrationData {
+pub struct AdaRegistrationData {


If its on Cardano, then CardanoRegistrationData. Ada is only a token.

I assumed this is only related to ADA staking. In the case we want to add Cardano's native tokens into Minotaur, will they have the same registration data?

imo CardanoRegistrationData should work for both ada and native tokens, while AdaRegistrationData is only for ada

LGLO · 2024-09-02T06:47:19Z

mainchain-follower/db-sync-follower/src/candidates/mod.rs

@@ -313,14 +330,15 @@ impl CandidatesDataSourceImpl {
 				Some((IntegerDatum(p), IntegerDatum(t))) => {
 					p.to_u16().zip(t.to_u16()).map(|(p, t)| DParameter {
 						num_permissioned_candidates: p,
-						num_registered_candidates: t,
+						num_ada_candidates: t,
+						num_eth_candidates: 0,


Could this decoder handle situations when DParam on Cardano has 3 values and 2 values? Only in the latter case it would use 0 as default?

Yes, if we generalize a bit and declare it like this:

pub struct DParameter { pub num_permissioned_candidates: u16, pub num_ada_candidates: u16, pub num_other_candidates: Vec<u16>, // one item for each chain }

This is more universal encoding, so both empty vector and a vector with single element can be supported. It will also support for example adding PC native token to the virtual stake.

just curious, will there be a new candidate selection algorithm?

(also if this new structure remains we should consider renaming it)

Yes, Minotaur "Mini", to replace Ariadne. Currently I assume that if num_other_candidates is empty, then it should behave as Ariadne. It may change in future, though.

(also if this new structure remains we should consider renaming it)

We need a good name, please suggest.
People got used to DParameter and what it means, and the meaning will stay the same, maybe somewhat generalized.

do we want a simple map here <permission_type, num>? could add enums for permissioned, ada, eth and also combine if we need to. when we add another token...we need to come back and add here which is not ideal.

IMO (u16, u16, vec) doesn't help.
After all it could be a Vec only or fields only and there has to be connection between types of candidates and parts of D-Parameter. Please bare in mind that this part of code is reading data from Datum and this can be affected by how the data is written.

For the decision Vec vs Map vs named fields should be driven by shape of data and what are we building: a generalization to N types of candidates or generalization to 3 candidates types

mpskowron

Is there any document with requirements/design/etc. or some jira task on which those changes are based?

aslesarenko · 2024-09-02T09:49:59Z

@mpskowron, yes, replied in Slack.

vvviiimmm · 2024-09-02T11:31:08Z

primitives/authority-selection-inherents/src/authority_selection_inputs.rs

+	// Thus:
+	// - only one CandidateRegistrations per each candidate
+	// - each registered_candidates[i]:
+	//   - should contain valid mainchain_pub_key


can you please clarify which mainchain_pub_key, ethereum? If so, do we need optional eth_pub_key as suggested bellow?

clarified in the code comments.

so in case of a valid eth candidate mainchain_pub_key == Some(eth_pub_key)?

No, mainchain_pub_key is on Cardano, and eth_pub_key is on Ethereum, they can never be the same. The eth_pub_key field is optional, because registered_candidates vector should have only one item for each PC operator identified by sidechain_public_key, in other words, registered_candidates should have been Map<SidechainPublicKey, CandidateRegistrations>.

The reason I didn't introduced this map, is because I didn't want to deal with serialisation, but we can assume there is such mapping next to registered_candidates.

vvviiimmm · 2024-09-02T11:32:33Z

primitives/domain/src/lib.rs

+
+	// TODO ETH: u64 if not enough to count stake on Ethereum in Wei
+	/// Amount of Wei (which is a fraction of 1 ETH) staked/locked on Ethereum
+	pub eth: StakeAmount,


for later: better to keep these as separate types

Introduced EthStakeAmount type

LGLO · 2024-09-02T11:37:14Z

primitives/authority-selection-inherents/src/authority_selection_inputs.rs

 	pub epoch_nonce: EpochNonce,
+
+	// TODO ETH: add nonce from Ethereum chain


I think that either we target 3 sources and hardcode it in this way, so we have permissioned_candidates, registered_cardano_candidates, registered_ethereum_candidates or this code should be generic for any number of sources, where each source has its "score" and each candidate inside this candidates source has it's score relative to other candidates of this type.

Said that I don't see Minotaur algorithm and I can only guess who it is supposed to be generalization of Ariadne.

I'm strongly for generalization here. I don't think that this part of the code has to know anything about the name of the source of candidates.

The goal of this PR is to make a first step towards multi-resource committee selection, so it not just about 3 types of resources, but it is hard to refactor in one go.
However, that is the plan, so before I can do further generalisations, I would like to finalise this PR and resolve all the questions with just one additional ETH resource.

Minotaur's virtual stake algorithm, in its first approximation is prototyped here https://github.com/input-output-hk/innovation-minotaur/blob/e89ee5d08fe4c7e0c3f2b62fd2252bab4471005b/minotaur-design-sketch/minotaur_lib/src/consensus.rs#L27

It already assumes a general case of a list of resources, which is relatively easy to do in isolation, but will be hard to put into PC code as-is.

@aslesarenko lets chat more about this one. given we now have 2 different tokens, makes sense to make generic here.

mpskowron · 2024-09-02T14:51:33Z

primitives/authority-selection-inherents/src/authority_selection_inputs.rs

 	pub epoch_nonce: EpochNonce,
+
+	// TODO ETH: add nonce from Ethereum chain


I'm strongly for generalization here. I don't think that this part of the code has to know anything about the name of the source of candidates.

mpskowron · 2024-09-02T14:54:54Z

mainchain-follower/db-sync-follower/src/candidates/mod.rs

+							.into_iter()
+							.map(Self::make_registration_data)
+							.collect(),
+						eth_registrations: vec![]  // TODO ETH: add ETH registrations


Eth registrations will be observed on Ethereum, won't they? If so, I'd rather have a separate module for following Ethereum. This way, the code will be more modular and it will be easier for the users to pick up only the parts they are interested in.

Yes, observed on Ethereum, but I would suggest to have 2 levels of data sources:

level 1: data source which is passed everywhere (which is currently CandidateDataSource)

level 2: one resource-specific data source for each resource type (CardanoDS, EthereumDS, etc.)

Thus defined CandidateDataSource will aggregate information from different sources.
From this POV, the next step would be to define a ResourceDS trait and move the Cardano-specific code there from CandidateDataSource.

What you wrote looks fine. However, I would suggest a different approach on getting there. Since this code already is a CardanoDS, instead of modifying it, create a separate module (or even a simple function in authority-selection-inherents for now) which will glue the results of CardanoDS and EthereumDS. This way the diff will be easier to read (especially of the next PR) and the change will be done in less invasive way.

aslesarenko · 2024-09-02T16:01:55Z

@mpskowron, to your question here, further generalisation is planned for the next PR (see my message above)

mpskowron

It's ok that we don't have it done in a generic way at the moment (maybe it's even better, cause I'm not sure if we have enough data to create a good abstraction). However, I think that it would be a good idea to have Eth related code separated more clearly from Ada part (for example, it is tightly coupled together in filter_invalid_candidates now). Then, we can have thin layers of "glue code" on top. I think that this will end up in cleaner and more readable code. This will also make future refactorings, like adding new sources or making generalizations much easier.

mpskowron · 2024-09-03T08:27:42Z

mainchain-follower/db-sync-follower/src/candidates/mod.rs

+							.into_iter()
+							.map(Self::make_registration_data)
+							.collect(),
+						eth_registrations: vec![]  // TODO ETH: add ETH registrations


What you wrote looks fine. However, I would suggest a different approach on getting there. Since this code already is a CardanoDS, instead of modifying it, create a separate module (or even a simple function in authority-selection-inherents for now) which will glue the results of CardanoDS and EthereumDS. This way the diff will be easier to read (especially of the next PR) and the change will be done in less invasive way.

AmbientTea · 2024-10-22T15:16:06Z

There's not been any activity on this PR for over a month. Is it still relevant?

aslesarenko added 6 commits August 30, 2024 16:22

minotaur: ada, eth in DParameter

3468670

minotaur: RegistrationData to keep both ADA and ETH registrations

8530b78

minotaur: unified representation of CandidateRegistrationEntry

6204256

minotaur: all tests are fixed

0451cea

minotaur: CandidateType introduced, multichain StakeDelegation

fcc1c76

minotaur: add eth_pub_key to CandidateRegistrations and pass down to …

1e5dbfe

…validation of registration signatures

aslesarenko marked this pull request as draft August 30, 2024 15:54

minotaur: fixed typos + more // TODO ETH

533ffca

LGLO reviewed Sep 2, 2024

View reviewed changes

mpskowron reviewed Sep 2, 2024

View reviewed changes

minotaur: addressed review comments

64257de

vvviiimmm reviewed Sep 2, 2024

View reviewed changes

LGLO reviewed Sep 2, 2024

View reviewed changes

minotaur: renamed AdaRegistrationData -> CardanoRegistrationData

ab53d3e

mpskowron reviewed Sep 2, 2024

View reviewed changes

minotaur: stake types introduced

6722236

mpskowron reviewed Sep 3, 2024

View reviewed changes

		pub epoch_nonce: EpochNonce,

		// TODO ETH: add nonce from Ethereum chain

[Minotaur] Refactoring of Ariadne towards handing ETH stake #67

Are you sure you want to change the base?

[Minotaur] Refactoring of Ariadne towards handing ETH stake #67

Conversation

aslesarenko commented Aug 30, 2024 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

aslesarenko Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpskowron left a comment

Choose a reason for hiding this comment

aslesarenko commented Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aslesarenko commented Sep 2, 2024

mpskowron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmbientTea commented Oct 22, 2024

aslesarenko commented Aug 30, 2024 •

edited

Loading

aslesarenko Sep 2, 2024 •

edited

Loading