-
Notifications
You must be signed in to change notification settings - Fork 15
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
chore: Da dispersal unit tests update #720
Open
romanzac
wants to merge
18
commits into
master
Choose a base branch
from
chore-da-dispersal-unit-tests-update
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 17 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
243dba8
test: prepare to call stream disperse
romanzac 9fd2594
fix: formatting
romanzac 96a0e85
Merge branch 'master' into chore-da-dispersal-unit-tests-update
romanzac 2d73e61
Merge branch 'master' into chore-da-dispersal-unit-tests-update
romanzac a75921d
Merge branch 'master' into chore-da-dispersal-unit-tests-update
romanzac 1f6f835
fix: remove test_stream_disperse_error_cases
romanzac 82b8999
test: utils for cli
romanzac ef3d9f1
test: test_dispersal_with_swarms
romanzac a40b5da
fix: cleanup
romanzac ec30c0b
Merge branch 'master' into chore-da-dispersal-unit-tests-update
romanzac 1d81476
fix: rewrite without using sampling
romanzac 7fdea16
fix: outstanding space
romanzac a4103a2
fix: use executor.run
romanzac ca8a3a2
fix: formatting
romanzac 5cec099
Merge branch 'master' into chore-da-dispersal-unit-tests-update
romanzac 830b2ac
Merge branch 'master' into chore-da-dispersal-unit-tests-update
romanzac 84fac83
Merge branch 'master' into chore-da-dispersal-unit-tests-update
romanzac 7618d00
Merge branch 'master' into chore-da-dispersal-unit-tests-update
romanzac File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -129,3 +129,78 @@ where | |
} | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
pub mod test { | ||
use crate::da::network::swarm::ExecutorSwarm; | ||
use crate::test_utils::AllNeighbours; | ||
use kzgrs_backend::common::blob::DaBlob; | ||
use kzgrs_backend::common::Column; | ||
use libp2p::identity::Keypair; | ||
use libp2p::PeerId; | ||
use nomos_da_network_core::address_book::AddressBook; | ||
use nomos_da_network_core::swarm::validator::ValidatorSwarm; | ||
use nomos_libp2p::Multiaddr; | ||
use std::time::Duration; | ||
use tokio::sync::mpsc::unbounded_channel; | ||
|
||
#[tokio::test] | ||
async fn test_dispersal_with_swarms() { | ||
let k1 = Keypair::generate_ed25519(); | ||
let k2 = Keypair::generate_ed25519(); | ||
let executor_peer = PeerId::from_public_key(&k1.public()); | ||
let validator_peer = PeerId::from_public_key(&k2.public()); | ||
let neighbours = AllNeighbours { | ||
neighbours: [ | ||
PeerId::from_public_key(&k1.public()), | ||
PeerId::from_public_key(&k2.public()), | ||
] | ||
.into_iter() | ||
.collect(), | ||
}; | ||
|
||
let addr: Multiaddr = "/ip4/127.0.0.1/udp/5063/quic-v1".parse().unwrap(); | ||
let addr2 = addr.clone().with_p2p(validator_peer).unwrap(); | ||
let addr2_book = AddressBook::from_iter(vec![(executor_peer, addr2.clone())]); | ||
|
||
let (dispersal_broadcast_sender, _) = unbounded_channel(); | ||
|
||
let mut executor = | ||
ExecutorSwarm::new(k1, neighbours.clone(), dispersal_broadcast_sender.clone()); | ||
let (mut validator, _) = ValidatorSwarm::new(k2, neighbours.clone(), addr2_book); | ||
|
||
tokio::spawn(async move { | ||
let validator_swarm = validator.protocol_swarm_mut(); | ||
validator_swarm.listen_on(addr).unwrap(); | ||
|
||
validator.run().await; | ||
}); | ||
|
||
tokio::time::sleep(Duration::from_secs(1)).await; | ||
executor.dial(addr2).unwrap(); | ||
tokio::time::sleep(Duration::from_secs(1)).await; | ||
|
||
let executor_disperse_blob_sender = executor.swarm.behaviour().blobs_sender(); | ||
|
||
println!("Sending blob..."); | ||
executor_disperse_blob_sender | ||
.send(( | ||
0, | ||
DaBlob { | ||
column_idx: 0, | ||
column: Column(vec![]), | ||
column_commitment: Default::default(), | ||
aggregated_column_commitment: Default::default(), | ||
aggregated_column_proof: Default::default(), | ||
rows_commitments: vec![], | ||
rows_proofs: vec![], | ||
}, | ||
)) | ||
.unwrap(); | ||
|
||
let executor_task = tokio::spawn(async move { | ||
executor.run().await; | ||
}); | ||
assert!(executor_task.await.is_ok()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be great if you could update the asserts part to listen for responses, we should consider test successful if for all dispersed blobs we'd get |
||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
use libp2p::PeerId; | ||
use std::collections::HashSet; | ||
use subnetworks_assignations::MembershipHandler; | ||
|
||
#[derive(Clone)] | ||
pub struct AllNeighbours { | ||
pub neighbours: HashSet<PeerId>, | ||
} | ||
|
||
impl MembershipHandler for AllNeighbours { | ||
type NetworkId = u32; | ||
type Id = PeerId; | ||
|
||
fn membership(&self, _self_id: &Self::Id) -> HashSet<Self::NetworkId> { | ||
[0].into_iter().collect() | ||
} | ||
|
||
fn is_allowed(&self, _id: &Self::Id) -> bool { | ||
true | ||
} | ||
|
||
fn members_of(&self, _network_id: &Self::NetworkId) -> HashSet<Self::Id> { | ||
self.neighbours.clone() | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
After dialing, executor needs to signal the other end about new stream:
To have
open_stream_sender
you'd need to target theda-integration-tests
branch.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 could not wrap my head around one problem. When I call
executor_disperse_blob_sender.send()
, handle_dispersal_event() is never called from run().loop. What else should I do ?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 feel like our Swarm abstractions are only wrappers around libp2p's concepts? And we just forward all good and bad into upper layers.
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.
Well, they are. This is just a networking layer, it should not do anything else that forward/received messages according to the specs.
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.
If you inspect the executor dispersal behaviour, you'll see that before the blob is sent, the
peer_id
for desired subnetwork need to be available (.../executor/behaviour.rs:289
). For this reason try to apply the changes I suggested and callopen_stream_sender()
.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.
Let me invest one more day and finish this exercise. Perhaps it will be useful for error case tests which require deep understanding of streams.
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.
Here is an example how things get confusing:
Once
dispersal_broadcast_sender
becamedispersal_events_sender
. Just one level away we already start forgetting and shifting. Let my un-smartness be a guide here.nomos-node/nomos-cli/src/da/network/swarm.rs
Line 47 in 9f4f139
nomos-node/nomos-cli/src/da/network/backend.rs
Line 95 in 9f4f139
@danielSanchezQ Could not we have something like
Event Manager
which understands messages, errors? Backend user could just drop data and it will be taken care of ? Any performance penalty for this one more redirect ? WDYT ?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.
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.
Channel with the same purpose has different names.
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.
Well, yeah, we could make it more consistent in naming. But having the channels go away the layer should be good. I may be missing the point, but I do not think having an extra layer (event manager) would solve anything.
We usually have 3 layers:
The most important one is the Service, as the API should not change at all if possible. Imagine we change the core for something else (my most famous exaple: Pigeons). We just need to create a new (pigeons) backend while the service is left untouched. That makes pieces to be changed easily, is basically changing the type in the final overwatch app.
What I wanted to say with all this is that: