Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
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
2 changes: 1 addition & 1 deletion node/core/dispute-coordinator/src/initialized.rs
Original file line number Diff line number Diff line change
Expand Up @@ -916,7 +916,7 @@ impl Initialized {
} else {
self.metrics.on_queued_best_effort_participation();
}
let request_timer = Arc::new(self.metrics.time_participation_pipeline());
let request_timer = self.metrics.time_participation_pipeline();
let r = self
.participation
.queue_participation(
Expand Down
2 changes: 1 addition & 1 deletion node/core/dispute-coordinator/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ impl DisputeCoordinatorSubsystem {
?candidate_hash,
"Found valid dispute, with no vote from us on startup - participating."
);
let request_timer = Arc::new(self.metrics.time_participation_pipeline());
let request_timer = self.metrics.time_participation_pipeline();
participation_requests.push((
ParticipationPriority::with_priority_if(is_included),
ParticipationRequest::new(
Expand Down
27 changes: 21 additions & 6 deletions node/core/dispute-coordinator/src/participation/queues/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
// You should have received a copy of the GNU General Public License
// along with Polkadot. If not, see <http://www.gnu.org/licenses/>.

use std::{cmp::Ordering, collections::BTreeMap, sync::Arc};
use std::{cmp::Ordering, collections::BTreeMap};

use futures::channel::oneshot;
use polkadot_node_subsystem::{messages::ChainApiMessage, overseer};
Expand Down Expand Up @@ -65,12 +65,12 @@ pub struct Queues {
}

/// A dispute participation request that can be queued.
#[derive(Debug, Clone)]
#[derive(Debug)]
pub struct ParticipationRequest {
candidate_hash: CandidateHash,
candidate_receipt: CandidateReceipt,
session: SessionIndex,
_request_timer: Arc<Option<prometheus::HistogramTimer>>, // Sends metric data when request is dropped
_request_timer: Option<prometheus::HistogramTimer>, // Sends metric data when request is dropped
}

/// Whether a `ParticipationRequest` should be put on best-effort or the priority queue.
Expand Down Expand Up @@ -117,7 +117,7 @@ impl ParticipationRequest {
pub fn new(
candidate_receipt: CandidateReceipt,
session: SessionIndex,
request_timer: Arc<Option<prometheus::HistogramTimer>>,
request_timer: Option<prometheus::HistogramTimer>,
) -> Self {
Self {
candidate_hash: candidate_receipt.hash(),
Expand All @@ -142,8 +142,8 @@ impl ParticipationRequest {
}
}

// We want to compare participation requests in unit tests, so we
// only implement Eq for tests.
// We want to compare and clone participation requests in unit tests, so we
// only implement Eq and Clone for tests.
#[cfg(test)]
impl PartialEq for ParticipationRequest {
fn eq(&self, other: &Self) -> bool {
Expand All @@ -160,6 +160,21 @@ impl PartialEq for ParticipationRequest {
}
#[cfg(test)]
impl Eq for ParticipationRequest {}
#[cfg(test)]
impl Clone for ParticipationRequest {
Copy link
Member

Choose a reason for hiding this comment

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

I would just not have implemented Clone at all. This is a bit hacky. Instead of cloning, we should be able to create new instances in tests where necessary. E.g. just a new let ... = create_test_partitipation_request ... instead of cloning the previous one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and tried your suggestion. It turns out there's some non-determinism in the helper make_participation_request() such that the candidate hashes end up different. Particularly the signature field in the CandidateDescriptor within the CandidateReceipt is different each time it is generated.

This means that calling make_participation_request() multiple times results in requests that don't compare as equal, ruining our asserts.

Know of other ways we can do this without clone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or here's another idea which might be considered better form.

I could at least move the clone functionality into tests.rs as a helper function rather than implementing it on the ParticipationRequest type.

Copy link
Member

Choose a reason for hiding this comment

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

yes, making it a standalone function like test_clone core something would also be way better. The non-determinism is interesting, I don't see where it would be coming from?

fn clone(&self) -> Self {
ParticipationRequest {
candidate_receipt: self.candidate_receipt.clone(),
candidate_hash: self.candidate_hash.clone(),
session: self.session,
_request_timer: None,
}
}

fn clone_from(&mut self, source: &Self) {
*self = source.clone();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. My understanding is that I need to provide this function as a part of the Clone implementation. Is there a better way to handle these unused but required parts of a type?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see it's a part of the Clone trait! I've never seen it before. But it doesn't seem to be a required method, so you can remove it. The doc for it is interesting:

a.clone_from(&b) is equivalent to a = b.clone() in functionality, but can be overridden to reuse the resources of a to avoid unnecessary allocations.

}

impl Queues {
/// Create new `Queues`.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ use crate::{metrics::Metrics, ParticipationPriority};
use ::test_helpers::{dummy_candidate_receipt, dummy_hash};
use assert_matches::assert_matches;
use polkadot_primitives::{BlockNumber, Hash};
use std::sync::Arc;

use super::{CandidateComparator, ParticipationRequest, QueueError, Queues};

Expand All @@ -27,7 +26,7 @@ fn make_participation_request(hash: Hash) -> ParticipationRequest {
let mut receipt = dummy_candidate_receipt(dummy_hash());
// make it differ:
receipt.commitments_hash = hash;
let request_timer = Arc::new(Metrics::default().time_participation_pipeline());
let request_timer = Metrics::default().time_participation_pipeline();
ParticipationRequest::new(receipt, 1, request_timer)
}

Expand Down
2 changes: 1 addition & 1 deletion node/core/dispute-coordinator/src/participation/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ async fn participate_with_commitments_hash<Context>(
};
let session = 1;

let request_timer = Arc::new(participation.metrics.time_participation_pipeline());
let request_timer = participation.metrics.time_participation_pipeline();
let req = ParticipationRequest::new(candidate_receipt, session, request_timer);

participation
Expand Down