-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[DONTMERGE] Add debug logs and make approval voting failable #3783
base: master
Are you sure you want to change the base?
Changes from 6 commits
d12cde4
1f898c7
10fffc6
bf75f08
65f7767
3333849
175ed47
ac9f924
e10d910
1e357f1
4cc2878
c37096d
84b3422
bb7bb2f
dfbc50b
ed71d2d
22bd95a
cd30b94
b2e7087
0c4a781
733f108
3d135fe
106c876
050689b
5764bb4
61f2c13
0789bb9
c663ebd
91e5eaa
dbf8b8b
f924252
8650564
46551d1
dd1b115
d486616
bca88ea
e4f5fbe
0b63b3a
cac79f1
ccdebd0
ef8591f
b03e1bb
3a08e2b
06dd295
dade923
2cd9542
bff00af
b917461
a4fb3d6
072019b
aac3683
847bbb8
5d2ee2c
3779292
c0d7044
4f96620
543eec0
e193014
a503e70
c6a6876
5156917
d4ce775
95db2e2
a2cc915
a12a544
280f16e
217100c
d231167
d194180
4d11c41
cea63de
a629ea4
29989a2
10e09a2
4c0887a
e0efa29
d1ae091
dd84aa8
f802fd4
b223987
05ff8d4
7641d6c
ca490a1
f7d997d
e63e448
c6a16d1
58726b9
d6b1965
cdfd5be
9dd8afa
8664023
56a6e68
97879a8
22a7690
cd4b791
a9414ee
61de01c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ authors = ["Parity Technologies <[email protected]>"] | |
edition = "2018" | ||
|
||
[dependencies] | ||
rand = "0.5" | ||
futures = "0.3.17" | ||
futures-timer = "3.0.2" | ||
parity-scale-codec = { version = "2.0.0", default-features = false, features = ["bit-vec", "derive"] } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,9 @@ | |
//! of others. It uses this information to determine when candidates and blocks have | ||
//! been sufficiently approved to finalize. | ||
|
||
use rand::distributions::{Distribution, Normal}; | ||
use std::sync::atomic::{AtomicU64, Ordering}; | ||
|
||
use kvdb::KeyValueDB; | ||
use polkadot_node_jaeger as jaeger; | ||
use polkadot_node_primitives::{ | ||
|
@@ -97,6 +100,9 @@ const APPROVAL_CHECKING_TIMEOUT: Duration = Duration::from_secs(120); | |
const APPROVAL_CACHE_SIZE: usize = 1024; | ||
const TICK_TOO_FAR_IN_FUTURE: Tick = 20; // 10 seconds. | ||
const LOG_TARGET: &str = "parachain::approval-voting"; | ||
const DEBUG_LOG_TARGET: &str = "parachain::ladi-debug-approval-voting"; | ||
const MALICIOUS_MEAN: f64 = 2_500f64; | ||
const MALICIOUS_SDEV: f64 = 70f64; | ||
|
||
/// Configuration for the approval voting subsystem | ||
#[derive(Debug, Clone)] | ||
|
@@ -342,12 +348,15 @@ where | |
{ | ||
fn start(self, ctx: Context) -> SpawnedSubsystem { | ||
let backend = DbBackend::new(self.db.clone(), self.db_config); | ||
let normal = Normal::new(MALICIOUS_MEAN, MALICIOUS_SDEV); | ||
let offset: u64 = normal.sample(&mut rand::thread_rng()).round() as u64; | ||
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. I'd recommend adding |
||
let future = run::<DbBackend, Context>( | ||
ctx, | ||
self, | ||
Box::new(SystemClock), | ||
Box::new(RealAssignmentCriteria), | ||
backend, | ||
offset, | ||
) | ||
.map_err(|e| SubsystemError::with_origin("approval-voting", e)) | ||
.boxed(); | ||
|
@@ -479,8 +488,9 @@ struct ApprovalStatus { | |
block_tick: Tick, | ||
} | ||
|
||
#[derive(Copy, Clone)] | ||
#[derive(Clone)] | ||
enum ApprovalOutcome { | ||
Malicious((CandidateReceipt, SessionIndex)), | ||
Approved, | ||
Failed, | ||
TimedOut, | ||
|
@@ -499,6 +509,18 @@ impl ApprovalState { | |
fn failed(validator_index: ValidatorIndex, candidate_hash: CandidateHash) -> Self { | ||
Self { validator_index, candidate_hash, approval_outcome: ApprovalOutcome::Failed } | ||
} | ||
fn malicious( | ||
validator_index: ValidatorIndex, | ||
candidate_hash: CandidateHash, | ||
candidate: CandidateReceipt, | ||
session: SessionIndex, | ||
) -> Self { | ||
Self { | ||
validator_index, | ||
candidate_hash, | ||
approval_outcome: ApprovalOutcome::Malicious((candidate, session)), | ||
} | ||
} | ||
} | ||
|
||
struct CurrentlyCheckingSet { | ||
|
@@ -663,6 +685,7 @@ async fn run<B, Context>( | |
clock: Box<dyn Clock + Send + Sync>, | ||
assignment_criteria: Box<dyn AssignmentCriteria + Send + Sync>, | ||
mut backend: B, | ||
offset: u64, | ||
) -> SubsystemResult<()> | ||
where | ||
Context: SubsystemContext<Message = ApprovalVotingMessage>, | ||
|
@@ -680,6 +703,7 @@ where | |
let mut wakeups = Wakeups::default(); | ||
let mut currently_checking_set = CurrentlyCheckingSet::default(); | ||
let mut approvals_cache = lru::LruCache::new(APPROVAL_CACHE_SIZE); | ||
let counter = AtomicU64::new(0); | ||
|
||
let mut last_finalized_height: Option<BlockNumber> = None; | ||
|
||
|
@@ -727,26 +751,42 @@ where | |
} | ||
) = approval_state; | ||
|
||
if matches!(approval_outcome, ApprovalOutcome::Approved) { | ||
let mut approvals: Vec<Action> = relay_block_hashes | ||
.into_iter() | ||
.map(|block_hash| | ||
Action::IssueApproval( | ||
candidate_hash, | ||
ApprovalVoteRequest { | ||
validator_index, | ||
block_hash, | ||
}, | ||
match approval_outcome { | ||
ApprovalOutcome::Approved => { | ||
let mut approvals: Vec<Action> = relay_block_hashes | ||
.into_iter() | ||
.map(|block_hash| | ||
Action::IssueApproval( | ||
candidate_hash, | ||
ApprovalVoteRequest { | ||
validator_index, | ||
block_hash, | ||
}, | ||
) | ||
) | ||
) | ||
.collect(); | ||
actions.append(&mut approvals); | ||
.collect(); | ||
actions.append(&mut approvals); | ||
} | ||
ApprovalOutcome::Malicious((candidate, session_index)) => { | ||
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. This seems like a lot of extra machinery to add instead of just sending the message from within the 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. I was debating whether or not to do it simply with the I realize we aren't going to be re-using this code, but it wasn't a large change. Will keep in mind to keep it simple going forward. |
||
let sender = ctx.sender(); | ||
sender | ||
.send_message( | ||
DisputeCoordinatorMessage::IssueLocalStatement( | ||
session_index, | ||
candidate_hash, | ||
candidate, | ||
false, | ||
) | ||
.into(), | ||
) | ||
.await; | ||
} | ||
_ => {}, | ||
} | ||
|
||
actions | ||
} | ||
}; | ||
|
||
if handle_actions( | ||
&mut ctx, | ||
&mut state, | ||
|
@@ -757,6 +797,7 @@ where | |
&mut approvals_cache, | ||
&mut subsystem.mode, | ||
actions, | ||
if counter.fetch_add(1, Ordering::SeqCst) == offset { true } else { false }, | ||
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. nit: the if case can be dropped :) just use the boolean expression 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. I added logging in these two cases, so for now I'll leave it as an if-statement. I'll keep the logging for deploying to one node but can drop the logging before deploying to all validators. |
||
) | ||
.await? | ||
{ | ||
|
@@ -804,6 +845,7 @@ async fn handle_actions( | |
approvals_cache: &mut lru::LruCache<CandidateHash, ApprovalOutcome>, | ||
mode: &mut Mode, | ||
actions: Vec<Action>, | ||
malicious: bool, | ||
) -> SubsystemResult<bool> { | ||
let mut conclude = false; | ||
|
||
|
@@ -892,6 +934,7 @@ async fn handle_actions( | |
validator_index, | ||
block_hash, | ||
backing_group, | ||
malicious, | ||
) | ||
.await | ||
}, | ||
|
@@ -2079,6 +2122,7 @@ async fn launch_approval( | |
validator_index: ValidatorIndex, | ||
block_hash: Hash, | ||
backing_group: GroupIndex, | ||
malicious: bool, | ||
) -> SubsystemResult<RemoteHandle<ApprovalState>> { | ||
let (a_tx, a_rx) = oneshot::channel(); | ||
let (code_tx, code_rx) = oneshot::channel(); | ||
|
@@ -2227,7 +2271,26 @@ async fn launch_approval( | |
let expected_commitments_hash = candidate.commitments_hash; | ||
if commitments.hash() == expected_commitments_hash { | ||
let _ = metrics_guard.take(); | ||
return ApprovalState::approved(validator_index, candidate_hash) | ||
if !malicious { | ||
tracing::debug!( | ||
target: DEBUG_LOG_TARGET, | ||
"ladi-debug-approval APPROVED {:?}", | ||
candidate_hash, | ||
); | ||
return ApprovalState::approved(validator_index, candidate_hash) | ||
} else { | ||
tracing::debug!( | ||
target: DEBUG_LOG_TARGET, | ||
"ladi-debug-approval FAILED {:?}", | ||
candidate_hash | ||
); | ||
return ApprovalState::malicious( | ||
validator_index, | ||
candidate_hash, | ||
candidate.clone(), | ||
session_index, | ||
) | ||
} | ||
} else { | ||
// Commitments mismatch - issue a dispute. | ||
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.
nit:
.._STDDEV
is the common abbreviation.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.
Yeah, I wanted MEAN and SDEV to have the same length :)