Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(code): Actor-based node implementation #167

Merged
merged 89 commits into from
Apr 3, 2024
Merged

feat(code): Actor-based node implementation #167

merged 89 commits into from
Apr 3, 2024

Conversation

romac
Copy link
Member

@romac romac commented Mar 1, 2024

Closes: #XXX


To start a node, where INDEX is 0 (initial proposer), 1 or 2

$ cargo run --bin malachite-cli -- --index INDEX

Follow-up tasks

  • Rename Node actor to Consensus
  • Add top-level Node
  • Update flow diagram to match ADR and include in ADR
  • Let validator set change between heights
  • Proposal builder must gossip block parts
  • Add init and start commands
  • Add config file
  • Add persistent peers to Gossip
  • Move each actor into its respective crate

PR author checklist

@romac romac mentioned this pull request Mar 26, 2024
2 tasks
code/actors/src/consensus.rs Outdated Show resolved Hide resolved
code/actors/src/consensus.rs Outdated Show resolved Hide resolved
code/driver/src/driver.rs Show resolved Hide resolved
code/proto/src/malachite.proto Show resolved Hide resolved
Copy link
Collaborator

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work @romac!

code/cli/src/main.rs Outdated Show resolved Hide resolved
code/gossip/src/lib.rs Outdated Show resolved Hide resolved
code/gossip/src/lib.rs Outdated Show resolved Hide resolved
code/gossip/src/lib.rs Outdated Show resolved Hide resolved
@ancazamfir
Copy link
Collaborator

It seems that we don't check the vote heights when counting and we mix votes from different heights but same round.

diff --git a/code/driver/src/driver.rs b/code/driver/src/driver.rs
index e4c26ca..779a94b 100644
--- a/code/driver/src/driver.rs
+++ b/code/driver/src/driver.rs
@@ -231,6 +231,10 @@ where
     }

     fn apply_vote(&mut self, vote: Ctx::Vote) -> Result<Option<RoundOutput<Ctx>>, Error<Ctx>> {
+        if self.height() != vote.height() {
+            return Ok(None);
+        }
+
         let validator = self
             .validator_set
             .get_by_address(vote.validator_address())

@ancazamfir
Copy link
Collaborator

I see messages received in a different/ reverse order than sent. It should be fine for consensus messages but was wondering why.
Here is an example:
sender:

2024-03-30T06:47:20.459915Z DEBUG gossip{peer=12D3KooWDAFhzcuGpsGMpyrf61sH4yRmWSvTwVxian2TKE6ixtwQ}: Broadcasted message 313244334b6f6f57444146687a6375477073474d70797266363173483479526d5753765477567869616e32544b4536697874775131373131373831323238313835363039303035 of 154 bytes
2024-03-30T06:47:20.460391Z DEBUG gossip{peer=12D3KooWDAFhzcuGpsGMpyrf61sH4yRmWSvTwVxian2TKE6ixtwQ}: Broadcasted message 313244334b6f6f57444146687a6375477073474d70797266363173483479526d5753765477567869616e32544b4536697874775131373131373831323238313835363039303036 of 137 bytes

receiver:

2024-03-30T06:47:20.482535Z DEBUG gossip{peer=12D3KooWGpoh2tcZ8WdWdYCmDKjnaCGTiJefKFsA5VkyY6t8uJXN}: Received message 313244334b6f6f57444146687a6375477073474d70797266363173483479526d5753765477567869616e32544b4536697874775131373131373831323238313835363039303036 from 12D3KooWDAFhzcuGpsGMpyrf61sH4yRmWSvTwVxian2TKE6ixtwQ on channel /consensus of 137 bytes
2024-03-30T06:47:20.482693Z DEBUG gossip{peer=12D3KooWGpoh2tcZ8WdWdYCmDKjnaCGTiJefKFsA5VkyY6t8uJXN}: Received message 313244334b6f6f57444146687a6375477073474d70797266363173483479526d5753765477567869616e32544b4536697874775131373131373831323238313835363039303035 from 12D3KooWDAFhzcuGpsGMpyrf61sH4yRmWSvTwVxian2TKE6ixtwQ on channel /consensus of 154 bytes

@romac
Copy link
Member Author

romac commented Apr 2, 2024

I see messages received in a different/reverse order than sent. It should be fine for consensus messages but was wondering why.

Not sure. Messages are sent in order through GossipSub, but I don't think that GossipSub provides any message ordering guarantees. Will look more into it.

@romac
Copy link
Member Author

romac commented Apr 3, 2024

It seems that we don't check the vote heights when counting and we mix votes from different heights but same round.

Good catch! I did the same check for proposals as well: 340d6b4

@romac romac marked this pull request as ready for review April 3, 2024 09:03
@ancazamfir
Copy link
Collaborator

Not sure. Messages are sent in order through GossipSub, but I don't think that GossipSub provides any message ordering guarantees. Will look more into it.

Maybe we open a separate issue for this so we can merge this PR?

@romac romac merged commit 0d81bb5 into main Apr 3, 2024
12 of 13 checks passed
@romac romac deleted the romac/node-actor branch April 3, 2024 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants