-
Notifications
You must be signed in to change notification settings - Fork 19
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: Upgrade Malachite #184
base: main
Are you sure you want to change the base?
feat: Upgrade Malachite #184
Conversation
thanks for the pr, someone will review it soon |
also, can you comment on the issue so that i can assign it to you @varun-doshi ? |
Were you able to start up 3 nodes locally and see consensus progressing? This will likely not work until we propagate the gossip messages into consensus (i.e. do what we were previously doing for the |
So far this looks reasonable, let me know if it'd be useful to get some input on the remaining |
You'll want to rebase the changes from #187 so that the Docker image builds. Tests are still failing however, so we'll want to make sure those are fixed as well. |
the first command works and creates the config toml files but running any of the nodes give an error like this:
|
Ah, you should update config.toml to use 127.0.0.1 but just different ports. We'll fix the script to do this by default. |
After updating the config toml, looks like the nodes are discovering each other and setting up a conenction, but it crashes when trying to do
|
Awesome, the next step would be to implement the incomplete function and any others that prevent consensus from progressing. |
understood...I'll get on this |
This is what Context for SnapChainValidator looks like right now:
The Also
|
@varun-doshi this issue is actually more complicated that we realized. We'll take your branch over from here and get it merged. Thanks for taking this on! We'll add some more issues we're open to contributions on over the next few days-- feel free to pick any of those up if you're interested. |
Understood... looking forward to working on other issues |
|
||
Effect::GetValue(height, round, timeout) => { | ||
let timeout = timeouts.duration_for(timeout.step); | ||
//Not available in Effect anymore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Effect::Broadcast
was renamed to Effect::Publish
@@ -105,7 +191,7 @@ pub struct Signature(pub Vec<u8>); | |||
pub type PublicKey = libp2p::identity::ed25519::PublicKey; | |||
pub type PrivateKey = libp2p::identity::ed25519::SecretKey; | |||
|
|||
impl malachite_common::SigningScheme for Ed25519 { | |||
impl informalsystems_malachitebft_core_types::SigningScheme for Ed25519 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will publish on crates.io (not yet done) the crate informalsystems-malachitebft-signing-ed25519 that can be reused here potentially.
Some other changes to watch out for as well informalsystems/malachite#723 |
Ref #181
This PR resulted in quite some changes, primarily due to breaking changes in
Malachite v0.0.1
compared to the commit Snapchain was using previously.There are many missing implementations in traits and Enums such as in
SnapchainValidatorContext
andEffect
,TimeoutKind
.Another function that needs to be looked at is
handle_msg
insrc/consensus/consensus.rs