refactor(sequencing): cende context, add logic and tests #3007
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @DvirYo-starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 10 at r1 (raw file):
#[case::success(RECORDER_WRITE_BLOB_PATH, 200, 10, Some(10), 1, true)] #[case::none_prev_block(RECORDER_WRITE_BLOB_PATH, 200, 10, None, 0, false)] #[case::mismatch_heihgt_prev_block(RECORDER_WRITE_BLOB_PATH, 200, 10, Some(7), 0, false)]
Suggestion:
mismatch_height_prev_blockThere was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 34 at r1 (raw file):
} let reciver = cende_ambassador.write_prev_height_blob(BlockNumber(written_height));
Suggestion:
receiverThere was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 37 at r1 (raw file):
assert_eq!(reciver.await.unwrap(), result); mock.expect(expected_calls);
from reading the docs I think you need to add mock.assert() after this line for the expectation to be enforced.
Suggestion:
mock.expect(expected_calls);
mock.assert();
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 15 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 8 at r1 (raw file):
#[rstest] #[case::success(RECORDER_WRITE_BLOB_PATH, 200, 10, Some(10), 1, true)]
define a const for prev_height = 10 that will be used also for the other test.
Then you can delete the written_height from the input to the function.
Code quote:
10crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 9 at r1 (raw file):
#[rstest] #[case::success(RECORDER_WRITE_BLOB_PATH, 200, 10, Some(10), 1, true)] #[case::none_prev_block(RECORDER_WRITE_BLOB_PATH, 200, 10, None, 0, false)]
Suggestion:
no_prev_blockcrates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 10 at r1 (raw file):
#[case::success(RECORDER_WRITE_BLOB_PATH, 200, 10, Some(10), 1, true)] #[case::none_prev_block(RECORDER_WRITE_BLOB_PATH, 200, 10, None, 0, false)] #[case::mismatch_heihgt_prev_block(RECORDER_WRITE_BLOB_PATH, 200, 10, Some(7), 0, false)]
Suggestion:
prev_block_height_mismatchcrates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 11 at r1 (raw file):
#[case::none_prev_block(RECORDER_WRITE_BLOB_PATH, 200, 10, None, 0, false)] #[case::mismatch_heihgt_prev_block(RECORDER_WRITE_BLOB_PATH, 200, 10, Some(7), 0, false)] #[case::send_recorder_fail("different_recorder_path", 200, 10, Some(10), 0, false)]
Suggestion:
send_to_recorder_failedcrates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 11 at r1 (raw file):
#[case::none_prev_block(RECORDER_WRITE_BLOB_PATH, 200, 10, None, 0, false)] #[case::mismatch_heihgt_prev_block(RECORDER_WRITE_BLOB_PATH, 200, 10, Some(7), 0, false)] #[case::send_recorder_fail("different_recorder_path", 200, 10, Some(10), 0, false)]
iiuc this case only tests the mock itself.
Code quote:
#[case::send_recorder_fail("different_recorder_path", 200, 10, Some(10), 0, false)]crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 17 at r1 (raw file):
#[case] path: &str, #[case] status_code: usize, #[case] written_height: u64,
this is not written yet,
I would go with height_to_write or just prev_height
Suggestion:
height_to_writecrates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 20 at r1 (raw file):
#[case] prev_block: Option<u64>, #[case] expected_calls: usize, #[case] result: bool,
Suggestion:
expected_resultcrates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 45 at r1 (raw file):
CendeAmbassador::new(CendeConfig { recorder_url: "http://parsable_url".parse().unwrap() }); const WRITTEN_HEIGHT: BlockNumber = BlockNumber(1701);
use the global const PREV_HEIGHT=10
Code quote:
const WRITTEN_HEIGHT: BlockNumber = BlockNumber(1701);crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 26 at r1 (raw file):
pub(crate) struct AerospikeBlob { // TODO(yael, dvir): add the blob fields. block_number: BlockNumber,
block_number is a part of the block_info
Code quote:
block_number: BlockNumber,crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 98 at r1 (raw file):
#[async_trait] impl CendeContext for CendeAmbassador { fn write_prev_height_blob(&self, height: BlockNumber) -> oneshot::Receiver<bool> {
Suggestion:
write_prev_height_blob(&self, prev_height: BlockNumber)crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 127 at r1 (raw file):
// Can happen in case the consensus got a block from the state sync and due to that not // update the cende ambassador in `decision_reached` function.
Suggestion:
// Can happen in case the consensus got a block from the state sync and due to that did not
// update the cende ambassador in `decision_reached` function.crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 128 at r1 (raw file):
// Can happen in case the consensus got a block from the state sync and due to that not // update the cende ambassador in `decision_reached` function. if blob.block_number != height {
iiuc you describe a scenario where the blob is older than the one we should write.
what happens if the blob block_number is bigger? I guess this function should return an error since it could indicate a bug in the consensus_context.
Suggestion:
blob.block_number < height
DvirYo-starkware
force-pushed
the
dvir/cende_refactor
branch
from
608d5fa to
e2fa3f7
Compare
There was a problem hiding this comment.
Reviewed 3 of 4 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)
crates/starknet_integration_tests/src/utils.rs line 77 at r2 (raw file):
create_state_sync_config(state_sync_storage_config, available_ports.get_next_port()); // Creates a mockito server for the recorder. The server returns a success status for each call, // which means that the recorder side will not have any problems in the test.
Suggestion:
// Creates a mockito server for the recorder. The server returns a success status for each call,
// which means that the recorder side will never return an error in the test.
There was a problem hiding this comment.
Reviewed 2 of 4 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @DvirYo-starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 136 at r1 (raw file):
oneshot_send(sender, false); return; }
Can you move this logic to some is_blob_valid(...) function?
The entire function is getting too long and hard to follow.
Code quote:
let Some(ref blob) = *prev_height_blob.lock().await else {
// This case happens when restarting the node, `prev_height_blob` intial value is
// `None`.
debug!("No blob to write to Aerospike.");
oneshot_send(sender, false);
return;
};
// Can happen in case the consensus got a block from the state sync and due to that not
// update the cende ambassador in `decision_reached` function.
if blob.block_number != height {
debug!(
"Mismatch blob block number and height, can't write blob to Aerospike. Blob \
block number {}, height {}",
blob.block_number, height
);
oneshot_send(sender, false);
return;
}crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 158 at r1 (raw file):
oneshot_send(sender, false); } }
This section can also move to a helper function.
Code quote:
debug!("Writing blob to Aerospike.");
match request_builder.json(blob).send().await {
Ok(response) => {
if response.status().is_success() {
debug!("Blob written to Aerospike successfully.");
oneshot_send(sender, true);
} else {
debug!(
"The recorder failed to write blob.\nStatus code: {}\nMessage: {}",
response.status(),
response.text().await.unwrap_or_default()
);
oneshot_send(sender, false);
}
}
Err(err) => {
debug!("Failed to send a request to the recorder. Error: {}", err);
oneshot_send(sender, false);
}
}crates/starknet_integration_tests/src/utils.rs line 79 at r2 (raw file):
// which means that the recorder side will not have any problems in the test. let mut server = mockito::Server::new_async().await; server.mock("POST", "/write_blob").with_status(200).create();
Suggestion:
/cende/write_blob
DvirYo-starkware
force-pushed
the
dvir/cende_refactor
branch
from
e2fa3f7 to
a42e0be
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 7 files reviewed, 19 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 8 at r1 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
define a const for prev_height = 10 that will be used also for the other test.
Then you can delete the written_height from the input to the function.
Done.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 11 at r1 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
iiuc this case only tests the mock itself.
No, it tested the case that the send failed, which is different than the case in which the send was successful, but the recorder returned an error.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 17 at r1 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
this is not written yet,
I would go with height_to_write or just prev_height
Done.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 37 at r1 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
from reading the docs I think you need to add mock.assert() after this line for the expectation to be enforced.
You are right, fixed
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 45 at r1 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
use the global const PREV_HEIGHT=10
Done.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 26 at r1 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
block_number is a part of the block_info
We need to have this field directly on the Python side to prevent deserialization of all the BlockInfo (if I remember correctly, it is part of the StateDiff)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 128 at r1 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
iiuc you describe a scenario where the blob is older than the one we should write.
what happens if the blob block_number is bigger? I guess this function should return an error since it could indicate a bug in the consensus_context.
It really does mean a bug. I added a todo for that for now.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 136 at r1 (raw file):
Previously, dafnamatsry wrote…
Can you move this logic to some
is_blob_valid(...)function?
The entire function is getting too long and hard to follow.
I made some changes. Is it cleaner now?
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 158 at r1 (raw file):
Previously, dafnamatsry wrote…
This section can also move to a helper function.
Moved to utility function
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 9 at r1 (raw file):
#[rstest] #[case::success(RECORDER_WRITE_BLOB_PATH, 200, 10, Some(10), 1, true)] #[case::none_prev_block(RECORDER_WRITE_BLOB_PATH, 200, 10, None, 0, false)]
Done.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 10 at r1 (raw file):
#[case::success(RECORDER_WRITE_BLOB_PATH, 200, 10, Some(10), 1, true)] #[case::none_prev_block(RECORDER_WRITE_BLOB_PATH, 200, 10, None, 0, false)] #[case::mismatch_heihgt_prev_block(RECORDER_WRITE_BLOB_PATH, 200, 10, Some(7), 0, false)]
Done.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 10 at r1 (raw file):
#[case::success(RECORDER_WRITE_BLOB_PATH, 200, 10, Some(10), 1, true)] #[case::none_prev_block(RECORDER_WRITE_BLOB_PATH, 200, 10, None, 0, false)] #[case::mismatch_heihgt_prev_block(RECORDER_WRITE_BLOB_PATH, 200, 10, Some(7), 0, false)]
Done.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 11 at r1 (raw file):
#[case::none_prev_block(RECORDER_WRITE_BLOB_PATH, 200, 10, None, 0, false)] #[case::mismatch_heihgt_prev_block(RECORDER_WRITE_BLOB_PATH, 200, 10, Some(7), 0, false)] #[case::send_recorder_fail("different_recorder_path", 200, 10, Some(10), 0, false)]
Done.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 20 at r1 (raw file):
#[case] prev_block: Option<u64>, #[case] expected_calls: usize, #[case] result: bool,
Done.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 34 at r1 (raw file):
} let reciver = cende_ambassador.write_prev_height_blob(BlockNumber(written_height));
Done.
crates/starknet_integration_tests/src/utils.rs line 77 at r2 (raw file):
create_state_sync_config(state_sync_storage_config, available_ports.get_next_port()); // Creates a mockito server for the recorder. The server returns a success status for each call, // which means that the recorder side will not have any problems in the test.
I have done this with a simple HTTP server instead. The mockito didn't work; I don't know exactly why.
crates/starknet_integration_tests/src/utils.rs line 79 at r2 (raw file):
// which means that the recorder side will not have any problems in the test. let mut server = mockito::Server::new_async().await; server.mock("POST", "/write_blob").with_status(200).create();
Done.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 98 at r1 (raw file):
#[async_trait] impl CendeContext for CendeAmbassador { fn write_prev_height_blob(&self, height: BlockNumber) -> oneshot::Receiver<bool> {
This intentionally. See the documentation for this function in the CendeContext trait.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 127 at r1 (raw file):
// Can happen in case the consensus got a block from the state sync and due to that not // update the cende ambassador in `decision_reached` function.
Done.
DvirYo-starkware
force-pushed
the
dvir/cende_refactor
branch
from
a42e0be to
558ee54
Compare
There was a problem hiding this comment.
Reviewed 3 of 6 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: 6 of 7 files reviewed, 19 unresolved discussions (waiting on @DvirYo-starkware and @Yael-Starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 136 at r1 (raw file):
Previously, DvirYo-starkware wrote…
I made some changes. Is it cleaner now?
Yes.
I think the other checks can also move out of the spawned task.
crates/starknet_integration_tests/src/utils.rs line 151 at r4 (raw file):
fn spawn_success_recorder() -> Url { // [127, 0, 0, 1] is the localhost IP address. let socket_addr = ([127, 0, 0, 1], find_free_port()).into();
into what? Can you specify the type or use from instead?
Code quote:
.into();crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 147 at r4 (raw file):
return receiver; async fn sent_write_blob(
Why are you defining it inside write_prev_height_blob?
Suggestion:
async fn send_write_blob(
DvirYo-starkware
force-pushed
the
dvir/cende_refactor
branch
from
558ee54 to
06ab7b0
Compare
There was a problem hiding this comment.
Reviewable status: 5 of 7 files reviewed, 19 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)
crates/starknet_integration_tests/src/utils.rs line 151 at r4 (raw file):
Previously, dafnamatsry wrote…
into what? Can you specify the type or use
frominstead?
Done.
DvirYo-starkware
requested review from
Itay-Tsabary-Starkware
and removed request for
Itay-Tsabary-Starkware
There was a problem hiding this comment.
Reviewable status: 5 of 7 files reviewed, 20 unresolved discussions (waiting on @dafnamatsry, @DvirYo-starkware, and @Yael-Starkware)
crates/starknet_integration_tests/src/utils.rs line 151 at r5 (raw file):
fn spawn_success_recorder() -> Url { // [127, 0, 0, 1] is the localhost IP address. let socket_addr = SocketAddr::from(([127, 0, 0, 1], find_free_port()));
We don't use this function any more. Let's further discuss in person.
As a pointer, check out the AvailablePorts struct.
Code quote:
let socket_addr = SocketAddr::from(([127, 0, 0, 1], find_free_port()));
There was a problem hiding this comment.
Reviewed 1 of 6 files at r3.
Reviewable status: 6 of 7 files reviewed, 9 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 11 at r1 (raw file):
Previously, DvirYo-starkware wrote…
No, it tested the case that the send failed, which is different than the case in which the send was successful, but the recorder returned an error.
but in practice it doesn't take the if branch you want it to .
The mock returns ok with error code 501 in this case and takes the same if branch in the code as the next case.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 26 at r1 (raw file):
Previously, DvirYo-starkware wrote…
We need to have this field directly on the Python side to prevent deserialization of all the
BlockInfo(if I remember correctly, it is part of theStateDiff)
so maybe worth adding a check to see that it matches the value from the block_info, please add a todo for it.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 98 at r1 (raw file):
Previously, DvirYo-starkware wrote…
This intentionally. See the documentation for this function in the
CendeContexttrait.
so maybe current_height, just to make it clear when using the function.
crates/starknet_integration_tests/src/utils.rs line 77 at r2 (raw file):
Previously, DvirYo-starkware wrote…
I have done this with a simple HTTP server instead. The mockito didn't work; I don't know exactly why.
lets understand why, but not as part of this PR.
In general the mocking part should be in a separate PR from the cende internal logic.
There was a problem hiding this comment.
Reviewable status: 6 of 7 files reviewed, 11 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 128 at r5 (raw file):
// Can happen in case the consensus got a block from the state sync and due to that did // not update the cende ambassador in `decision_reached` function.
I think that if the consensus got a newer block from the state_update it should clear the cende_ambassador.
maybe we need an additional api for clear_blob.
and then any height mismatch means a consensus_context bug.
Code quote:
// Can happen in case the consensus got a block from the state sync and due to that did
// not update the cende ambassador in `decision_reached` function.crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 188 at r5 (raw file):
#[derive(Clone, Debug, Default)] pub struct BlobParameters { height: u64,
if you're worried for the serialization, BlockNumber also serializes to an integer so it's fine to keep it that way.
Code quote:
u64,
DvirYo-starkware
force-pushed
the
dvir/cende_refactor
branch
from
06ab7b0 to
01ab55f
Compare
There was a problem hiding this comment.
Reviewable status: 2 of 7 files reviewed, 11 unresolved discussions (waiting on @dafnamatsry, @Itay-Tsabary-Starkware, and @Yael-Starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 11 at r1 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
but in practice it doesn't take the if branch you want it to .
The mock returns ok with error code 501 in this case and takes the same if branch in the code as the next case.
You are right; my mistake. Removed.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 26 at r1 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
so maybe worth adding a check to see that it matches the value from the block_info, please add a todo for it.
It will be taken from the block info when we create the real blob. This will happen in the next PR.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 98 at r1 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
so maybe current_height, just to make it clear when using the function.
done
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 136 at r1 (raw file):
Previously, dafnamatsry wrote…
Yes.
I think the other checks can also move out of the spawned task.
I like it that way because all the relevant logic is directly in the spawned task.
In addition, this is a bit hard because we play with a reference to a blob. Unless you have a strong opinion about that, let's keep it that way.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 147 at r4 (raw file):
Previously, dafnamatsry wrote…
Why are you defining it inside
write_prev_height_blob?
Done.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 128 at r5 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
I think that if the consensus got a newer block from the state_update it should clear the cende_ambassador.
maybe we need an additional api for clear_blob.and then any height mismatch means a consensus_context bug.
I prefer not to do this because this invariant is hard to save. You can add it to the Monday and we will discuss it in the future.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 188 at r5 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
if you're worried for the serialization, BlockNumber also serializes to an integer so it's fine to keep it that way.
No. I use height when this is a raw info from consensus (in consensus the use height as u64 and not BlockNumber)
crates/starknet_integration_tests/src/utils.rs line 77 at r2 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
lets understand why, but not as part of this PR.
In general the mocking part should be in a separate PR from the cende internal logic.
This is part of this PR because, without that, it will not pass tests
crates/starknet_integration_tests/src/utils.rs line 151 at r5 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
We don't use this function any more. Let's further discuss in person.
As a pointer, check out theAvailablePortsstruct.
Done.
There was a problem hiding this comment.
Reviewable status: 2 of 7 files reviewed, 12 unresolved discussions (waiting on @dafnamatsry, @DvirYo-starkware, and @Yael-Starkware)
crates/starknet_integration_tests/src/utils.rs line 128 at r6 (raw file):
timeouts.proposal_timeout *= 3; let recorder_url = spawn_success_recorder(available_ports.get_next_port());
Please move this to be near the spawning of the rpc state reader server.
Code quote:
let recorder_url = spawn_success_recorder(available_ports.get_next_port());
There was a problem hiding this comment.
Reviewable status: 2 of 7 files reviewed, 8 unresolved discussions (waiting on @dafnamatsry, @DvirYo-starkware, and @Itay-Tsabary-Starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 188 at r5 (raw file):
Previously, DvirYo-starkware wrote…
No. I use height when this is a raw info from consensus (in consensus the use height as
u64and notBlockNumber)
I prefer BlockNumber. optional
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 139 at r6 (raw file):
oneshot_send(sender, false); return; }
extract to a function should_write_blob().
Code quote:
let Some(ref blob): Option<AerospikeBlob> = *prev_height_blob.lock().await else {
// This case happens when restarting the node, `prev_height_blob` intial value is
// `None`.
debug!("No blob to write to Aerospike.");
oneshot_send(sender, false);
return;
};
// Can happen in case the consensus got a block from the state sync and due to that did
// not update the cende ambassador in `decision_reached` function.
// TODO(dvir): what to do in the case of the `blob.block_number.0 >= height.0`? this
// means a bug.
if blob.block_number.0 + 1 != current_height.0 {
debug!(
"Mismatch blob block number and height, can't write blob to Aerospike. Blob \
block number {}, height {}",
blob.block_number, current_height
);
oneshot_send(sender, false);
return;
}crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 16 at r6 (raw file):
#[tokio::test] async fn write_prev_height_blob( #[case] status_code: usize,
Suggestion:
mock_status_code
DvirYo-starkware
force-pushed
the
dvir/cende_refactor
branch
from
01ab55f to
fdde48e
Compare
DvirYo-starkware
force-pushed
the
dvir/cende_refactor
branch
from
51ee6df to
ec5fd61
Compare
There was a problem hiding this comment.
Reviewable status: 2 of 10 files reviewed, 5 unresolved discussions (waiting on @dafnamatsry, @Itay-Tsabary-Starkware, and @Yael-Starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 136 at r1 (raw file):
Previously, dafnamatsry wrote…
Not insisting, but here is why I think it would be better to move it out:
- Saves spawing a task when not needed. This kind of basic validations don't need to be a separated task.
- In invalid cases (such as blob.height > current_height) you could return immediately with an informative error.
Although I do not entirely agree with the separation, I would do that because you both asked, but there is technical complexity. Unless we decide we don't care to clone the blob object (which can be very big) in each writing, that means returning locked objects between functions and returning a reference from a function, which will make it harder to understand and refactor in need.
I added a todo for that.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 139 at r6 (raw file):
Previously, dafnamatsry wrote…
+1
added a todo
crates/starknet_integration_tests/src/utils.rs line 128 at r6 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Please move this to be near the spawning of the rpc state reader server.
Done.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 16 at r6 (raw file):
#[tokio::test] async fn write_prev_height_blob( #[case] status_code: usize,
I think this is clear from the context
|
Benchmark movements: |
There was a problem hiding this comment.
Reviewed 1 of 2 files at r4, 1 of 6 files at r8, 3 of 3 files at r9, all commit messages.
Reviewable status: 6 of 10 files reviewed, 6 unresolved discussions (waiting on @dafnamatsry, @DvirYo-starkware, and @Itay-Tsabary-Starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 16 at r6 (raw file):
Previously, DvirYo-starkware wrote…
I think this is clear from the context
I actually had to read the docs to be sure it is the return value and not some input to verify.
"return_code/return_status_code" is also fine.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 139 at r6 (raw file):
Previously, DvirYo-starkware wrote…
added a todo
ack, if it makes it easier to apply in a separate PR.
temp.txt line 1 at r9 (raw file):
please remove this file from the PR
There was a problem hiding this comment.
Reviewed 3 of 6 files at r8, 3 of 3 files at r9, all commit messages.
Reviewable status: 8 of 10 files reviewed, 6 unresolved discussions (waiting on @DvirYo-starkware, @Itay-Tsabary-Starkware, and @Yael-Starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 136 at r1 (raw file):
Previously, DvirYo-starkware wrote…
Although I do not entirely agree with the separation, I would do that because you both asked, but there is technical complexity. Unless we decide we don't care to clone the blob object (which can be very big) in each writing, that means returning locked objects between functions and returning a reference from a function, which will make it harder to understand and refactor in need.
I added a todo for that.
We don't want to clone it.
But not sure I understand the complexity, can you elaborate?
Also, I don't see anything wrong with returning a reference from a function.
DvirYo-starkware
force-pushed
the
dvir/cende_refactor
branch
from
ec5fd61 to
ec8f48f
Compare
There was a problem hiding this comment.
Reviewable status: 5 of 10 files reviewed, 6 unresolved discussions (waiting on @dafnamatsry, @Itay-Tsabary-Starkware, and @Yael-Starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/cende_test.rs line 16 at r6 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
I actually had to read the docs to be sure it is the return value and not some input to verify.
"return_code/return_status_code" is also fine.
Done. We call this value status_code in other tests in the rep.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 136 at r1 (raw file):
Previously, dafnamatsry wrote…
We don't want to clone it.
But not sure I understand the complexity, can you elaborate?
Also, I don't see anything wrong with returning a reference from a function.
עד כמה שניסיתי, אם מחזירים רפרנס לאובייקט בתוך מוטקס צריך עדין להיות עם נעילה על המוטקס וזה נהיה קצת מסורבל ולא נעים טכנית ולקורא.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 139 at r6 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
ack, if it makes it easier to apply in a separate PR.
Done.
temp.txt line 1 at r9 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
please remove this file from the PR
Done.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r4, 1 of 3 files at r7, 2 of 6 files at r8, 1 of 5 files at r10, all commit messages.
Reviewable status: 6 of 10 files reviewed, 4 unresolved discussions (waiting on @dafnamatsry, @DvirYo-starkware, and @Itay-Tsabary-Starkware)
There was a problem hiding this comment.
Reviewable status: 6 of 10 files reviewed, 4 unresolved discussions (waiting on @dafnamatsry, @DvirYo-starkware, and @Itay-Tsabary-Starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 136 at r1 (raw file):
Previously, DvirYo-starkware wrote…
עד כמה שניסיתי, אם מחזירים רפרנס לאובייקט בתוך מוטקס צריך עדין להיות עם נעילה על המוטקס וזה נהיה קצת מסורבל ולא נעים טכנית ולקורא.
I don't think you can pass a locked object between threads.
DvirYo-starkware
force-pushed
the
dvir/cende_refactor
branch
from
ec8f48f to
49ca25e
Compare
|
Benchmark movements: |
There was a problem hiding this comment.
Reviewed 2 of 5 files at r10, 3 of 3 files at r11, all commit messages.
Reviewable status: 9 of 10 files reviewed, 3 unresolved discussions (waiting on @DvirYo-starkware, @Itay-Tsabary-Starkware, and @Yael-Starkware)
There was a problem hiding this comment.
Reviewable status: 9 of 10 files reviewed, 2 unresolved discussions (waiting on @DvirYo-starkware and @Itay-Tsabary-Starkware)
There was a problem hiding this comment.
Reviewed 1 of 6 files at r8, 2 of 3 files at r11.
Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @DvirYo-starkware)
-- commits line 2 at r11:
Conceptual 
- Please wait with this until we discuss the system test implication with @dan-starkware
- There's a conflict with
main
Code quote:
New commits in r11 on 05/01/2025 at 14:38, probably rebased from r10:
- 49ca25e: refactor(sequencing): cende context, add logic and tests
49ca25e to
0685a6e
Compare
There was a problem hiding this comment.
Reviewable status: 7 of 10 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry and @Itay-Tsabary-Starkware)
crates/starknet_integration_tests/src/integration_test_setup.rs line 22 at r12 (raw file):
use crate::config_utils::dump_config_file_changes; use crate::state_reader::StorageTestSetup; use crate::utils::{create_node_config, spawn_success_recorder};
rebase things
There was a problem hiding this comment.
Reviewed 1 of 2 files at r4, 1 of 3 files at r7, 2 of 5 files at r10, 1 of 3 files at r11, 2 of 2 files at r12, all commit messages.
Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @DvirYo-starkware)
DvirYo-starkware
force-pushed
the
dvir/cende_refactor
branch
2 times, most recently
from
bc58bce to
976cfda
Compare
There was a problem hiding this comment.
Reviewed 4 of 5 files at r13, all commit messages.
Reviewable status: 8 of 10 files reviewed, 2 unresolved discussions (waiting on @DvirYo-starkware, @Itay-Tsabary-Starkware, and @Yael-Starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 113 at r13 (raw file):
current_height ); return tokio::spawn(ready(true));
Either:
- Change the return value to somthing like
Result<Joinhandle<bool>>and returnErr(Skipped)or something like that. - Split to 2 functions:
should_writeandwrite.
Code quote:
return tokio::spawn(ready(true));
There was a problem hiding this comment.
Reviewable status: 8 of 10 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @DvirYo-starkware, @Itay-Tsabary-Starkware, and @Yael-Starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 113 at r13 (raw file):
Previously, dafnamatsry wrote…
Either:
- Change the return value to somthing like
Result<Joinhandle<bool>>and returnErr(Skipped)or something like that.- Split to 2 functions:
should_writeandwrite.
- We want this kind of behavior to be transparent in the consensus context. the context tries to write to AS; if it succeeds, he can continue. Otherwise, it fails and does not need to understand the inner complexity. Note that this case is not an error or negative flow.
- We talked about this already. I will try to do it in a different PR. There is a todo.
DvirYo-starkware
force-pushed
the
dvir/cende_refactor
branch
from
976cfda to
3038b21
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r14, all commit messages.
Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @DvirYo-starkware)
There was a problem hiding this comment.
Reviewed 1 of 5 files at r10.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @DvirYo-starkware)
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:
No description provided.