[CX_HARDENING] Enhance DA Task Test Coverage #3568

shamb0 · 2024-08-13T06:36:48Z

This PR:

This PR partially addresses issue #3160 by adding test cases for the following scenarios:

Handling outdated proposals.
Detecting duplicate votes.
Validating vote collection and processing.
Ensuring non-leader nodes correctly handle DA votes.

Key Changes

Added a new test file: crates/testing/tests/tests_cx_hardening/cxh_da_task.rs.
Introduced the create_lean_test_handle() function to create a lightweight system handle for task state testing.
Fixed a bug in the build_da_cert<> function located in HotShot/crates/testing/src/helpers.rs.
Enhanced test functionality to verify external events on output_event_stream.

This PR does not:

Introduce any other changes beyond the mentioned test cases and bug fixes.

Key Areas to Review:

All files committed as part of this PR.

- Test outdated proposal handling - Verify duplicate vote detection - Validate vote collection and processing - Ensure correct DA vote handling by non-leader nodes Signed-off-by: shamb0 <[email protected]>

jparr721

Great first step, and thank you for doing this! Big ticket items are the following:

Let's just put all the da tests in the existing test file instead of its own module
Let's avoid spawning a custom handler for this test until, as a team, we're ready to refactor the entire test suite to use a more lean handler. If you want a more lean test handler without forcing a rewrite of the tests themselves, just change the build_system_handler helper method to call SystemContext::new instead of init, and that should get us 90% of what you've done here. If that works without needing to add all this new behavior, that'd be a big win.
Let's try to avoid handling external events since they nearly-always get turned into internal events anyway. A good place to look is network.rs or, broadly, the network task.

jparr721 · 2024-08-13T18:46:59Z

crates/hotshot/src/lib.rs

@@ -639,6 +639,31 @@ impl<TYPES: NodeType, I: NodeImplementation<TYPES>, V: Versions> SystemContext<T
    }
 }

+#[cfg(feature = "hotshot-testing")]
+impl<TYPES: NodeType, I: NodeImplementation<TYPES>, V: Versions> SystemContext<TYPES, I, V> {


I think we can just add this to the existing trait impl and enable the method when testing is enabled instead of doing separate bespoke impl.

I think this should actually just go in testing/src/helpers.rs (ungated), next to build_system_handle (which is what we use currently in the tests). I'd call it build_inactive_handle or something (I think "lean" is slightly misleading, because it's just a dummy handle that isn't running anything).

I made an issue a while ago to do something like this for the non-integration tests (all the task-specific tests in tests_1), so I think this is good to have in general

Good One @ss-es, I wanted to acknowledge the suggestion to place the test-specific helper function in testing/src/helpers.rs. During my implementation, I encountered issues due to the private and public(crate) visibility of dependent fields in pub struct SystemContext<> and pub struct SystemContextHandle<>. To avoid modifying the existing visibility of these fields, I opted for an alternative approach.

jparr721 · 2024-08-13T18:48:59Z

crates/testing/src/helpers.rs

+/// create the [`SystemContextHandle`] from a launcher
+/// # Panics
+/// if cannot create a [`HotShotInitializer`]
+pub async fn build_system_handle_from_launcher<


I'm hesitant to make such a specific change for the tests in this case, especially since you have them working and debugging is not as big of an issue at this point. While it's not ideal to spin up the entire SystemContext for unit tests, this method unfortunately creates a rift in what is the accepted testing framework, and what is done here. If we made this change, we'd want to propagate it to all tests, lest we end up in the situation where specific knowledge of this specific suite rests solely with someone who is not on the team.

I am not opposed to the outright removal of this in theory, though I think for this PR, we should stick to the existing spawner.

@jparr721, I understand your concerns about consistency in our testing framework. To clarify, the existing hotshot-testing::helpers::build_system_handle() function remains unchanged, and all current test cases continue to work as expected.

The new API, hotshot-testing::helpers::build_system_handle_from_launcher(), was added to address issue #3160, which requires 'Permute the inputs and verify if the tests still pass.'. While build_system_handle() uses default test configuration parameters, the new function provides flexibility by allowing modifications to the setup.

I suggest we review its usage in the test scenarios. If it doesn’t add significant value, we can consider removing it. Specifically,

Validating vote collection and processing.

Ensuring non-leader nodes correctly handle DA votes.

Why not just use the random! macro to get permuted inputs in that case? Furthermore, we do handle the vote collection and DA votes within the integration tests (e.g. test_success), so spinning up additional nodes for those might not be explicitly necessary.

Thank you, @jparr721, for your suggestion. I agree that utilizing random!() could be an effective way to introduce fuzziness in test configuration parameters. I will explore this approach and provide an update on the outcome.

jparr721 · 2024-08-13T18:49:29Z

crates/testing/src/helpers.rs

@@ -393,3 +538,40 @@ pub fn build_fake_view_with_leaf_and_state(
        },
    }
 }
