Add Networking and Republishing logic to MsgPool #732
Conversation
ec2
requested review from
ansermino,
austinabell and
dutterbutter
as code owners
There was a problem hiding this comment.
Have you considered using some actual linked list implementation such as the rust one? Seems very inefficient to use the vector and with all the unwraps and unsafe indexing scares me.
I'll go through more in depth later too because it's hard for me to wrap my head around all the mutex usage here
| if tail[0].curr().unwrap().gas_perf >= head.last().unwrap().curr().unwrap().gas_perf { | ||
| let mut chain_a_msgs = tail[0].curr().unwrap().msgs.clone(); | ||
| head.last_mut() | ||
| .unwrap() | ||
| .curr_mut() | ||
| .unwrap() | ||
| .msgs | ||
| .append(&mut chain_a_msgs); | ||
| head.last_mut().unwrap().curr_mut().unwrap().gas_reward += | ||
| &tail[0].curr().unwrap().gas_reward; | ||
| head.last_mut().unwrap().curr_mut().unwrap().gas_limit += | ||
| head.last().unwrap().curr().unwrap().gas_limit; | ||
| head.last_mut().unwrap().curr_mut().unwrap().gas_perf = get_gas_perf( | ||
| &head.last().unwrap().curr().unwrap().gas_reward, | ||
| head.last().unwrap().curr().unwrap().gas_limit, | ||
| ); | ||
| tail[0].curr_mut().unwrap().valid = false; | ||
| merged += 1; |
| } | ||
| pub fn get_gas_perf(gas_reward: &BigInt, gas_limit: i64) -> f64 { | ||
| let a = gas_reward * types::BLOCK_GAS_LIMIT; | ||
| a.to_f64().unwrap() / gas_limit as f64 |
There was a problem hiding this comment.
this isn't safe? Why is it assumed that f64 can accommodate the value?
There was a problem hiding this comment.
https://doc.rust-lang.org/std/f64/constant.MAX.html
MAX is stupidly high
There was a problem hiding this comment.
Yeah but the unwrap is based on how many significant figures as well, It's not clear all of the use cases of this function so it seems like it opens a vulnerability that can be triggered? Or is there something I'm missing here?
There was a problem hiding this comment.
OK, so i did some research. Im using the num-rational crate now here. It has the ability to convert a ratio of integers to a f64. I unwrap because it seems the only case where it returns None is when the denominator is 0. This should never be the case because the gas_limit is guaranteed to have a non-zero value.
| &tail[0].curr().unwrap().gas_reward; | ||
| head.last_mut().unwrap().curr_mut().unwrap().gas_limit += | ||
| head.last().unwrap().curr().unwrap().gas_limit; | ||
| head.last_mut().unwrap().curr_mut().unwrap().gas_perf = get_gas_perf( |
There was a problem hiding this comment.
Since you are unwrapping the same stuff for each iteration, you can just unwrap once and make all the changes to that variable, and it doesn't look as scary lol. Something like this might work :
let mut head_last_curr = head.last_mut().unwrap().curr_mut().unwrap();
head_last_curr.msgs.append(&mut chain_a_msgs);
| } | ||
| } | ||
| *cur_tipset.write().await = Arc::new(ts); | ||
| } | ||
| if repub { | ||
| repub_trigger.send(()).await; |
There was a problem hiding this comment.
should this not check if the channel is full before sending? The only issue I have with this is that it's a channel of undefined length and if you send multiple republish signals, would it not do duplicate unnecessary work? Wouldn't you just want it to behave like an atomic bool and some waking mechanism, where the other thread is just notified when there was an event, perform the work, then go back to sleep?
There was a problem hiding this comment.
Only place that a repub_trigger.send() happens is in head_change, so there shouldnt be duplicate work. This is because the republish function will return early if theres nothing pending. In head_change the pending messages map will be updated to delete messages that dont need to be propitigated.
Summary of changes
Changes introduced in this pull request:
The MsgChainNode and MsgChain structs indeed look kinda of funny. I feel like it can definitely be improved on, but the idea is to mimic a doubly linked list in a way that is close enough for our purposes. There are definitely a few things that seem kinda sketchy. Please reach out to me if the logic is super hard to follow.
Those unwraps on
.curr()will definitely raise some eyebrows, but they are safe insofar that you create MsgChain through the constructor. I figured this might not matter, since this is an internal datastructure. If you have a better way of doing around this, please let me know 😄Reference issue to close (if applicable)
Closes
#708
Other information and links