[Follow up] Add Transaction Permissioning Hook to PermissioningService Interface

[relu91]

PR description

This PR builds upon the work started in #7952, finalizing the implementation while maintaining consistency with the existing changes.

Key Updates:

• Completed the feature without altering the previous implementation significantly.
• Added some improvements to the acceptance tests
• Ensured consistency with the existing codebase by following established style and logic.
• Took inspiration from the NodePermissioningController for handling plugin-based submissions, aligning the approach with Besu’s existing permissioning mechanisms.

Basically, after merging this work, people can filter transactions using the PermissioningService in their plugins. I would say adding some examples to the documentation might be useful.

Note: I didn't like the previous acceptance test, I found it a little bit convoluted for simply assigning the filtering mechanism of the plugins. As I said, I tried to be as conservative as I could. If you want, we can discuss possible improvements there.

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:

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

[macfarla]

thanks for your contribution! Strong preference for not modifying the smart contract permissioning code at this stage since it's about to be removed.

[macfarla]

we've deprecated smart contract based permissioning per the sunset plan https://www.lfdecentralizedtrust.org/blog/sunsetting-tessera-and-simplifying-hyperledger-besu

so this class is going to be removed

[macfarla]

and right now we're effectively in a code freeze for the sunset features. So would prefer not to modify this code if possible, and if not possible, would prefer to remove smart contract permissioning first.

[relu91]

That was indeed another thing that I wanted to mention in the first comment. I can revert that change. Should I also add @Depracted tag? or is it not common practice here?

[relu91]

Meanwhile, I checked the latest version from main reverting the changes. Let me know if I need to do other cleanups of the previous changes.

[macfarla]

looking pretty good, couple of minor suggestions

[macfarla]

@jflo if you want to take a look, this replaces #7952

[relu91]

I fixed the build error of the previous CI run, it should be ready now.

[macfarla]

thinking we should standardise the class naming and logging to "Transaction permissioning" rather than "Account permissioning" - but this can be a separate PR

[jflo]

You could even call it "Transaction Rejector" or something that conveys that the default case is to permit the transaction.

[relu91]

Should I resolve the conflicts with main? I don't want to trigger CI again for a trivial hash check in the build system.

[jflo]

I think this is good, the only part of the design I would tighten up would be to require transaction permissioning to be defined under a broader permissioning config.

Not sure if that is by design or not, but one could view it as a leaky abstraction.

[jflo]

Allowing transaction permissioning without a broader permissioningConfiguration seems weird, we will need to clarify in the docs that you can set up permissioning on just the transactions in this manner.

[relu91]

I followed the same behavior of Node permissioning above. Should I change both of them? How should the end result looks like? User create an empty account permissioning file to enable a plugin that filters transactions? Shall I create an additional option --permissions-accounts-plugin-enable (and also --permissions-node-plugin-enable) ?

[macfarla]

I think this could be refactored separately (different PR) since the same pattern is used for node permissioning

[relu91]

@macfarla Opened an issue to discuss possible solutions: #8407 (I can volunteer if needed)

[jflo]

You could even call it "Transaction Rejector" or something that conveys that the default case is to permit the transaction.

[relu91]

I've updated the logging messages, however, if you ask me, it is weird that a class called AccountPermissioningController logs messages with Transaction Rejector ... . Should we probably think of a major refactoring of the concept? ( and probably explain something here)

[jflo]

I've updated the logging messages, however, if you ask me, it is weird that a class called AccountPermissioningController logs messages with Transaction Rejector ... . Should we probably think of a major refactoring of the concept? ( and probably explain something here)

Thats ok, the details on which class are emitting the log should be preserved via the SLF4J pattern. So the message should focus on what it is doing, i.e. rejecting transactions.