+
+pub async fn check_external_events<TYPES: NodeType, S: StreamExt<Item = Event<TYPES>> + Unpin>(


Curious as to what this is doing that isn't handled by the testing macro?

@jparr721, I introduced a new function, fn check_external_events(), to validate events from the external event stream. This function provides additional validation that complements the existing testing macro. I've also created a simplified block diagram to illustrate its role within the TaskState validation context.

Note:: The dotted boxes labeled 'network layer' and 'external service layer' are not relevant to the 'TaskState' validation context.

See remark below. This is not necessary since the network task will transform the raw event into a HotShot event, so the checks will be redundant.

jparr721 · 2024-08-13T18:50:23Z

crates/testing/tests/tests_cx_hardening.rs

@@ -0,0 +1,3 @@
+mod tests_cx_hardening {
+    automod::dir!("tests/tests_cx_hardening");


I think tests_cx_hardening would make a task namespace into a first-class feature of the codebase, muddying the circumstances for when we should change them. Instead, let's just add these under the existing da task tests and remove this special file.

+1 to just throw this into one of the test groups

jparr721 · 2024-08-13T18:52:36Z

crates/testing/tests/tests_cx_hardening/cxh_da_task.rs

+    let external_event_expectations = vec![expect_external_events(vec![ext_event_exact(Event {
+        view_number: view3.view_number,
+        event: EventType::DaProposal {
+            proposal: view3.da_proposal.clone(),
+            sender: view3.leader_public_key,
+        },
+    })])];
+
+    // Create DA task state and script for the test
+    let da_state = DaTaskState::<TestTypes, MemoryImpl>::create_from(&handle).await;
+    let mut da_script = TaskScript {
+        timeout: Duration::from_millis(100),
+        state: da_state,
+        expectations,
+    };


I don't think we need these external event handlers as the DaProposal event is transformed into DaProposalRecv in the downstream handlers.

Related to the comment, #3568 (comment)

I think I still feel strongly that we don't necessarily need to process the external events. If the event is transformed successfully into a, for example, DaProposalRecv event, then we know with certainty that the external event was received, so we don't need to handle this stream in that case. To avoid bespoke changes, I'd prefer to remove it since the checks are redundant under this circumstance.

jparr721 · 2024-08-13T18:55:57Z

crates/types/src/event.rs

+impl<TYPES: NodeType> Eq for EventType<TYPES> {}
+
+impl<TYPES: NodeType> PartialEq for EventType<TYPES> {


This might be a bit overkill if the above external event changes are removed. If we cannot derive Eq and PartialEq here, perhaps we should consider fixing the types within EventType to do so instead, that way we can avoid arduously maintaining this for a yet-unneeded change.

+1 this should be a #derive. I think you might need to derive Eq and/or PartialEq for a few types, but I don't think that should be an issue

@jparr721, @ss-es, I agree that it’s not an ideal solution. My initial approach was to add #[derive(Eq, PartialEq)] annotations to the dependent inner types. However, this would have required modifying a significant number of files, so I considered handling it in a separate PR to reduce the review effort.

Please let me know if you prefer that I include these changes in this PR or if it would be better to address them in a separate PR.

I think I'm just strongly opposed to a custom Eq or PartialEq implementation.

But after looking over the PR some more, I'm not sure if any of this is really that necessary; I think I agree with @jparr721 that the external event checking could just be removed altogether.

We might have to discuss internally about whether we want to test this (and to what extent), but I think I would just split this off from the PR

Done @ss-es, I have completed the requested changes. Specifically, I removed the modifications related to 'external event checking' from this pull request.

Signed-off-by: shamb0 <[email protected]>

…DATask-test-coverage

Signed-off-by: shamb0 <[email protected]> [Libp2p] DHT overhaul (EspressoSystems#3548) Update builder marketplace API (EspressoSystems#3573) Fix BuilderDataSource for marketplace builder (EspressoSystems#3576) * Fix BuilderDataSource for marketplace builder Batch dependabot PRs (EspressoSystems#3570) Bump serde_json in the all group across 1 directory (EspressoSystems#3579) Fix upgrade lock in network task (EspressoSystems#3580) Bump the all group with 2 updates (EspressoSystems#3582) merge to upstream main Signed-off-by: shamb0 <[email protected]> Refactor: Address review comments Signed-off-by: shamb0 <[email protected]>

…DATask-test-coverage

[CX_HARDENING] Enhance DA Task Test Coverage

5023e59

- Test outdated proposal handling - Verify duplicate vote detection - Validate vote collection and processing - Ensure correct DA vote handling by non-leader nodes Signed-off-by: shamb0 <[email protected]>

shamb0 marked this pull request as ready for review August 13, 2024 06:37

shamb0 requested a review from bfish713 as a code owner August 13, 2024 06:37

jparr721 requested changes Aug 13, 2024

View reviewed changes

shamb0 added 3 commits August 14, 2024 09:29

Refactor: Address review comments

3cb4bc0

Signed-off-by: shamb0 <[email protected]>

Merge remote-tracking branch 'upstream/main' into shamb0/cxh-improve-…

8411537

…DATask-test-coverage

shamb0 force-pushed the shamb0/cxh-improve-DATask-test-coverage branch from 11f1190 to 02a3d4d Compare August 16, 2024 06:07

Merge remote-tracking branch 'upstream/main' into shamb0/cxh-improve-…

dfacef7

…DATask-test-coverage

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CX_HARDENING] Enhance DA Task Test Coverage #3568

[CX_HARDENING] Enhance DA Task Test Coverage #3568

shamb0 commented Aug 13, 2024

jparr721 left a comment •

edited

Loading

jparr721 Aug 13, 2024

ss-es Aug 14, 2024

shamb0 Aug 16, 2024

jparr721 Aug 13, 2024 •

edited

Loading

shamb0 Aug 14, 2024

jparr721 Aug 14, 2024 •

edited

Loading

shamb0 Aug 16, 2024

jparr721 Aug 13, 2024

shamb0 Aug 14, 2024 •

edited

Loading

jparr721 Aug 14, 2024

jparr721 Aug 13, 2024

ss-es Aug 14, 2024

jparr721 Aug 13, 2024

shamb0 Aug 14, 2024

jparr721 Aug 14, 2024

jparr721 Aug 13, 2024

ss-es Aug 14, 2024

shamb0 Aug 14, 2024

ss-es Aug 14, 2024

shamb0 Aug 16, 2024

		@@ -0,0 +1,3 @@
		mod tests_cx_hardening {
		automod::dir!("tests/tests_cx_hardening");

		impl<TYPES: NodeType> Eq for EventType<TYPES> {}

		impl<TYPES: NodeType> PartialEq for EventType<TYPES> {

[CX_HARDENING] Enhance DA Task Test Coverage #3568

Are you sure you want to change the base?

[CX_HARDENING] Enhance DA Task Test Coverage #3568

Conversation

shamb0 commented Aug 13, 2024

This PR:

Key Changes

jparr721 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jparr721 Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jparr721 Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamb0 Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jparr721 left a comment •

edited

Loading

jparr721 Aug 13, 2024 •

edited

Loading

jparr721 Aug 14, 2024 •

edited

Loading

shamb0 Aug 14, 2024 •

edited

Loading