-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[Consensus Observer] Add ObservedOrderedBlock to various stores. #15892
base: co_exec_pool2
Are you sure you want to change the base?
Conversation
⏱️ 1h 45m total CI duration on this PR
|
d14fca6
to
ac7a415
Compare
decd111
to
fd55929
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
fd55929
to
1b11876
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
e9dcf63
to
d45756e
Compare
This comment has been minimized.
This comment has been minimized.
d45756e
to
1537252
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1537252
to
3197ade
Compare
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
pub enum ObservedOrderedBlock { | ||
Ordered(OrderedBlock), | ||
OrderedWithWindow(OrderedBlockWithWindow), | ||
} |
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.
Lovely
|
||
// A map of ordered blocks that are without payloads. The key is | ||
// the (epoch, round) of the first block in the ordered block. | ||
blocks_without_payloads: BTreeMap<(u64, Round), PendingBlockWithMetadata>, | ||
blocks_without_payloads: BTreeMap<(u64, Round), Arc<PendingBlockWithMetadata>>, | ||
|
||
// A map of ordered blocks that are without payloads. The key is | ||
// the hash of the first block in the ordered block. | ||
// Note: this is the same as blocks_without_payloads, but with a different key. | ||
blocks_without_payloads_by_hash: BTreeMap<HashValue, Arc<PendingBlockWithMetadata>>, |
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.
Two questions here:
-
Is this struct meant to emulate
PendingBlocks
on the ConsensusObservor side? If so might be good to add a doc comment at the top of the struct referencing it -
Does
blocks_without_payloads: BTreeMap<(u64, Round), Arc<PendingBlockWithMetadata>>
need both theepoch
andround
as keys? Why not justround
?
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.
Also maybe blocks_without_payloads
could be renamed to blocks_without_payloads_by_epoch_round
or just blocks_without_payloads_by_round
if epoch isn't necessary
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.
Is this struct meant to emulate PendingBlocks on the ConsensusObservor side?
Hmm... I'm not sure. There seem to be some similarities, but I'm reluctant to say they represent exactly the same thing. Probably best to keep them separate for now. 🤔
Why not just round?
Consensus observer can buffer blocks across epoch changes (so we require both fields). This helps with performance around epoch transitions.
Also maybe blocks_without_payloads could be renamed to blocks_without_payloads_by_epoch_round or just blocks_without_payloads_by_round if epoch isn't necessary
Hmm... the only reason to add by_hash
was to disambiguate it from the primary store above. Might just leave it for now.
Note: this PR is built on: #15869
Description
This PR adds a new
ObservedOrderedBlock
wrapper to consensus observer, with skeleton code. This supports self-contained ordered blocks, and blocks with windows (for execution pool).The PR offers the following commits:
ObservedOrderedBlock
type, and integrate it into the pending block store.ObservedOrderedBlock
to the ordered block store.In the next PR, I'll work on the window dependency traversal logic, and start working on the new buffering and clearing logic.
Testing Plan
New and existing test infrastructure.