Add Transaction Permissioning Hook to PermissioningService Interface #7952

vaidikcode · 2024-11-27T20:50:52Z

Signed-off-by: vaidikcode vaidikbhardwaj00@gmail.com

PR description

I’ve implemented the registerTransactionPermissioningProvider method to allow custom transaction permissioning logic by adding providers to a collection. A method to iterate over these providers and check if transactions are permitted has also been included.

Currently, other methods in the PermissioningService interface (like registerNodePermissioningProvider and registerNodeMessagePermissioningProvider) are left unimplemented, as they are not part of this PR's scope.

I may need guidance on whether you would like me to proceed with implementing the other methods, and any improvements or adjustments needed to integrate this into the broader permissioning system.

Fixed Issue(s)

#7835

Thanks for sending a pull request! Have you done the following?

Checked out our contribution guidelines?
Considered documentation and added the doc-change-required label to this PR if updates are required.
Considered the changelog and included an update if required.
For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

unit tests: ./gradlew build
acceptance tests: ./gradlew acceptanceTest
integration tests: ./gradlew integrationTest
reference tests: ./gradlew ethereum:referenceTests:referenceTests

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

macfarla

thanks for the contrib!

a few comments. the logging would be better at info level. you can have extra logging in the plugin if needed, and/or you can use custom log4j config to turn this up for troubleshooting but don't want this permanently at info level. The one-off "registered plugin" is fine at info level.

there's also the TestPermissioningPlugin and PermissioningPluginTest - you could expand the test plugin and add a test to verify the tx permissioning callback works as expected

you'll also need to run the gradle target checkAPIChanges to calculate and update the api hash

and prob add a changelog entry

macfarla · 2024-11-27T21:48:19Z

besu/src/main/java/org/hyperledger/besu/services/PermissionServiceImpl.java

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


Suggested change

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

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

this would be spammy at info level

macfarla · 2024-11-27T21:48:53Z

besu/src/main/java/org/hyperledger/besu/services/PermissionServiceImpl.java

+import java.util.List;
+
+public class PermissionServiceImpl implements PermissioningService {
+    private static final Logger log = LoggerFactory.getLogger(PermissionServiceImpl.class);


Suggested change

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

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

all upper case naming convention for static fields

macfarla · 2024-11-27T21:49:58Z

besu/src/main/java/org/hyperledger/besu/services/PermissionServiceImpl.java

+    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());


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());

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

…hen it already exists Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

…er#7835 # Conflicts: # acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/plugins/PermissioningPluginTest.java

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

macfarla

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

macfarla · 2024-11-28T21:53:23Z

besu/src/main/java/org/hyperledger/besu/services/PermissioningServiceImpl.java

+  /**
+   * Gets transaction rules.
+   *
+   * @return if the transaction is valid


Suggested change

* @return if the transaction is valid

* @return whether the transaction is permitted

macfarla · 2024-11-28T21:54:47Z

CHANGELOG.md

@@ -32,6 +32,7 @@
  - Fast Sync

 ### Additions and Improvements
+- Add support for registeringTransactionPermissionProvider in Plugin API to define and validate transaction rules [#7952](https://github.com/hyperledger/besu/pull/7952)


Suggested change

- Add support for registeringTransactionPermissionProvider in Plugin API to define and validate transaction rules [#7952](https://github.com/hyperledger/besu/pull/7952)

- Add support for transaction permissioning rules in Plugin API [#7952](https://github.com/hyperledger/besu/pull/7952)

macfarla · 2024-11-28T22:01:14Z

plugin-api/src/main/java/org/hyperledger/besu/plugin/services/PermissioningService.java

@@ -14,6 +14,7 @@
 */
 package org.hyperledger.besu.plugin.services;

+import org.hyperledger.besu.ethereum.permissioning.account.TransactionPermissioningProvider;


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

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

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!

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.

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?

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

Thank you very much. It works perfectly!.

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

…er#7835

Signed-off-by: Vaidik <vaidikbhardwaj00@gmail.com>

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

…er#7835

