Nc 1942 Add account whitelisting and refactor into permissioning package #460

Shinabyss · 2018-12-19T06:19:29Z

PR description

Create account whitelisting serving as the groundwork for role based permissioning system for transactions

Include:

"--accounts-whitelisting" config on startup
logic to reject transaction on validation when using account whitelist is active
Unit testing
refactor account whitelisting from p2p to permissioning package

Fixed Issue(s)

…NC-1361

…NC-1942 # Conflicts: # pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java # pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java Resolved

…g package

…NC-1942 # Conflicts: # consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftControllerTest.java # docs/Getting-Started/ExplorerBlockDetails.png # docs/Getting-Started/ExplorerSearch.png # docs/Getting-Started/ExplorerSummary.png # ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/PermissioningConfiguration.java # ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/PermissioningConfigurationTest.java # pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java # pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java

…NC-1942

lucassaldanha · 2018-12-19T08:01:01Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionPool.java

@@ -131,6 +134,16 @@ public PendingTransactions getPendingTransactions() {
      return basicValidationResult;
    }

+    if (accountWhitelistController.isPresent()


It would be good to have this whole logic inside a private method with a meaningful name like checkIfAccountIsWhitelisted or something similar.

ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/core/TransactionPoolTest.java

lucassaldanha · 2018-12-19T09:29:58Z

pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java

+    AccountWhitelistController accountWhitelistController =
+        new AccountWhitelistController(permissioningConfiguration);
+
+    transactionPool.setAccountWhitelist(accountWhitelistController);


We don't need to call the setter if the account whitelist property hasn't been set. Why don't we check it here?

if (permissioningConfiguration.isAccountWhitelistSet()) { transactionPool.setAccountWhitelist(accountWhitelistController); }

…NC-1942

macfarla · 2018-12-20T01:21:57Z

pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java

@@ -404,13 +404,24 @@ private void setRefreshDelay(final Long refreshDelay) {
    names = {"--nodes-whitelist"},
    paramLabel = "<enode://id@host:port>",
    description =
-        "Comma separated enode URLs for permissioned networks. Not intended to be used with mainnet or public testnets.",
+        "Comma separated enode URLs for permissioned networks. You may specify an empty list.",


You've reverted a text change here - can you change it back?

macfarla · 2018-12-20T01:22:36Z

pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java

+    description =
+        "Comma separated hexString of account public key "
+            + "for permissioned/role-based transaction. You may specify an empty list.",
+    split = ",",


s/transaction/transactions

macfarla · 2018-12-20T01:22:53Z

pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java

    split = ",",
    arity = "0..*",
    converter = EnodeToURIPropertyConverter.class
  )
  private final Collection<URI> nodesWhitelist = null;

+  @Option(
+    names = {"--accounts-whitelist"},
+    paramLabel = "<hexString of account public key>",


…NC-1942

lucassaldanha · 2018-12-20T01:53:26Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionPool.java

@@ -131,6 +134,13 @@ public PendingTransactions getPendingTransactions() {
      return basicValidationResult;
    }

+    String sender = transaction.getSender().toString();
+    if (!checkIfAccountIsWhitelisted(sender)) {


Now that I read this, I think the if statement will be better as something like:

if (!accountIsWhitelisted(sender)) {

What do you think?

Or maybe even accountIsNotWhitelisted(sender)) so we don't need to negate the expression... idk

lucassaldanha

It is looking good now. I've added another comment re readability and Sally has got some comments herself that I believe it would be good to implement.

…NC-1942

…ssioning package

…NC-1942

Shinabyss added 28 commits December 4, 2018 12:36

WS sync subscription delay added

334ced0

Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …

7b52fd4

…NC-1361

WS sync subscription delay added with unit testing

4acf4cf

Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …

0d517ba

…NC-1361

WS sync subscription delay added with unit testing

1db0792

Merge branch 'master' into NC-1361

189b25a

Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …

462536b

…NC-1361

changed number to a constant in constructor

3295b21

Merge remote-tracking branch 'origin/NC-1361' into NC-1361

609cf0c

Use default from websocket class instead of making new one

ae5e2ce

Merge branch 'master' into NC-1361

7b90fea

Merge branch 'master' into NC-1361

931af28

Removed magic numbers

113ba99

Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …

f8dcba8

…NC-1361

Merge remote-tracking branch 'origin/NC-1361' into NC-1361

35d3e7d

Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …

00497f2

…NC-1361

Added Controller

d7e5519

Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …

011b93d

…NC-1942 # Conflicts: # pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java # pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java Resolved

Refactor permissioning config and account whitelist into permissionin…

3971892

…g package

Refactor permissioning config and account whitelist into permissionin…

677f511

…g package

Refactor permissioning config and account whitelist into permissionin…

5ac5f04

…g package

Iron out merge conflict introduced bugs

e270cf9

Iron out merge conflict introduced bugs

d94d186

Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …

6d65170

…NC-1942

Iron out merge conflict introduced bugs

54e5235

Shinabyss self-assigned this Dec 19, 2018

Shinabyss requested a review from lucassaldanha December 19, 2018 06:19

Shinabyss requested a review from macfarla December 19, 2018 06:19

Shinabyss changed the title ~~Nc 1942~~ Nc 1942 Add account whitelisting and refactor into permissioning package Dec 19, 2018

lucassaldanha suggested changes Dec 19, 2018

View reviewed changes

Shinabyss added 4 commits December 20, 2018 00:20

PR Change request actioned

2cbd358

Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …

f388b41

…NC-1942

PR Change request actioned

8e5b91e

Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …

1690f21

…NC-1942

macfarla reviewed Dec 20, 2018

View reviewed changes

Shinabyss added 3 commits December 20, 2018 11:30

PR Change request actioned

1cf498e

Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …

0715b2d

…NC-1942

Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …

1e57049

…NC-1942

lucassaldanha reviewed Dec 20, 2018

View reviewed changes

lucassaldanha approved these changes Dec 20, 2018

View reviewed changes

Shinabyss added 5 commits December 20, 2018 12:14

PR Change request actioned

2e9804d

Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …

3b6f92b

…NC-1942

updated node whitelist acceptance test to conform to refactored permi…

26f83ed

…ssioning package

Merge branch 'master' of https://github.com/PegaSysEng/pantheon into …

bc1f782

…NC-1942

text change

bacf2cb

Shinabyss merged commit 6b60370 into PegaSysEng:master Dec 20, 2018

Shinabyss deleted the NC-1942 branch December 20, 2018 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nc 1942 Add account whitelisting and refactor into permissioning package #460

Nc 1942 Add account whitelisting and refactor into permissioning package #460

Shinabyss commented Dec 19, 2018

lucassaldanha Dec 19, 2018

Shinabyss Dec 19, 2018

lucassaldanha Dec 19, 2018

Shinabyss Dec 19, 2018

macfarla Dec 20, 2018

Shinabyss Dec 20, 2018

macfarla Dec 20, 2018

Shinabyss Dec 20, 2018

macfarla Dec 20, 2018

Shinabyss Dec 20, 2018

lucassaldanha Dec 20, 2018

lucassaldanha Dec 20, 2018 •

edited

Loading

Shinabyss Dec 20, 2018

lucassaldanha left a comment

Nc 1942 Add account whitelisting and refactor into permissioning package #460

Nc 1942 Add account whitelisting and refactor into permissioning package #460

Conversation

Shinabyss commented Dec 19, 2018

PR description

Fixed Issue(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucassaldanha Dec 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucassaldanha left a comment

Choose a reason for hiding this comment

lucassaldanha Dec 20, 2018 •

edited

Loading