Implement Sync API and improve syncing #539
Conversation
austinabell
requested review from
ansermino,
dutterbutter and
ec2
as code owners
austinabell
requested review from
RajarupanSampanthan,
StaticallyTypedAnxiety and
flodesi
| } | ||
|
|
||
| /// Puts a bad block Cid in the cache with a given reason. | ||
| pub async fn put(&self, c: Cid, reason: String) -> Option<String> { |
There was a problem hiding this comment.
Is there a reason why you wanted to make the reason a String? I would've done a enum similar to the Error Types we have for the Actors
There was a problem hiding this comment.
there just isn't a reason to distinguish specifically yet, and the flexibility is nice. Is a good idea though and I may look into it before this PR comes in
There was a problem hiding this comment.
I made the change, but decided to force push back because using an enum for this uses 56 bytes rather than the 24 needed for a string. There is no functional gain for this, so the extra bytes on such a large cache is counterproductive
austinabell
force-pushed
the
austin/rpc/sync
branch
from
d189ec6
to
d64d53d
Compare
| } | ||
|
|
||
| Ok(FullTipset::new(blocks).map_err(|e| e.to_string())?) |
There was a problem hiding this comment.
Optional : another approach would be to move this into a try_fold but the downside is that you would be allocating
There was a problem hiding this comment.
for what benefit?
There was a problem hiding this comment.
You would be potentially getting rid of a mutable block vec and switching control to the try_fold but the scope if fairly small so not too worried about that
There was a problem hiding this comment.
why would you try fold instead of just doing an iter() map()? I'm not really seeing any optimization here, but I can commit this?
Edit: I committed and I get that it's more idiomatic, but the negative to doing things like this is that it's less readable to people who don't understand rust which seems like a con with no benefit
There was a problem hiding this comment.
That's fair, I can see that.
Will retract my suggestion
| } | ||
| Ok(msgs) |
There was a problem hiding this comment.
Couldn't we just collect indexes.iter().map() into a Result<Vec<>,> here without allocating the msg?
There was a problem hiding this comment.
It gets allocated either way and is functionally the same, but I switched it
austinabell
requested review from
StaticallyTypedAnxiety and
RajarupanSampanthan
| pub struct ChainSyncer<DB, TBeacon> { | ||
| /// Syncing state of chain sync | ||
| state: SyncState, | ||
| // TODO should be a vector once syncing done async and ideally not wrap each state in mutex. | ||
| state: Arc<RwLock<SyncState>>, |
There was a problem hiding this comment.
I'd rename this to stage since the getter/setter are (get/set)_stage. Or change the method name
There was a problem hiding this comment.
SyncStage is a different struct and meaning, and there is no methods you are referring to, can you reference what you mean? The set_stage function sets the stage in the sync status, maybe confusing but suggest a different word for the state and stage if you'd like
There was a problem hiding this comment.
Oh youre right. I misread, the stage is inside the state. M'bad, carry on :)
Summary of changes
Changes introduced in this pull request:
GossipBlockto be able to serialize/deserialize them (Equivalent ofBlockMsgin Lotus)Also did a few other couple small things so feel free to ask about motivation or explanation for anything
Edit: oops forgot to add example commands to test manually:
Reference issue to close (if applicable)
Closes #535
Other information and links