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

Add Transaction Permissioning Hook to PermissioningService Interface #7952

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
1724426
commit
vaidikcode Nov 27, 2024
858c8ee
addressing comments
vaidikcode Nov 28, 2024
dd498e7
updating api hash
vaidikcode Nov 28, 2024
aae1526
fixing the issue of implementing the permission service class in pr w…
vaidikcode Nov 28, 2024
2509d7d
add tests
vaidikcode Nov 28, 2024
6c7b463
change log
vaidikcode Nov 28, 2024
4a0c5dd
fix
vaidikcode Nov 28, 2024
2d51ec2
fix
vaidikcode Nov 28, 2024
7851707
Merge remote-tracking branch 'origin/#7835' into #7835
vaidikcode Nov 28, 2024
da2c3de
fix
vaidikcode Nov 28, 2024
dd44cca
fix
vaidikcode Nov 28, 2024
549db1f
fix
vaidikcode Dec 3, 2024
7f75593
Merge branch 'hyperledger:main' into #7835
vaidikcode Dec 3, 2024
d77f294
refactor
vaidikcode Dec 3, 2024
245e8ef
Merge remote-tracking branch 'origin/#7835' into #7835
vaidikcode Dec 3, 2024
b51669b
Merge branch 'main' into #7835
vaidikcode Dec 4, 2024
94620ed
fix
vaidikcode Dec 9, 2024
531ff5d
Merge remote-tracking branch 'origin/#7835' into #7835
vaidikcode Dec 9, 2024
ae8f3c8
Merge branch 'main' into #7835
vaidikcode Dec 9, 2024
5d815c2
spotless
vaidikcode Dec 10, 2024
9cfc510
Merge remote-tracking branch 'origin/#7835' into #7835
vaidikcode Dec 10, 2024
80c832c
compile error fix
vaidikcode Dec 11, 2024
b74a3e1
Merge branch 'main' into #7835
macfarla Dec 11, 2024
0b23b89
minor fix
vaidikcode Dec 11, 2024
de4ee35
Merge remote-tracking branch 'origin/#7835' into #7835
vaidikcode Dec 11, 2024
040857a
minor fix
vaidikcode Dec 11, 2024
a037c07
final
macfarla Dec 11, 2024
028c103
Merge branch '#7835' of github.com:vaidikcode/besu into #7835
macfarla Dec 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package org.hyperledger.besu.services;

import org.hyperledger.besu.ethereum.permissioning.account.TransactionPermissioningProvider;
import org.hyperledger.besu.plugin.services.PermissioningService;
import org.hyperledger.besu.plugin.services.permissioning.NodeConnectionPermissioningProvider;
import org.hyperledger.besu.plugin.services.permissioning.NodeMessagePermissioningProvider;
import org.hyperledger.besu.ethereum.core.Transaction;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.ArrayList;
import java.util.List;

public class PermissionServiceImpl implements PermissioningService {
private static final Logger log = LoggerFactory.getLogger(PermissionServiceImpl.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private static final Logger log = LoggerFactory.getLogger(PermissionServiceImpl.class);
private static final Logger LOG = LoggerFactory.getLogger(PermissionServiceImpl.class);

Copy link
Contributor

Choose a reason for hiding this comment

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

all upper case naming convention for static fields

private final List<TransactionPermissioningProvider> transactionPermissioningProviders = new ArrayList<>();

@Override
public void registerNodePermissioningProvider(NodeConnectionPermissioningProvider provider) {

}

@Override
public void registerTransactionPermissioningProvider(TransactionPermissioningProvider provider) {
transactionPermissioningProviders.add(provider);
log.info("Registered new transaction permissioning provider.");
}

@Override
public void registerNodeMessagePermissioningProvider(NodeMessagePermissioningProvider provider) {

}

public boolean isTransactionPermitted(Transaction transaction) {
for (TransactionPermissioningProvider provider : transactionPermissioningProviders) {
if (!provider.isPermitted(transaction)) {
log.info("Transaction {} not permitted by one of the providers.", transaction.getHash());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log.info("Transaction {} not permitted by one of the providers.", transaction.getHash());
log.debug("Transaction {} not permitted by one of the providers.", transaction.getHash());

return false;
}
}
log.info("Transaction {} permitted.", transaction.getHash());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log.info("Transaction {} permitted.", transaction.getHash());
log.debug("Transaction {} permitted.", transaction.getHash());

Copy link
Contributor

Choose a reason for hiding this comment

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

this would be spammy at info level

return true;
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*/
package org.hyperledger.besu.plugin.services;

import org.hyperledger.besu.ethereum.permissioning.account.TransactionPermissioningProvider;
Copy link
Contributor

Choose a reason for hiding this comment

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

will need some refactoring to move this TransactionPermissioningProvider interface into the plugin package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback! Are you referring to this location: plugin-api/src/main/java/org/hyperledger/besu/plugin/services/permissioning/?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a quick question: Since the TransactionPermissioningProvider is implemented by both the AccountLocalConfigPermissioningController and the TransactionSmartContractPermissioningController, what would be the best way to move forward with this? Your guidance would be greatly appreciated!

Copy link
Contributor

Choose a reason for hiding this comment

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

I would have a look at how NodeMessagePermissioningProvider (functional interface) is used, since that's already exposed to plugins and used elsewhere in besu. It's a common pattern in besu to have an interface in the plugin-api package which is imported and implemented by classes where needed. BlockHeader is another good example but you can find lots of examples in the plugin-api package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There seems to be a circular dependency issue caused by importing the Transaction class into the TransactionPermissioningProvider interface. introducing a lightweight abstraction (like a DTO or a subset of transaction properties) for permissioning logic might be a good solution. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

does it help if the TransactionPermissioningProvider uses the org.hyperledger.besu.datatypes.Transaction interface instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much. It works perfectly!.

import org.hyperledger.besu.plugin.services.permissioning.NodeConnectionPermissioningProvider;
import org.hyperledger.besu.plugin.services.permissioning.NodeMessagePermissioningProvider;

Expand All @@ -38,6 +39,13 @@ public interface PermissioningService extends BesuService {
*/
void registerNodePermissioningProvider(NodeConnectionPermissioningProvider provider);

/**
* Registers a callback for transaction permission.
*
* @param provider The provider to register
*/
void registerTransactionPermissioningProvider(TransactionPermissioningProvider provider);

/**
* Registers a callback to allow the interception of a devp2p message sending request
*
Expand Down