Skip to content
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

Added NodeSmartContractV2PermissioningController #1435

Merged
merged 7 commits into from
Oct 19, 2020

Conversation

lucassaldanha
Copy link
Member

@lucassaldanha lucassaldanha commented Oct 8, 2020

PR description

  • Created AbstractNodeSmartContractPermissioningController that contains some of the shared functionality between all smart contract-based node permissioning providers.
  • Added NodeSmartContractV2PermissioningController that conforms with the updated Node Permissioning EEA interface.
  • Created a new config in SmartContractPermissioningConfiguration that is used in NodePermissioningControllerFactory to choose the right smart contract permissioning provider (EEA v1 or v2) - currently it defaults to 1 (EEA v1 interface).
  • Created CLI option that will set the correct value on SmartContractPermissioningConfiguration (nodeSmartContractInterfaceVersion).
  • Tests :)

Note about EEA compatibility:
It looks like the final change on the return type from byte32 to bool hasn't been accepted yet. However, I believe it is likely to be. The discussion can be seen here: https://github.com/EntEthAlliance/client-spec/issues/902

Changelog

@lucassaldanha lucassaldanha added TeamRevenant GH issues worked on by Revenant Team permissioning account or node permissioning labels Oct 8, 2020
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

looks good overall

private boolean isPermitted(final EnodeURL enode) {
final Bytes payload = createPayload(enode);
final CallParameter callParams =
new CallParameter(null, contractAddress, -1, null, null, payload);
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the -1 value?

Copy link
Member Author

Choose a reason for hiding this comment

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

the gas limit. It isn't relevant for the simulation call.

@lucassaldanha lucassaldanha force-pushed the node-perm-update branch 5 times, most recently from fda1b7c to 62b258a Compare October 15, 2020 05:51
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
@lucassaldanha lucassaldanha marked this pull request as ready for review October 16, 2020 01:17
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

LGTM. A few small comments

CHANGELOG.md Outdated
@@ -4,6 +4,7 @@
* `--random-peer-priority-enabled` flag added. Allows for incoming connections to be prioritized randomly. This will prevent (typically small, stable) networks from forming impenetrable peer cliques. [#1440](https://github.com/hyperledger/besu/pull/1440)
* Hide deprecated `--host-whitelist` option. [\#1444](https://github.com/hyperledger/besu/pull/1444)
* Prioritize high gas prices during mining. Previously we ordered only by the order in which the transactions were received. This will increase expected profit when mining. [\#1449](https://github.com/hyperledger/besu/pull/1449)
* Added support for the updated smart contract-based node permissioning EEA interface. [\#1435](https://github.com/hyperledger/besu/pull/1435)
Copy link
Contributor

Choose a reason for hiding this comment

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

is there an EEA version to link to?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I'll update

final byte[] ip = NodeSmartContractV2PermissioningController.encodeIp(enodeUrl.getIp());
final int port = enodeUrl.getListeningPortOrZero();

final Function addNodeFunction =
Copy link
Contributor

Choose a reason for hiding this comment

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

connectionAllowedFunction?

final byte[] ip = NodeSmartContractV2PermissioningController.encodeIp(enodeUrl.getIp());
final int port = enodeUrl.getListeningPortOrZero();

final Function addNodeFunction =
Copy link
Contributor

Choose a reason for hiding this comment

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

removeNodeFunction


permissionedNode.verify(connectionIsAllowed(allowedNode));
permissionedNode.verify(connectionIsAllowed(bootnode));
permissionedNode.verify(connectionIsAllowed(permissionedNode));
Copy link
Contributor

Choose a reason for hiding this comment

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

is it worth adding verify(connectionNotAllowed(forbiddenNode))

extends AbstractNodeSmartContractPermissioningController {

public static final Bytes TRUE_RESPONSE = Bytes.fromHexString(TypeEncoder.encode(new Bool(true)));
public static Bytes FALSE_RESPONSE = Bytes.fromHexString(TypeEncoder.encode(new Bool(false)));
Copy link
Contributor

Choose a reason for hiding this comment

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

final

public void before() {
when(transactionSimulator.doesAddressExistAtHead(any())).thenReturn(Optional.of(true));
}

Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed? no other changes in this test

Copy link
Member Author

Choose a reason for hiding this comment

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

It is! because now the existence of the contract is tested a bit earlier in the code. Without this, all tests fail :)

Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
permissioning account or node permissioning TeamRevenant GH issues worked on by Revenant Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants