-
Notifications
You must be signed in to change notification settings - Fork 85
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
Dvir/papyrus consensus context #2111
Conversation
fd3ecd3
to
63bbbe2
Compare
63bbbe2
to
64ed484
Compare
64ed484
to
ed89722
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2111 +/- ##
==========================================
- Coverage 68.40% 68.40% -0.01%
==========================================
Files 132 132
Lines 17602 17602
Branches 17602 17602
==========================================
- Hits 12041 12040 -1
Misses 4226 4226
- Partials 1335 1336 +1 ☔ View full report in Codecov by Sentry. |
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.
Reviewed 3 of 3 files at r1, 7 of 7 files at r2.
Reviewable status: 5 of 7 files reviewed, 9 unresolved discussions (waiting on @DvirYo-starkware)
crates/sequencing/papyrus_consensus/src/lib.rs
line 32 at r1 (raw file):
loop { info!("Starting consensus for height {start_height}"); let mut shc = SingleHeightConsensus::new(start_height, context.clone(), validator_id).await;
Suggestion:
let mut shc = SingleHeightConsensus::new(current_height, context.clone(), validator_id).await;
crates/sequencing/papyrus_consensus/src/papyrus_consensus_context.rs
line 2 at r2 (raw file):
#[cfg(test)] #[path = "papyrus_context_test.rs"]
?
Suggestion:
papyrus_consensus_context_test
crates/sequencing/papyrus_consensus/src/papyrus_consensus_context.rs
line 57 at r2 (raw file):
} const CHANNEL_SIZE: usize = 5000;
Why 5k?
Code quote:
const CHANNEL_SIZE: usize = 5000;
crates/sequencing/papyrus_consensus/src/papyrus_consensus_context.rs
line 100 at r2 (raw file):
.block_hash, }) .expect("Send should succeed");
And below
Suggestion:
let block_hash = txn
.get_block_header(height)
.expect("Get header from storage failed")
.expect(&format!(
"Block in {height} was no found in storage although waiting for it"
))
.block_hash;
fin_sender
.send(PapyrusConsensusBlock {
content: transactions,
id: block_hash,
})
.expect("Send should succeed");
crates/sequencing/papyrus_consensus/src/papyrus_consensus_context.rs
line 124 at r2 (raw file):
.expect("Get transactions from storage failed") .expect(&format!( "Block in {height} was no found in storage although waiting for it"
Suggestion:
"Block in {height} was not found in storage despite waiting for it"
crates/sequencing/papyrus_consensus/src/papyrus_consensus_context.rs
line 128 at r2 (raw file):
for tx in transactions.iter() { let recived_tx = content
And below
Suggestion:
received_tx
crates/sequencing/papyrus_consensus/src/papyrus_consensus_context.rs
line 162 at r2 (raw file):
fn proposer(&self, _validators: &Vec<ValidatorId>, _height: BlockNumber) -> ValidatorId { 0u8.into() }
btw for validators
, in M1 we just expect to pass a config on startup with a static list of validators.
proposer
will just be something simple like self.config.validators[height % len]
Code quote:
async fn validators(&self, _height: BlockNumber) -> Vec<ValidatorId> {
vec![0u8.into(), 1u8.into(), 2u8.into()]
}
fn proposer(&self, _validators: &Vec<ValidatorId>, _height: BlockNumber) -> ValidatorId {
0u8.into()
}
crates/sequencing/papyrus_consensus/src/papyrus_consensus_context.rs
line 195 at r2 (raw file):
const SLEEP_BETWEEN_CHECK_FOR_BLOCK: Duration = Duration::from_secs(10); async fn wait_to_block(
This means that papyrus storage is behind the block that we want and so we will wait until the block can be found?
Suggestion:
async fn wait_for_block(
crates/sequencing/papyrus_consensus/src/papyrus_context_test.rs
line 101 at r2 (raw file):
} fn get_storage_with_block_and_papyrus_context()
Doesn't mention network_receiver and is already pretty long.
I thought that convention was to put these types of functions above the tests themselves?
Suggestion:
test_setup
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.
Reviewed 1 of 2 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @DvirYo-starkware)
crates/sequencing/papyrus_consensus/src/papyrus_consensus_context.rs
line 130 at r4 (raw file):
.next() .await .expect(&format!("Not recived transaction equals to {tx:?}"));
Suggestion:
received
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matan-starkware)
crates/sequencing/papyrus_consensus/src/lib.rs
line 32 at r1 (raw file):
loop { info!("Starting consensus for height {start_height}"); let mut shc = SingleHeightConsensus::new(start_height, context.clone(), validator_id).await;
Done.
crates/sequencing/papyrus_consensus/src/papyrus_consensus_context.rs
line 2 at r2 (raw file):
Previously, matan-starkware wrote…
?
This is on purpose. The name is getting too long, and the context is clear. I change this to end with this PR.
crates/sequencing/papyrus_consensus/src/papyrus_consensus_context.rs
line 57 at r2 (raw file):
Previously, matan-starkware wrote…
Why 5k?
why not? It should be larger than any existing block and not be too high.
crates/sequencing/papyrus_consensus/src/papyrus_consensus_context.rs
line 100 at r2 (raw file):
Previously, matan-starkware wrote…
And below
Done.
crates/sequencing/papyrus_consensus/src/papyrus_consensus_context.rs
line 124 at r2 (raw file):
.expect("Get transactions from storage failed") .expect(&format!( "Block in {height} was no found in storage although waiting for it"
Done.
crates/sequencing/papyrus_consensus/src/papyrus_consensus_context.rs
line 128 at r2 (raw file):
Previously, matan-starkware wrote…
And below
Done.
crates/sequencing/papyrus_consensus/src/papyrus_consensus_context.rs
line 162 at r2 (raw file):
Previously, matan-starkware wrote…
btw for
validators
, in M1 we just expect to pass a config on startup with a static list of validators.
proposer
will just be something simple likeself.config.validators[height % len]
Make this change after you run the consensus for the first time. It will make a problem when you don't have all the validators.
crates/sequencing/papyrus_consensus/src/papyrus_consensus_context.rs
line 195 at r2 (raw file):
Previously, matan-starkware wrote…
This means that papyrus storage is behind the block that we want and so we will wait until the block can be found?
Done.
crates/sequencing/papyrus_consensus/src/papyrus_context_test.rs
line 101 at r2 (raw file):
Previously, matan-starkware wrote…
Doesn't mention network_receiver and is already pretty long.
I thought that convention was to put these types of functions above the tests themselves?
Done.
59a9dcd
to
6d5bbcf
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.
Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @DvirYo-starkware)
6d5bbcf
to
54737b6
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.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @DvirYo-starkware)
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.
Reviewed 5 of 7 files at r2, 2 of 2 files at r3, 3 of 3 files at r5, 2 of 3 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @DvirYo-starkware)
This change is