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

Support deletes #68

Merged
merged 4 commits into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ threadpool = "1.8.1"
rand = "0.8.5"
humantime-serde = "1.1.1"
humantime = "2.1.0"
itertools = "0.13.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

don't think this made it into the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It did. This is what implements into_group_map_by

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, it's one of those things that adds magic behind the scenes. Still getting used to that.


[build-dependencies]
tonic-build = "0.9.2"
Expand Down
2 changes: 1 addition & 1 deletion src/consensus/consensus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use malachite_consensus::{Effect, ProposedValue, Resume, SignedConsensusMsg};
use malachite_metrics::Metrics;

use crate::consensus::timers::{TimeoutElapsed, TimerScheduler};
use crate::consensus::validator::{self, ShardValidator};
use crate::consensus::validator::ShardValidator;
use crate::core::types::{
Height, ShardId, SnapchainContext, SnapchainShard, SnapchainValidator,
SnapchainValidatorContext,
Expand Down
2 changes: 1 addition & 1 deletion src/consensus/proposer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ impl BlockProposer {
async fn publish_new_block(&self, block: Block) {
match self.block_tx.send(block.clone()).await {
Err(err) => {
error!("Erorr publishing new block {:#?}", err)
error!("Error publishing new block {:?}", err.to_string());
}
Ok(_) => {}
}
Expand Down
17 changes: 17 additions & 0 deletions src/core/message.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::proto::msg as message;
use crate::proto::snapchain;

impl message::Message {
pub fn is_type(&self, message_type: message::MessageType) -> bool {
Expand All @@ -20,4 +21,20 @@ impl message::Message {
0
}
}

pub fn hex_hash(&self) -> String {
hex::encode(&self.hash)
}
}

impl snapchain::ValidatorMessage {
pub fn fid(&self) -> u32 {
Copy link
Contributor

@suurkivi suurkivi Nov 22, 2024

Choose a reason for hiding this comment

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

Two things:

  1. I might prefer something like below for improved readability:
impl snapchain::ValidatorMessage {
    pub fn fid(&self) -> u32 {
        if let Some(fname) = &self.fname_transfer {
            return fname.fid as u32;
        }
        if let Some(event) = &self.on_chain_event {
            return event.fid as u32;
        }
        0
    }
}

I think upon seeing the unwrap there I really have to scrutinize the code, and it took me a moment to realize that it was indeed safe due to the is_some() above.

I think we can probably remove most of the unwraps() in this file with similar refactors and it will be MUCH faster to reason about, especially at a glance.

  1. What to do in the fall through case? Should we return an error or even panic? We don't expect this to ever be either? Can we model as actual enum in protobuf?

Maybe 0 is fine, but perhaps a comment? Or a TODO for end state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Ah, much cleaner. Fixed.
  2. I think if we use oneof it will use enums. We should probably switch to that actually. But even if we did, it won't solve the problem because it's acceptable to have 0 as a value in the protobuf, so callers have to handle fid 0 (which is not valid for the application, fids start from 1). So, I think defaulting to 0 is fine here.

if let Some(fname) = &self.fname_transfer {
return fname.fid as u32;
}
if let Some(event) = &self.on_chain_event {
return event.fid as u32;
}
0
}
}
8 changes: 4 additions & 4 deletions src/network/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::core::error::HubError;
use crate::proto::msg as message;
use crate::proto::rpc::snapchain_service_server::SnapchainService;
use crate::proto::rpc::{BlocksRequest, BlocksResponse, ShardChunksRequest, ShardChunksResponse};
use crate::storage::store::engine::Message;
use crate::storage::store::engine::MempoolMessage;
use crate::storage::store::shard::ShardStore;
use crate::storage::store::BlockStore;
use hex::ToHex;
Expand All @@ -13,7 +13,7 @@ use tonic::{Request, Response, Status};
use tracing::info;

pub struct MySnapchainService {
message_tx: mpsc::Sender<Message>,
message_tx: mpsc::Sender<MempoolMessage>,
block_store: BlockStore,
shard_stores: HashMap<u32, ShardStore>,
}
Expand All @@ -22,7 +22,7 @@ impl MySnapchainService {
pub fn new(
block_store: BlockStore,
shard_stores: HashMap<u32, ShardStore>,
message_tx: mpsc::Sender<Message>,
message_tx: mpsc::Sender<MempoolMessage>,
) -> Self {
Self {
block_store,
Expand All @@ -43,7 +43,7 @@ impl SnapchainService for MySnapchainService {

let message = request.into_inner();
self.message_tx
.send(Message::UserMessage(message.clone()))
.send(MempoolMessage::UserMessage(message.clone()))
.await
.unwrap(); // Do we need clone here? I think yes?

Expand Down
7 changes: 3 additions & 4 deletions src/node/snapchain_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,9 @@ use crate::core::types::{
SnapchainValidatorSet,
};
use crate::network::gossip::GossipEvent;
use crate::proto::msg as message;
use crate::proto::snapchain::{Block, ShardChunk};
use crate::storage::db::RocksDB;
use crate::storage::store::engine::{BlockEngine, Message, ShardEngine};
use crate::storage::store::engine::{BlockEngine, MempoolMessage, ShardEngine};
use crate::storage::store::shard::ShardStore;
use crate::storage::store::BlockStore;
use libp2p::identity::ed25519::Keypair;
Expand All @@ -24,7 +23,7 @@ const MAX_SHARDS: u32 = 3;

pub struct SnapchainNode {
pub consensus_actors: BTreeMap<u32, ActorRef<ConsensusMsg<SnapchainValidatorContext>>>,
pub messages_tx_by_shard: HashMap<u32, mpsc::Sender<Message>>,
pub messages_tx_by_shard: HashMap<u32, mpsc::Sender<MempoolMessage>>,
pub shard_stores: HashMap<u32, ShardStore>,
pub address: Address,
}
Expand All @@ -45,7 +44,7 @@ impl SnapchainNode {

let (shard_decision_tx, shard_decision_rx) = mpsc::channel::<ShardChunk>(100);

let mut shard_messages: HashMap<u32, mpsc::Sender<Message>> = HashMap::new();
let mut shard_messages: HashMap<u32, mpsc::Sender<MempoolMessage>> = HashMap::new();

let mut shard_stores: HashMap<u32, ShardStore> = HashMap::new();

Expand Down
2 changes: 1 addition & 1 deletion src/proto/blocks.proto
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ message Transaction {

// Fname transfers
message FnameTransfer {

uint64 fid = 1;
}

// Validator initiated prunes/revokes etc
Expand Down
1 change: 0 additions & 1 deletion src/storage/store/account/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ use crate::{
proto::msg::{link_body::Target, message_data::Body, Message, MessageType},
storage::db::{RocksDB, RocksDbTransactionBatch},
};
use prost::Message as _;
use std::clone::Clone;
use std::string::ToString;
use std::sync::{Arc, Mutex};
Expand Down
Loading