Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Changes from 1 commit
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
31cb1ad
feat bitfield distribution
drahnr Jul 7, 2020
ddb6228
feat bitfield distribution part 2
drahnr Jul 7, 2020
6ac2c62
pair programming with rustc & cargo
drahnr Jul 7, 2020
4b100df
lets go
drahnr Jul 7, 2020
a3bd8c0
move bitfield-distribution to the node/network folder
drahnr Jul 8, 2020
7909d47
shape shifting
drahnr Jul 8, 2020
e3c8248
lunchtime
drahnr Jul 8, 2020
86f4fdb
ignore the two fn recursion for now
drahnr Jul 8, 2020
3cf0878
step by step
drahnr Jul 9, 2020
6a6d7bc
triplesteps
drahnr Jul 9, 2020
1aa1842
bandaid commit
drahnr Jul 10, 2020
ad0555b
unordered futures magic
drahnr Jul 10, 2020
7966769
chore
drahnr Jul 10, 2020
e8c901a
reword markdown
drahnr Jul 10, 2020
e33dafe
clarify
drahnr Jul 10, 2020
6594dee
lacks abortable processing impl details
drahnr Jul 14, 2020
4c6f9b4
slimify
drahnr Jul 14, 2020
af2849b
fix: warnings and avoid ctx.clone() improve comments
drahnr Jul 15, 2020
977102b
review comments
drahnr Jul 20, 2020
92431f3
fix details
drahnr Jul 20, 2020
23c050e
make sure outgoing messages are tracked
drahnr Jul 20, 2020
a431601
fix name
drahnr Jul 20, 2020
a5077ee
Merge remote-tracking branch 'origin/master' into bernhard-bitfield-d…
drahnr Jul 20, 2020
be2533e
fix subsystem
drahnr Jul 20, 2020
64b6ec1
partial test impl
drahnr Jul 20, 2020
52d9800
relax context bounds
drahnr Jul 21, 2020
df8fe1d
test
drahnr Jul 21, 2020
38661f7
X
drahnr Jul 21, 2020
f1f7cab
X
drahnr Jul 21, 2020
f94b8c5
initial test
drahnr Jul 21, 2020
7a4bd5e
fix relay_message not tracked when origin is self
drahnr Jul 21, 2020
b299ad3
fix/guide: grammar
drahnr Jul 21, 2020
09766c1
work around missing Eq+PartialEq
drahnr Jul 21, 2020
df7a6f0
fix: add missing message to provisioner
drahnr Jul 21, 2020
2398681
unify per_job to job_data
drahnr Jul 21, 2020
8ed8aba
fix/review: part one
drahnr Jul 22, 2020
65e8edb
fix/review: more grumbles
drahnr Jul 22, 2020
087784a
fix/review: track incoming messages per peer
drahnr Jul 22, 2020
d91bcbd
fix/review: extract fn, avoid nested matches
drahnr Jul 22, 2020
c957bf9
fix/review: more tests, simplify test
drahnr Jul 22, 2020
36a6672
fix/review: extend tests to cover more cases
drahnr Jul 22, 2020
5b7c963
chore/rename: Tracker -> ProtocolState
drahnr Jul 22, 2020
794c6c3
chore check and comment rewording
drahnr Jul 22, 2020
f3a718e
feat test: invalid peer message
drahnr Jul 22, 2020
e33e943
remove ignored test cases and unused macros
drahnr Jul 22, 2020
b8d3941
Merge remote-tracking branch 'origin/master' into bernhard-bitfield-d…
drahnr Jul 23, 2020
99f574e
fix master merge fallout + warnings
drahnr Jul 23, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
unify per_job to job_data
  • Loading branch information
