refactor!: don't send public key with signature #4518

mversic · 2024-04-26T13:50:18Z

Description

Linked issue

Closes #4393

Benefits

Checklist

I've read CONTRIBUTING.md
I've used the standard signed-off commit format (or will squash just before merging)
All applicable CI checks pass (or I promised to make them pass later)
(optional) I've written unit tests for the code changes
I replied to all comments after code review, marking all implemented changes with thumbs up

data_model/src/block.rs

core/src/sumeragi/network_topology.rs

core/src/block.rs

mversic · 2024-05-22T06:54:20Z

core/src/sumeragi/view_change.rs

+            self.signatures
+                .iter()
+                .try_fold(IndexSet::new(), |mut acc, elem| {
+                    if !acc.insert(elem) {
+                        return Err("Duplicate signature in proof");
+                    }


I'm not sure whether proofs are duplicated if peer is repeatedly requesting view change?

@Erigara @SamHSmith

Since you've changed type from set to vector singnatures now can duplicate.

You can look at merge_signatures function.

Signed-off-by: Marin Veršić <marin.versic101@gmail.com>

client/src/config/user/boilerplate.rs

Erigara · 2024-05-23T06:59:47Z

core/src/smartcontracts/isi/query.rs

-        let topology = Topology::new(UniqueVec::new());
+        let (peer_public_key, _) = KeyPair::random().into_parts();
+        let peer_id = PeerId::new("127.0.0.1:8080".parse().unwrap(), peer_public_key);
+        let topology = Topology::new(vec![peer_id]);


You've changed topology to have at least one element?

If so what happens when last peer is unregistered in Unregister<Peer>?

I didn't think about that though. I didn't strictly impose the limit that there must be 1 peer in the topology, rather it's just that new topology has to be created with 1 peer at least. Do you agree with this change? Do you think we should impose a limit to prevent topology from going to 0?

Do you think we should impose a limit to prevent topology from going to 0?

Right now we would panic in Topology::new if topology is empty.
But it's possible to get empty topology through block_committed call afaik.

As well calls to leader and proxy_tail would panic in case topology is empty.

So i think we should bring consistency.
Essentially empty topology is possible but it's the end of the blockchain since no new blocks would be produced ever.

Found ticket #3522 created by me exactly 1 year ago lol

Erigara · 2024-05-23T07:13:46Z

core/src/block.rs

+                    if additional_leader_signatures.next().is_some() {
+                        return Err(SignatureVerificationError::DuplicateSignatures {
+                            signatory: leader_index,
+                        });
+                    }


I have concern about this check, can't malicious peer insert wrong index and valid signature (for random key pair)?
So that it looks like leader submitted multiple signatures.

This way malicous peer can't prevent safety of blockchain, but can violate liveliness.

Maybe we can require that at least one signature should be from leader?

malicious peer can send all sorts of invalid blocks. What you're saying is true, but is no less dangerous than sending other types of invalid blocks. We just reject a block sent from this peer, there is no harm in that IMO

I think what @Erigara is saying is that the block validation will globally and repeatedly fail unless it has a mechanism to identify and ignore (or expel from the validator set) the peer that spoof index zero with its signature.
The problem here would be trusting the index without checking it against the signature

I think what @Erigara is saying is that the block validation will globally and repeatedly fail unless it has a mechanism to identify and ignore (or expel from the validator set) the peer that spoof index zero with its signature.

spoofing 0 index is just one way in which malicious peer can affect consensus. Worst case, malicious peer can withhold sending any messages to other peers. It will only fail consensus repeatedly until the malicious peer becomes observing peer. 0 index spoofing is no special case and I don't think we should be more forgiving in this case

Ok, at least this is not a problem specific to this PR. It used to trust public keys instead of indices

Erigara · 2024-05-23T07:14:18Z

core/src/block.rs

+            //let roles: &[Role] = if topology.view_change_index() >= 1 {
+            //    &[Role::ValidatingPeer, Role::ObservingPeer]
+            //} else {
+            //    if topology
+            //        .filter_signatures_by_roles(&[Role::ObservingPeer], block.signatures())
+            //        .next()
+            //        .is_some()
+            //    {
+            //        return Err(SignatureVerificationError::UnknownSignatory);
+            //    }
+
+            //    &[Role::ValidatingPeer]
+            //};


Commented code.

core/src/tx.rs

core/src/sumeragi/network_topology.rs

Erigara · 2024-05-23T12:13:58Z

core/src/sumeragi/view_change.rs

+            self.signatures
+                .iter()
+                .try_fold(IndexSet::new(), |mut acc, elem| {
+                    if !acc.insert(elem) {
+                        return Err("Duplicate signature in proof");
+                    }


Since you've changed type from set to vector singnatures now can duplicate.

You can look at merge_signatures function.

Erigara · 2024-05-23T12:27:31Z

data_model/src/query/mod.rs

+            Serialize,
+            IntoSchema,
+        )]
+        pub struct QuerySignature(pub PublicKey, pub SignatureOf<ClientQueryPayload>);


After Sato's work pub key can be extracted from AccoundId.

data_model/src/block.rs

mversic · 2024-05-23T18:50:15Z

core/src/sumeragi/main_loop.rs

+                // FIX: Release writer before handling block sync
+                let curr_block = voting_block.take();


I have this issue in 4 places. It happens either on BlockSyncUpdate or BlockCreated. At the time of receiving a new block it's possible that the block is invalid and I would like to keep the old block while validating the new one. However, this creates deadlock as I have not released previous writer

@Erigara do you have some suggestions?

Unfortunately it’s current limitation of our state layout.
That it’s single writer and locks state for duration of block.

When i’ve designed state i made some decisions for making optimisations for happy path (block committed) without problems.
We might reconsider some of this decisions if necessary.

Maybe we can be smarter about when to reject incoming block? Like checking signatures, height, hashes this checks doesn’t need ability to modify state.

well, I'll just drop the current voting block in this PR as it was before and open an issue for this

Erigara · 2024-05-24T06:14:44Z

core/src/sumeragi/main_loop.rs

+            (BlockMessage::BlockSigned(BlockSigned { signatures }), Role::ProxyTail) => {
+                info!(
+                    peer_id=%self.peer_id,
+                    role=%self.role(),
+                    "Received block signatures"
+                );

-                let roles: &[Role] = if current_view_change_index >= 1 {
+                let roles: &[Role] = if view_change_index >= 1 {
                    &[Role::ValidatingPeer, Role::ObservingPeer]
                } else {
                    &[Role::ValidatingPeer]
                };
-                let valid_signatures =
-                    current_topology.filter_signatures_by_roles(roles, &signatures);
-
-                if let Some(voted_block) = voting_block.as_mut() {
-                    let voting_block_hash = voted_block.block.as_ref().hash_of_payload();
-
-                    if hash == voting_block_hash {
-                        add_signatures::<true>(voted_block, valid_signatures);
-                    } else {
-                        debug!(%voting_block_hash, "Received signatures are not for the current block");
-                    }
+                let valid_signatures = self
+                    .topology
+                    .filter_signatures_by_roles(roles, &signatures)
+                    .cloned();
+
+                if let Some(mut voted_block) = voting_block.take() {
+                    add_signatures::<true>(&mut voted_block, valid_signatures, &self.topology);
+                    *voting_block = self.try_commit_block(voted_block, is_genesis_peer);
                } else {
                    // NOTE: Due to the nature of distributed systems, signatures can sometimes be received before
                    // the block (sent by the leader). Collect the signatures and wait for the block to be received
                    voting_signatures.extend(valid_signatures);
                }
            }


By looking at this code and try_commit it look like we don't check that signature indeed belong to the peer.
So any peer (including malicious leader itself) can send BlockSigned message with random block matching signatures for enough validating/observing peers and block will be committed.

Another problem is that on this path signatures are not checked for duplication.
So as before peer can submit BlockSigned message with the same index and signature and block will be committed.

Erigara · 2024-05-24T06:25:04Z

core/src/sumeragi/main_loop.rs

+                .block
+                .commit(&self.topology)
+                .unpack(|e| self.send_event(e))
+                .expect("INTERNAL BUG: Proxy tail failed to commit block");
+


Maybe instead of unwrapping we can try to commit the block in the if statement?
I think it would be more reliable in long term if we decide that to commit block it's not enough to just receive enough signatures.
And proxy tail can add it's signature on BlockCreated message.

Erigara · 2024-05-24T06:25:57Z

core/src/block.rs

+                if let Err(err) = Self::verify_proxy_tail_signature(self.as_ref(), topology) {
                    return WithEvents::new(Err((self, err.into())));
                }


We probably should check that whole set of sigantures is valid.

ValidBlock is guaranteed to have all signatures valid except ProxyTail

Signed-off-by: Marin Veršić <marin.versic101@gmail.com>

mversic requested review from s8sato, SamHSmith, outoftardis, Erigara, 0x009922, DCNick3, VAmuzing, Asem-Abdelhady and dima74 as code owners April 26, 2024 13:50

github-actions bot added iroha2-dev The re-implementation of a BFT hyperledger in RUST Refactor Improvement to overall code quality labels Apr 26, 2024

mversic force-pushed the remove_public_key branch 2 times, most recently from cee8760 to df420c1 Compare April 26, 2024 14:02

github-actions bot added the api-changes Changes in the API for client libraries label Apr 26, 2024

mversic force-pushed the remove_public_key branch from df420c1 to 120c0df Compare April 26, 2024 14:21

Erigara assigned mversic and Erigara Apr 26, 2024

mversic force-pushed the remove_public_key branch from 120c0df to f66a58f Compare April 27, 2024 08:00

mversic changed the title ~~[refactor] #4393: don't send public key with signature~~ (refactor): don't send public key with signature May 1, 2024

mversic changed the title ~~(refactor): don't send public key with signature~~ refactor!: don't send public key with signature May 1, 2024

mversic force-pushed the remove_public_key branch 2 times, most recently from dc4d7d7 to 3701fc5 Compare May 2, 2024 07:52

mversic removed the iroha2-dev The re-implementation of a BFT hyperledger in RUST label May 2, 2024

mversic force-pushed the remove_public_key branch from 3701fc5 to ecb3bce Compare May 2, 2024 08:33

Erigara requested changes May 2, 2024

View reviewed changes

data_model/src/block.rs Outdated Show resolved Hide resolved

data_model/src/block.rs Outdated Show resolved Hide resolved

core/src/sumeragi/network_topology.rs Outdated Show resolved Hide resolved

core/src/block.rs Show resolved Hide resolved

mversic force-pushed the remove_public_key branch 5 times, most recently from bdb903e to fb852e9 Compare May 8, 2024 10:42

mversic requested review from s8sato and Erigara May 17, 2024 13:24

mversic force-pushed the remove_public_key branch 7 times, most recently from 050891b to 51eff2c Compare May 21, 2024 12:07

s8sato previously approved these changes May 22, 2024

View reviewed changes

mversic dismissed s8sato’s stale review via d1c63f2 May 22, 2024 06:48

mversic force-pushed the remove_public_key branch 2 times, most recently from d1c63f2 to 9edd012 Compare May 22, 2024 06:50

mversic commented May 22, 2024

View reviewed changes

refactor!: don't send public key with signature

79e3adf

Signed-off-by: Marin Veršić <marin.versic101@gmail.com>

Erigara requested changes May 23, 2024

View reviewed changes

Erigara mentioned this pull request May 23, 2024

How block hash should be derived? #4641

Open

mversic force-pushed the remove_public_key branch 6 times, most recently from 211e87a to ad1aa08 Compare May 23, 2024 18:46

mversic commented May 23, 2024

View reviewed changes

mversic force-pushed the remove_public_key branch from ad1aa08 to 1479b74 Compare May 23, 2024 19:52

mversic mentioned this pull request May 24, 2024

Keep voting blocks #4643

Open

Erigara reviewed May 24, 2024

View reviewed changes

mversic force-pushed the remove_public_key branch from 1479b74 to 4e1cf76 Compare May 24, 2024 14:10

fix: view change updates

5b5f2cb

Signed-off-by: Marin Veršić <marin.versic101@gmail.com>

mversic force-pushed the remove_public_key branch from 4e1cf76 to 5b5f2cb Compare May 24, 2024 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: don't send public key with signature #4518

refactor!: don't send public key with signature #4518

mversic commented Apr 26, 2024 •

edited

mversic May 22, 2024

Erigara May 23, 2024

Erigara May 23, 2024

mversic May 23, 2024

Erigara May 23, 2024 •

edited

Erigara May 23, 2024 •

edited

Erigara May 23, 2024

mversic May 23, 2024

s8sato May 24, 2024

mversic May 24, 2024

s8sato May 24, 2024

Erigara May 23, 2024

Erigara May 23, 2024

Erigara May 23, 2024

mversic May 23, 2024

Erigara May 23, 2024

mversic May 23, 2024

Erigara May 24, 2024 •

edited

Erigara May 24, 2024

Erigara May 24, 2024

mversic May 24, 2024 •

edited

		// FIX: Release writer before handling block sync
		let curr_block = voting_block.take();

refactor!: don't send public key with signature #4518

Are you sure you want to change the base?

refactor!: don't send public key with signature #4518

Conversation

mversic commented Apr 26, 2024 • edited

Description

Linked issue

Benefits

Checklist

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

Erigara May 23, 2024 • edited

Choose a reason for hiding this comment

Erigara May 23, 2024 • edited

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

Erigara May 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mversic May 24, 2024 • edited

Choose a reason for hiding this comment

mversic commented Apr 26, 2024 •

edited

Erigara May 23, 2024 •

edited

Erigara May 23, 2024 •

edited

Erigara May 24, 2024 •

edited

mversic May 24, 2024 •

edited