-
Notifications
You must be signed in to change notification settings - Fork 30
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
Make BlockPayload::from_transactions async and pass ValidatedState #3219
Conversation
ad1a525
to
237abaa
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.
This seems fine, though the testing changes should be reviewed by at least one other person. Your justification seems good though for adding this. Just check over my requests and let me know if any of them are unclear. Feel free to disregard if something isn't relevant.
let null_block_fee = null_block::builder_fee::<TYPES>( | ||
quorum_membership.total_nodes(), | ||
validated_state.as_ref(), | ||
instance_state.as_ref(), | ||
) | ||
.await | ||
.context("Failed to calculate null block fee info")?; |
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.
I think we are introducing a problem where this will block the consensus task for potentially a very long time. It only happens when there is an upgrade in flight so it would probably lay dormant until we need to do an upgrade and some nodes might stall.
I think we need to do this in the spawned proposal task now to avoid this issue
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.
done in 2d78d1e
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.
reverted
self.instance_state.as_ref(), | ||
) else { | ||
) |
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 issue here. I think the fix is straightforward. Just move this logic till the broadcast of BlockRecv
into a ansyc task (async_spawn
it)
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.
done in 2d78d1e
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.
where do I keep the handle of this task if we want to do it? I see some tasks handles kept in TaskRegistry
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.
You could do something like the consensus task does:
pub spawned_tasks: BTreeMap<TYPES::Time, Vec<JoinHandle<()>>>,
So we can keep a map around and key on the view or whatever relevant key we need, then we can cancel the task from there. The key here cancels old tasks if we propose for a newer view, so in this case we can probably cancel when we get a new view change? We could cancel with something reminiscent of the following: https://github.com/EspressoSystems/HotShot/blob/main/crates/task-impls/src/consensus/mod.rs#L151 but we might be able to just cancel all tasks older than view
. Thoughts @bfish713?
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.
I think this shouldn't be canceled on a view change since we should be able to complete catch up even if there is a view change to keep the updated chain config in ValidatedState.
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.
could be canceled after a certain time maybe
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.
I think we shouldn't drop the handle, though I don't think TaskRegistry
is the right place to put this (it will likely be impossible to put it there very soon). It should definitely be stored in TransactionTaskState
somehow, probably either in a Vec<JoinHandle<()>>
or a BTreeMap
like Jarred suggested.
I guess the question is: what is the expected behavior if consensus is on view (say) 100 and a calculation for view 50 is still going? do we just keep going indefinitely and risk accumulating these spawned subtasks? or is there a point where we can just give up? (I don't know enough to know what the answer is)
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.
reverted this change as builder_fee function is synchronous now 047cf5c
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 good, but I'm going to let Brendon approve since he had more specific requests.
dbb5612
to
047cf5c
Compare
Looks good I like that we change to add |
@jparr721 I'm holding off approval because this would be considered a breaking change during the RC process. Can you accept this we're ok merging it? |
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.
Phew, glad we got the dependency task tests included here.
The
BlockPayload::from_transactions
function takes anInstance
parameter, which containsChainConfig
on the sequencer side. To support updates to ChainConfig, we need to add it to a mutableValidatedState
. This requires the function to accept an additionalValidatedState
parameter. When the chainconfig commitment does not match the instance chainconfig, this function on the sequencer side will do catchup from the peers (GET request) to get the updated ChainConfig so it needs to be async.This PR:
EspressoSystems/espresso-sequencer#1475
EspressoSystems/espresso-sequencer#1492 (comment)