drahnr committed Jul 21, 2020
commit 2398681a99e460206bc5f9034955f3b9a89be7dc
22 changes: 11 additions & 11 deletions node/network/bitfield-distribution/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,10 @@ impl BitfieldDistribution {
signed_availability,
) => {
trace!(target: "bitd", "Processing DistributeBitfield");
let per_job = &mut tracker.per_relay_parent.get_mut(&hash).expect("Overseer does not send work items related to relay parents that are not part of our workset. qed");
let job_data = &mut tracker.per_relay_parent.get_mut(&hash).expect("Overseer does not send work items related to relay parents that are not part of our workset. qed");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to be defensive here. The overseer doesn't do deep inspection of messages sent, so a "rogue" or buggy subsystem implementation might send a message with bad or outdated relay-parent hash. Better to just ignore.


let validator = {
per_job
job_data
.validator_set
.get(signed_availability.validator_index() as usize)
.expect("Our own validation index exists. qed")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, again I don't find this to be a convincing proof. It's better to avoid cross-module and cross-crate assumptions for these panic-proofs, because there is nothing stopping a subsystem from sending us a bad piece of data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Garbage-in-garbage-out, but better not to take the node down.

Expand All @@ -158,7 +158,7 @@ impl BitfieldDistribution {
signed_availability,
};

relay_message(&mut ctx, per_job, peer_views, validator, msg)
relay_message(&mut ctx, job_data, peer_views, validator, msg)
.await?;
}
BitfieldDistributionMessage::NetworkBridgeUpdate(event) => {
Expand Down Expand Up @@ -226,7 +226,7 @@ where
/// Can be originated by another subsystem or received via network from another peer.
async fn relay_message<Context>(
ctx: &mut Context,
per_job: &mut PerRelayParentData,
job_data: &mut PerRelayParentData,
peer_views: &mut HashMap<PeerId, View>,
validator: ValidatorId,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validator here is redundant if message_sent_to_peer is altered to hold ValidatorIndex and not ValidatorId. There is a one-to-one mapping between them for any given relay-parent. And the validator index is present within the signed bitfield itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant yes, but from where the message is used, the validator is already extracted and hence error handling is externalised from this fn.

message: BitfieldGossipMessage,
Expand All @@ -247,7 +247,7 @@ where
)
).await;

let message_sent_to_peer = &mut (per_job.message_sent_to_peer);
let message_sent_to_peer = &mut (job_data.message_sent_to_peer);

// concurrently pass on the bitfield distribution to all interested peers
let interested_peers = peer_views
Expand Down Expand Up @@ -403,17 +403,17 @@ where
let delta_set: HashMap<ValidatorId, BitfieldGossipMessage> = delta_vec
.into_iter()
.filter_map(|new_relay_parent_interest| {
if let Some(per_job) = (&*tracker).per_relay_parent.get(&new_relay_parent_interest) {
if let Some(job_data) = (&*tracker).per_relay_parent.get(&new_relay_parent_interest) {
// send all messages
let one_per_validator = per_job.one_per_validator.clone();
let one_per_validator = job_data.one_per_validator.clone();
let origin = origin.clone();
Some(
one_per_validator
.into_iter()
.filter(move |(validator, _message)| {
// except for the ones the peer already has
// let validator = validator.clone();
per_job.message_from_validator_needed_by_peer(&origin, validator)
job_data.message_from_validator_needed_by_peer(&origin, validator)
}),
)
} else {
Expand Down Expand Up @@ -442,14 +442,14 @@ async fn send_tracked_gossip_message<Context>(
where
Context: SubsystemContext<Message = BitfieldDistributionMessage>,
{
let per_job = if let Some(per_job) = tracker.per_relay_parent.get_mut(&message.relay_parent) {
per_job
let job_data = if let Some(job_data) = tracker.per_relay_parent.get_mut(&message.relay_parent) {
job_data
} else {
// @todo punishing here seems unreasonable
return Ok(());
};

let message_sent_to_peer = &mut (per_job.message_sent_to_peer);
let message_sent_to_peer = &mut (job_data.message_sent_to_peer);
message_sent_to_peer
.entry(dest.clone())
.or_default()
Expand Down