-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
2f511ca
to
bb72a4f
Compare
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.
VID related parts look good to me.
Only question: now the vid commitment computation is not in parallel, does it impact the performance?
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.
Same question as Chengyu's, and some minor comments.
crates/legacy/src/lib.rs
Outdated
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.
Looks like we can remove this file. No need to address it in this PR. Made an issue: #291.
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.
Actually we may still need it if we decide to add the VID computation task back.
crates/legacy/src/service.rs
Outdated
// tracing::info!("Waiting for vid commitment for block {id}"); | ||
|
||
// let timeout_after = Instant::now() + self.max_api_waiting_time; | ||
|
||
// tracing::info!("Got vid commitment for block {id}",); | ||
|
||
// TODO: Should this still be here? | ||
// // We got VID in time with margin left. | ||
// // Maybe we can handle bigger blocks? | ||
// if timeout_after.duration_since(Instant::now()) | ||
// > self.max_api_waiting_time / VID_RESPONSE_TARGET_MARGIN_DIVISOR | ||
// { | ||
// // Increase max block size | ||
// self.global_state | ||
// .write_arc() | ||
// .await | ||
// .block_size_limits | ||
// .try_increment_block_size(truncated); | ||
// } |
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.
Nit: Can we delete this commented-out part if no longer needed?
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.
cc @pls148
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.
It looks good to me overall but I'm far from being a builder expert. I'll let someone more familiar with the builder to approve.
crates/legacy/src/builder_state.rs
Outdated
let vid_commitment = | ||
hotshot_types::traits::block_contents::vid_commitment(&encoded_txns, num_nodes); |
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.
Forgive my ignorance here. I don't know much about the VID computation. I get from this change that the VID commit computation is quick compared to the VID commit + precompute computation. Is that correct.
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.
No. VID commitment computation is slow.
181b994
to
0075478
Compare
8157d2e
to
ce0acf6
Compare
ce0acf6
to
e2a6475
Compare
0ddd9df
to
a145a4f
Compare
No description provided.