Set voter to DA attestation #498
Conversation
nomos-da/full-replication/src/lib.rs
Outdated
| @@ -69,6 +71,7 @@ impl<A, C> AbsoluteNumber<A, C> { | |||
|
|
|||
| #[derive(Clone, Debug, Serialize, Deserialize)] | |||
| pub struct Settings { | |||
| pub private_key: [u8; 32], | |||
There was a problem hiding this comment.
Why is this a private key here?
There was a problem hiding this comment.
What I mean is that I am not really sure this is the right move. If we are not signing anything here. I think we are not. Just a public key should be enough. is this used to issue the Voter?
There was a problem hiding this comment.
You're right. I guessed we're gonna make voter sign on the attestation in the future (same as the consensus_engine::NodeId (== private_key)). But, if it's not (or if it's not determined yet), it may better to accept a voter: [u8; 32] (not even public_key).
There was a problem hiding this comment.
I'll change it and ping you again when it's ready.
There was a problem hiding this comment.
@danielSanchezQ Now, we don't involve any "key" concept: 765d5a2. We just accept a voter, which is a [u8; 32], as a parameter. I think this is the simplest approach for now since we don't know yet what we're gonna do with this voter.
tests/src/nodes/nomos.rs
Outdated
| // Using private_key as a voter only for easy deployment. | ||
| // It doesn't mean that private_key will be used for anything like signing | ||
| // in the FullReplication. | ||
| voter: private_key, |
There was a problem hiding this comment.
Can we use the public key instead?
There was a problem hiding this comment.
Good point. This can mislead. But, it's not natural to derive a public key from this private key, because we're not assuming any key type for now.
Actually, this argument name should be id (not private_key) because the caller of this function deals with ids (not private_keys):
nomos-node/tests/src/nodes/nomos.rs
Line 236 in ffc4054
I renamed the argument name: ffc4054.
| @@ -188,7 +193,10 @@ impl DaProtocol for FullReplication<AbsoluteNumber<Attestation, Certificate>> { | |||
| type Settings = Settings; | |||
|
|
|||
| fn new(settings: Self::Settings) -> Self { | |||
| Self::new(AbsoluteNumber::new(settings.num_attestations)) | |||
| Self::new( | |||
There was a problem hiding this comment.
one thing I was thinking is that authentication of the sender is probably orthogonal to the da protocol and we might want to keep them separate. Anyway, for now we don't do any real authentication so this is ok
To avoid the following error from libp2p gossipsub (especially with mixnet), we need to set
votertoAttestation. In other words, all messages should be differentiated at the upper layer than the network layer, because the network layer (libp2p) doesn't publish/receive duplicate messages. For more details, please see discussions in #497.I defined
Voterunder theFullReplicationinstead of making it generic, because it doesn't need to be used by other services, such as mempool or network. But, please let me know if you think it's better to make it generic.