Block malicous users by HoOngEe · Pull Request #1434 · CodeChain-io/codechain

HoOngEe · 2019-04-02T12:22:27Z

It closes #1414

core/src/miner/miner.rs

sgkim126 · 2019-04-05T12:50:10Z

And what about the RPCs?

HoOngEe · 2019-04-09T07:55:51Z

I Implemented five RPCs requested in the issue#1414 with the name
account_getBannedAccounts, account_unbanAccounts, account_banAccounts, account_getImmuneAccounts, account_registerImmuneAccounts

sgkim126 · 2019-04-10T10:36:25Z

Why do you implement them on the account while I designed it on the mempool?

HoOngEe · 2019-04-11T08:24:13Z

I changed wrongly implemented account RPCs to intended mempool RPCs.

sgkim126 · 2019-04-15T11:42:52Z

@HoOngEe Please give a reason if your implementation is different from the spec.

HoOngEe · 2019-04-15T11:52:18Z

I renamed the mempool_registerAccounts to mempool_registerImmuneAccounts and mempool_getAccounts to mempool_getImmuneAccounts.I think it helps understanding because the purpose of registering accounts here is to make some accounts immune to banning so that the transactions from those accounts would never be rejected by the reason they are malicious users.

sgkim126 · 2019-04-16T06:30:11Z

LGTM. But let me merge it after the next release.

majecty · 2019-10-02T10:42:13Z

core/src/miner/miner.rs

+                            .read()
+                            .is_local_transaction(hash)
+                            .expect("The tx is clearly fetched from the mem pool") => {}
+                        _ => {


Other errors are safe to ban the error creators?
And what can we do to reduce some bugs when we add a new RuntimeError?

Currently I cannot sure that the left runtime errors are considered as malicious behaviors. It's better to use the blacklist method when some errors are problematic.

I picked the two problematic runtime errors RuntimeError::AssetSupplyOverflow and RuntimeError::InvalidScript and blacklisted them.

@foriequal0 @sgkim126 What do you think about judging a malicious user only if the user's transaction failed with the blacklisted error?

I think it can be fixed later.

spec/JSON-RPC.md

foriequal0 · 2019-10-02T10:51:29Z

core/src/client/client.rs

+        self.importer.miner.register_immune_users(immune_user_vec)
+    }
+
+    fn get_network_id(&self) -> NetworkId {


Network id can be retrieved with EngineInfo::common_params(Latest)?.network_id()

I meant that get_network_id() is unnecessary.

foriequal0 · 2019-10-02T11:06:35Z

core/src/client/client.rs

+        self.importer.miner.get_malicious_users()
+    }
+
+    fn release_trusty_users(&self, trusty_vec: Vec<Address>) {


The objective trusty user is not matching with the malicious user.
It is actually a method that releases a malicious user before.
And it doesn't match with the RPC method name unban_accounts

I changed the name of release_trusty_users to release_malicious_users. For internal function names, I used "imprison" and "release" in a pair unlike the rpc name "ban" and "unban". Hence, I kept the verb "release" here. Is it better to unify the rpc name and the name of the internally called function?

I think unifying it would be better.

Immune users are immune from getting banned.

HoOngEe · 2019-10-10T04:59:30Z

I judged the two runtime errors RuntimeError::AssetSupplyOverflow and RuntimeError::InvalidScrip as malicious ones and blacklisted them.

sgkim126 requested changes Apr 2, 2019

View reviewed changes

core/src/miner/miner.rs Outdated Show resolved Hide resolved

sgkim126 added the mempool label Apr 2, 2019

HoOngEe changed the title ~~[WIP] Ban accounts~~ [WIP] Block malicous users Apr 3, 2019

HoOngEe force-pushed the ban-accounts branch from 8f65a0b to cb43aa8 Compare April 3, 2019 06:29

HoOngEe changed the title ~~[WIP] Block malicous users~~ Block malicous users Apr 3, 2019

HoOngEe force-pushed the ban-accounts branch from cb43aa8 to 1db5d85 Compare April 4, 2019 11:30

HoOngEe force-pushed the ban-accounts branch from 1db5d85 to f67e87a Compare April 9, 2019 07:53

HoOngEe force-pushed the ban-accounts branch from f67e87a to 2796c37 Compare April 9, 2019 07:58

HoOngEe force-pushed the ban-accounts branch from 2796c37 to fe7e569 Compare April 11, 2019 08:21

HoOngEe requested a review from ScarletBlue April 16, 2019 06:11

sgkim126 added the do-not-merge Do not merge (for mergify.io) label Apr 16, 2019

sgkim126 previously approved these changes Apr 17, 2019

View reviewed changes

HoOngEe dismissed sgkim126’s stale review via cb3a553 September 30, 2019 12:20

HoOngEe force-pushed the ban-accounts branch 4 times, most recently from b969477 to 5565f6b Compare October 1, 2019 09:34

HoOngEe requested review from foriequal0 and majecty October 1, 2019 09:35

majecty requested changes Oct 2, 2019

View reviewed changes

foriequal0 requested changes Oct 2, 2019

View reviewed changes

HoOngEe force-pushed the ban-accounts branch 2 times, most recently from b8e9911 to e50b9ed Compare October 4, 2019 07:00

HoOngEe force-pushed the ban-accounts branch from e50b9ed to 6c0e410 Compare October 4, 2019 08:27

Seonpyo Kim and others added 6 commits October 10, 2019 13:56

Fix typos

40e901e

Block malicious users who generated runtime errors

d33584e

Block malicious users who generated syntax errors

2485827

Add a new type of users called immune users

415a845

Immune users are immune from getting banned.

Implement new RPCs related banned users

2908d9e

Add newly implemented RPCs to spec file

a11c338

HoOngEe force-pushed the ban-accounts branch from 6c0e410 to a11c338 Compare October 10, 2019 04:57

HoOngEe requested review from foriequal0 and majecty October 10, 2019 05:00

majecty approved these changes Oct 11, 2019

View reviewed changes

foriequal0 approved these changes Oct 12, 2019

View reviewed changes

Merge branch 'master' into ban-accounts

4731002

HoOngEe merged commit 3d2afd7 into CodeChain-io:master Oct 14, 2019

HoOngEe deleted the ban-accounts branch October 14, 2019 06:17

foriequal0 mentioned this pull request Nov 8, 2019

Bump the version to 2.2.0 #1853

Merged

Comments

Conversation

HoOngEe commented Apr 2, 2019

Uh oh!

Uh oh!

sgkim126 commented Apr 5, 2019

Uh oh!

HoOngEe commented Apr 9, 2019

Uh oh!

sgkim126 commented Apr 10, 2019

Uh oh!

HoOngEe commented Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgkim126 commented Apr 15, 2019

Uh oh!

HoOngEe commented Apr 15, 2019

Uh oh!

sgkim126 commented Apr 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HoOngEe commented Oct 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HoOngEe commented Apr 11, 2019 •

edited

Loading