Signed-off-by: Vaidik <vaidikbhardwaj00@gmail.com>

macfarla · 2024-12-10T07:28:13Z

@vaidikcode spotless!

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

…er#7835

macfarla · 2024-12-10T23:48:48Z

@vaidikcode still getting compile errors - can you run compile and test tasks locally, your feedback loop will be waaaay faster

/Users/sm/workspace/b2/ethereum/permissioning/src/main/java/org/hyperledger/besu/ethereum/permissioning/TransactionSmartContractPermissioningController.java:205: warning: [rawtypes] found raw type: BaseUInt256Value
            .map(BaseUInt256Value::toBytes)
                 ^
  missing type arguments for generic class BaseUInt256Value<T>
  where T is a type-variable:
    T extends UInt256Value<T> declared in class BaseUInt256Value
error: warnings found and -Werror specified
1 error
1 warning

macfarla · 2024-12-11T05:18:15Z

converting to draft as a signal it's not ready to review

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

…er#7835

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

macfarla

couple of comments

also this test is failing when I run locally - PermissioningPluginTest.testGasLimitLogic

macfarla · 2024-12-11T22:05:47Z

CHANGELOG.md

@@ -57,6 +57,7 @@
  - Fast Sync

 ### Additions and Improvements
+- Add support for transaction permissioning rules in Plugin API [#7952](https://github.com/hyperledger/besu/pull/7952)


needs to move to "Unreleased" section

macfarla · 2024-12-11T22:13:46Z

...hyperledger/besu/ethereum/permissioning/TransactionSmartContractPermissioningController.java

-        encodeAddress(transaction.getTo()),
-        transaction.getValue(),
-        transaction.getGasPrice().map(BaseUInt256Value::toBytes).orElse(Bytes32.ZERO),
+        encodeAddress(transaction.getTo().map(Address.class::cast)),


I don't love this line - surely there is a cleaner way todo this

macfarla · 2024-12-11T23:26:21Z

...hyperledger/besu/ethereum/permissioning/TransactionSmartContractPermissioningController.java

@@ -231,4 +237,16 @@ private static Bytes encodeBytes(final Bytes value) {
    final Bytes padding = Bytes.wrap(new byte[(32 - (value.size() % 32))]);
    return Bytes.concatenate(dynamicParameterOffset, length, value, padding);
  }
+
+  // Convert the Quantity value to Bytes
+  private static Bytes convertQuantityToBytes(final Quantity quantity) {


Suggested change

private static Bytes convertQuantityToBytes(final Quantity quantity) {

private static Bytes encodeQuantity(final Quantity quantity) {

…perledger#7835

macfarla · 2024-12-11T23:29:43Z

@vaidikcode couple of comments - it's getting there - but tests are still failing. Can you please run tests locally before saying it's ready to review? Your feedback loop will be like 100x faster

commit

1724426

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

vaidikcode changed the title ~~commit~~ Add Transaction Permissioning Hook to PermissioningService Interface Nov 27, 2024

macfarla reviewed Nov 27, 2024

View reviewed changes

macfarla self-assigned this Nov 27, 2024

macfarla mentioned this pull request Nov 28, 2024

add tx permissioning to plugin hyperledger/besu-docs#1744

Draft

macfarla added the doc-change-required Indicates an issue or PR that requires doc to be updated label Nov 28, 2024

vaidikcode added 9 commits November 28, 2024 10:47

addressing comments

858c8ee

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

updating api hash

dd498e7

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

fixing the issue of implementing the permission service class in pr w…

aae1526

…hen it already exists Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

add tests

2509d7d

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

change log

6c7b463

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

fix

4a0c5dd

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

fix

2d51ec2

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

Merge remote-tracking branch 'origin/hyperledger#7835' into hyperledg…

7851707

…er#7835 # Conflicts: # acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/plugins/PermissioningPluginTest.java

fix

da2c3de

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

macfarla requested changes Nov 28, 2024

View reviewed changes

vaidikcode added 9 commits November 29, 2024 04:20

fix

dd44cca

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

fix

549db1f

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

Merge branch 'hyperledger:main' into hyperledger#7835

7f75593

refactor

d77f294

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

Merge remote-tracking branch 'origin/hyperledger#7835' into hyperledg…

245e8ef

…er#7835

Merge branch 'main' into hyperledger#7835

b51669b

Signed-off-by: Vaidik <vaidikbhardwaj00@gmail.com>

fix

94620ed

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

Merge remote-tracking branch 'origin/hyperledger#7835' into hyperledg…

531ff5d

…er#7835

Merge branch 'main' into hyperledger#7835

ae8f3c8

Signed-off-by: Vaidik <vaidikbhardwaj00@gmail.com>

vaidikcode added 2 commits December 10, 2024 14:59

spotless

5d815c2

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

Merge remote-tracking branch 'origin/hyperledger#7835' into hyperledg…

9cfc510

…er#7835

macfarla marked this pull request as draft December 11, 2024 05:17

compile error fix

80c832c

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

vaidikcode marked this pull request as ready for review December 11, 2024 09:33

macfarla and others added 4 commits December 12, 2024 06:52

Merge branch 'main' into hyperledger#7835

b74a3e1

minor fix

0b23b89

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

Merge remote-tracking branch 'origin/hyperledger#7835' into hyperledg…

de4ee35

…er#7835

minor fix

040857a

Signed-off-by: vaidikcode <vaidikbhardwaj00@gmail.com>

vaidikcode marked this pull request as draft December 11, 2024 21:35

vaidikcode marked this pull request as ready for review December 11, 2024 21:59

final

a037c07

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

macfarla reviewed Dec 11, 2024

View reviewed changes

Merge branch 'hyperledger#7835' of github.com:vaidikcode/besu into hy…

028c103

…perledger#7835

macfarla marked this pull request as draft December 11, 2024 23:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Transaction Permissioning Hook to PermissioningService Interface #7952

Add Transaction Permissioning Hook to PermissioningService Interface #7952

vaidikcode commented Nov 27, 2024

macfarla left a comment

macfarla Nov 27, 2024

macfarla Nov 27, 2024

macfarla Nov 27, 2024

macfarla Nov 27, 2024

macfarla Nov 27, 2024

macfarla left a comment

macfarla Nov 28, 2024

macfarla Nov 28, 2024

macfarla Nov 28, 2024

vaidikcode Nov 28, 2024

vaidikcode Nov 28, 2024

macfarla Dec 4, 2024

vaidikcode Dec 6, 2024

macfarla Dec 8, 2024

vaidikcode Dec 9, 2024

macfarla commented Dec 10, 2024

macfarla commented Dec 10, 2024 •

edited

Loading

macfarla commented Dec 11, 2024

macfarla left a comment

macfarla Dec 11, 2024

macfarla Dec 11, 2024

macfarla Dec 11, 2024

macfarla commented Dec 11, 2024

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

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

	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 if the transaction is valid
	* @return whether the transaction is permitted

	- Add support for registeringTransactionPermissionProvider in Plugin API to define and validate transaction rules [#7952](https://github.com/hyperledger/besu/pull/7952)
	- Add support for transaction permissioning rules in Plugin API [#7952](https://github.com/hyperledger/besu/pull/7952)

	private static Bytes convertQuantityToBytes(final Quantity quantity) {
	private static Bytes encodeQuantity(final Quantity quantity) {

Add Transaction Permissioning Hook to PermissioningService Interface #7952

Are you sure you want to change the base?

Add Transaction Permissioning Hook to PermissioningService Interface #7952

Conversation

vaidikcode commented Nov 27, 2024

PR description

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

Locally, you can run these tests to catch failures early:

macfarla left a 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

macfarla left a 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

macfarla commented Dec 10, 2024

macfarla commented Dec 10, 2024 • edited Loading

macfarla commented Dec 11, 2024

macfarla left a 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

macfarla commented Dec 11, 2024

macfarla commented Dec 10, 2024 •

edited

Loading