apollo_propeller: add signature verification module#11053
Conversation
guy-starkware
left a comment
There was a problem hiding this comment.
@guy-starkware reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @noamsp-starkware, @ShahakShama, and @sirandreww-starkware).
crates/apollo_propeller/src/signature.rs line 21 at r1 (raw file):
Ok(signature) => Ok(signature), Err(e) => Err(ShardPublishError::SigningFailed(e.to_string())), }
Maybe map_err with ? at the end is more concise?
Also, I think Shahak mentioned something about transparent errors, may make it possible to just use ?
Code quote:
match keypair.sign(&msg) {
Ok(signature) => Ok(signature),
Err(e) => Err(ShardPublishError::SigningFailed(e.to_string())),
}crates/apollo_propeller/src/signature.rs line 82 at r1 (raw file):
#[cfg(test)] mod tests {
Move tests to different file.
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 1 file and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @guy-starkware, @noamsp-starkware, and @sirandreww-starkware).
crates/apollo_propeller/src/signature.rs line 21 at r1 (raw file):
Previously, guy-starkware wrote…
Maybe map_err with ? at the end is more concise?
Also, I think Shahak mentioned something about transparent errors, may make it possible to just use ?
+1
crates/apollo_propeller/src/signature.rs line 82 at r1 (raw file):
Previously, guy-starkware wrote…
Move tests to different file.
+1
crates/apollo_propeller/src/signature.rs line 11 at r1 (raw file):
pub const SIGNING_PREFIX: &[u8] = b"<apollo-propeller>"; pub const SIGNING_POSTFIX: &[u8] = b"</apollo-propeller>";
Why do you need a postfix? Also, try to make this shorter
Also, these shouldn't hold apollo because other implementations are going to use the same string. If you really want you can write starknet instead of apollo
crates/apollo_propeller/src/signature.rs line 37 at r1 (raw file):
} pub fn try_extract_public_key_from_peer_id(peer_id: &PeerId) -> Option<PublicKey> {
Are you sure this functionality doesn't exist in libp2p
crates/apollo_propeller/src/signature.rs line 42 at r1 (raw file):
// Check if this is an identity multihash (code 0x00) if multihash.code() == 0x00 {
Try to find a constant for 0x00 in libp2p. If it doesn't exist, define your own
crates/apollo_propeller/src/signature.rs line 71 at r1 (raw file):
} else { // This is a hashed PeerId (SHA-256), cannot extract the original key tracing::trace!(peer=%peer_id, multihash_code=%multihash.code(), "PeerId uses hashed multihash, cannot extract public key");
So what is the plan? Ensure all our peer id schemes uses identity?
If that's the case, return error instead of none
crates/apollo_propeller/src/signature.rs line 92 at r1 (raw file):
let merkle_root = MessageRoot([ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24,
Define this as [1; 32] to reduce space. I guess the value doesn't matter
3429e4e to
6cfd5de
Compare
3069acd to
e6dbf88
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 7 comments.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @guy-starkware, @noamsp-starkware, and @ShahakShama).
crates/apollo_propeller/src/signature.rs line 11 at r1 (raw file):
Previously, ShahakShama wrote…
Why do you need a postfix? Also, try to make this shorter
Also, these shouldn't hold apollo because other implementations are going to use the same string. If you really want you can write starknet instead of apollo
I added both just to be safe (same logic that I used for merkle tree hash calculation).
I see your point on apollo an will make this shorter by removing it
crates/apollo_propeller/src/signature.rs line 21 at r1 (raw file):
Previously, ShahakShama wrote…
+1
I did your first suggestion and added a todo to do the second...
crates/apollo_propeller/src/signature.rs line 37 at r1 (raw file):
Previously, ShahakShama wrote…
Are you sure this functionality doesn't exist in libp2p
You are more than right to question this. From my research I did not find a function that does this, and the gossipsub implementation has a very similar function to this one https://github.com/libp2p/rust-libp2p/blob/e10ce4c1704a529161f62e8538e55a106e4ec2d6/protocols/gossipsub/src/protocol.rs#L227C1-L227C27
crates/apollo_propeller/src/signature.rs line 42 at r1 (raw file):
Previously, ShahakShama wrote…
Try to find a constant for 0x00 in libp2p. If it doesn't exist, define your own
I could not find this constant anywhere
crates/apollo_propeller/src/signature.rs line 71 at r1 (raw file):
Previously, ShahakShama wrote…
So what is the plan? Ensure all our peer id schemes uses identity?
If that's the case, return error instead of none
I'm not sure... eliptic curve public keys are short enough so they are put in PeerIDs, so for now all our peer IDs use code 0 because the public key is in the peer id. I also added a function to add a peer with an explicit public key to get around this in the general case (in that case this function will not be called).
I have a feeling that we shouldn't dwell on this for too long because we might need to change the functionality to use starkIDs in the future (to ensure all nodes agree on routing with no possible attacks)
That's the plan for now...
crates/apollo_propeller/src/signature.rs line 82 at r1 (raw file):
Previously, ShahakShama wrote…
+1
Done
crates/apollo_propeller/src/signature.rs line 92 at r1 (raw file):
Previously, ShahakShama wrote…
Define this as [1; 32] to reduce space. I guess the value doesn't matter
Done
guy-starkware
left a comment
There was a problem hiding this comment.
@guy-starkware reviewed 5 files and all commit messages, and resolved 1 discussion.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @noamsp-starkware and @ShahakShama).
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed all commit messages, made 1 comment, and resolved 5 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware and @sirandreww-starkware).
crates/apollo_propeller/src/signature.rs line 11 at r1 (raw file):
Previously, sirandreww-starkware (Andrew Luka) wrote…
I added both just to be safe (same logic that I used for merkle tree hash calculation).
I see your point on apollo an will make this shorter by removing it
Add a TODO to consider removing these as before
6cfd5de to
b3f3c87
Compare
e6dbf88 to
d526a1d
Compare
b3f3c87 to
fc56114
Compare
d526a1d to
1b6e505
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 1 comment.
Reviewable status: 1 of 5 files reviewed, 1 unresolved discussion (waiting on @guy-starkware, @noamsp-starkware, and @ShahakShama).
crates/apollo_propeller/src/signature.rs line 11 at r1 (raw file):
Previously, ShahakShama wrote…
Add a TODO to consider removing these as before
Sure, just stating that the prefix is in gossipsub too...
1b6e505 to
270ea07
Compare
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 4 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware).
270ea07 to
81bf74b
Compare
4033098 to
dfd8ae8
Compare
81bf74b to
2768240
Compare
2768240 to
5e29cdb
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware reviewed 5 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware).

No description provided.