-
Notifications
You must be signed in to change notification settings - Fork 51
Implement reportDoubleVote custom action #1640
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
Implement reportDoubleVote custom action #1640
Conversation
6e77495
to
2ac4e10
Compare
@HoOngEe What is the current status of this PR? |
@majecty I need someone to review it. |
@HoOngEe Please fix the conflicts. |
9ec2703
to
e375c1a
Compare
@majecty I resolved the conflicts |
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.
What happened to the delegated CCS of the banned validator.
core/src/consensus/stake/actions.rs
Outdated
)) | ||
})?; | ||
let signer1 = signers | ||
.get(signer_idx1) |
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.
The signer may be different from the message's signer. We should get the prev term's signers.
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.
Fixed
core/src/consensus/stake/mod.rs
Outdated
if let Some(criminal_info) = candidates.get_candidate(&criminal).map(Clone::clone) { | ||
candidates.remove(&criminal); | ||
if let Some(informant_info) = candidates.get_candidate(&public_to_address(informant)).map(Clone::clone) { | ||
candidates.add_deposit( |
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.
Why give CCC as a deposit?
Any reason for this? How about give CCC directly to the informant?
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.
Yes you are right, I misread the spec
2f4e11e
to
73fae8f
Compare
@foriequal0 I remember you mentioned about the expected compile error
at the line |
I have almost done. But I would better to change the error handling policy on "Cannot get validators from the parent state" part. |
906d18e
to
1ae58ea
Compare
Now I'll change the error types which would be returned by the |
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.
LGTM
edd8f8e
to
0224395
Compare
@foriequal0 Could you check this PR again? |
d0fface
to
5ccd56f
Compare
core/src/consensus/stake/actions.rs
Outdated
parent_block_hash, | ||
})?; | ||
|
||
if signer1 != signer2 { |
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.
(Question) Do we have a chance to have the same signer with two different signer indexes?
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.
So sharp. I'll change the code like this:
First compare signer_idx1
and signer_idx2
. If they are different, invoke an error.
Then, I'll get signer public key without collecting all pubkeys in vector.
34085c5
to
b319f60
Compare
I now need to reject reportDoubleVote for already banned account. |
16eb793
to
d6492aa
Compare
core/src/consensus/stake/mod.rs
Outdated
@@ -414,12 +443,13 @@ pub fn jail(state: &mut TopLevelState, addresses: &[Address], custody_until: u64 | |||
} | |||
|
|||
#[allow(dead_code)] |
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's remove #[allow(dead_code)]
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 removed it.
a62db10
to
cadfeea
Compare
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.
LGTM
cadfeea
to
3a34328
Compare
3a34328
to
848fa8a
Compare
pub fn verify( | ||
&self, | ||
current_params: &CommonParams, | ||
client: Option<Arc<ConsensusClient>>, |
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.
You could've accepted just &ConsensusClient
. Anyway, We'll ship it now and fix it later.
No description provided.