Process blocks coming off of GossipSub #808
Conversation
ec2
requested review from
ansermino,
austinabell and
dutterbutter
as code owners
|
Oops forgot to fix unit tests that this breaks... And point to a new fork of libp2p_bitswap. Switching to draft for now. NVM, cant switch to draft |
ec2
added
Status: DO NOT MERGE
Status: Needs Changes
and removed
Status: Needs Review
labels
ec2
added
Status: Needs Review
and removed
Status: DO NOT MERGE
Status: Needs Changes
labels
blockchain/chain_sync/src/sync.rs
Outdated
| secp_messages: smsgs, | ||
| }; | ||
| let ts = FullTipset::new(vec![block]).unwrap(); | ||
| if let Err(e) = self.inform_new_head(source.clone().unwrap(), &ts).await { |
There was a problem hiding this comment.
the source isn't guaranteed because I don't believe we reject responses with no peer id set, should handle this in some way
There was a problem hiding this comment.
The source address should be guaranteed because of the signing policies we enforce on gossipsub messages.
There was a problem hiding this comment.
no, actually we explicitly allow this (because with out it before it was an issue) If you want to ensure this is the case, you need to change the ValidationMode on the GossipSub config. In any case, it would be much safer if there wasn't this unwrap when dealing with inputs directly from the network as it's too easy to trigger
There was a problem hiding this comment.
I've changed the validation mode to Strict and ive tested it against a local lotus devnet.
| match res { | ||
| Ok(Ok(())) => { | ||
| match self.db.get(&content) { | ||
| Ok(Some(b)) => Ok(b), | ||
| Ok(None) => Err(format!("Bitswap response successful for: {:?}, but can't find it in the database", content)), | ||
| Err(e) => Err(format!("Bitswap response successful for: {:?}, but can't retreive it from the database: {}", content, e.to_string())), | ||
| } | ||
| } | ||
| Err(e) => { | ||
| Err(format!("Bitswap get for {:?} timed out: {}", content, e.to_string())) | ||
| } | ||
| Ok(Err(e)) => { | ||
| Err(format!("Bitswap get for {:?} failed: {}", content, e.to_string())) | ||
| } | ||
| } |
There was a problem hiding this comment.
why does this have to be all matches when it's just returning errors or the result?
There was a problem hiding this comment.
Just wanted to return more detailed err messages.
There was a problem hiding this comment.
This is more confusing imo, at least for reading, I don't see why you couldn't at least return the timeout error before this match
Summary of changes
Changes introduced in this pull request:
To test this on a Lotus devnet, you will need to edit miner and power actor policies to allow for 2K sectors and minimum consensus power to 2048 instead of 10 << 40
This is the fork of bitswap we use now: ChainSafe/libp2p-bitswap@bd7e083
Reference issue to close (if applicable)
Closes
#757
#759 (if I understand the scope of that properly)
Other information and links