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 the possibility for a plugin to validate a tx according to txpool rules #8401

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

fab-10
Copy link
Contributor

@fab-10 fab-10 commented Mar 11, 2025

PR description

This PR adds the new method

ValidationResult validateTransaction(Transaction transaction, boolean isLocal, boolean hasPriority);

to the TransactionPoolService interface, giving plugins the possibility to validate txs according to txpool rules, so that plugins implementing their own txpools can make sure incoming txs are valid.

Fixed Issue(s)

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

Sorry, something went wrong.

@fab-10 fab-10 force-pushed the txpool-service-validate-tx branch from 90570b6 to 2054100 Compare March 11, 2025 18:00
… rules

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@fab-10 fab-10 force-pushed the txpool-service-validate-tx branch from 2054100 to ea00e3f Compare March 12, 2025 15:59
@fab-10 fab-10 marked this pull request as ready for review March 12, 2025 15:59
Copy link
Contributor

@pinges pinges left a comment

Choose a reason for hiding this comment

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

Some comments

.until(
() -> {
if (file.exists()) {
try (final Stream<String> s = Files.lines(path)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds expensive for trying to find out whether that file is not empty. What about file.length()? I think it returns 0 if the file does not exist, so that might be all that you need to check for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, changed.
I blindly consolidated more copies of this function, to remove code duplication, but I have not checked the implementation

private static final Logger LOG = LoggerFactory.getLogger(TestTransactionPoolServicePlugin.class);
private static final Supplier<SignatureAlgorithm> SIGNATURE_ALGORITHM =
Suppliers.memoize(SignatureAlgorithmFactory::getInstance);
private static final SECPPrivateKey PRIVATE_KEY1 =
Copy link
Contributor

Choose a reason for hiding this comment

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

There is only one key, so we do not need a number :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

.createPrivateKey(
Bytes32.fromHexString(
"8f2a55949038a9610f50fb23b5883af3b4ecb3c3bb792cbcefbd1542c692be63"));
private static final KeyPair KEYS1 =
Copy link
Contributor

Choose a reason for hiding this comment

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

s.a.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

public void register(final ServiceManager serviceManager) {
LOG.info("Registering TestTransactionPoolServicePlugin");
this.serviceManager = serviceManager;
callbackDir = new File(System.getProperty("besu.plugins.dir", "plugins"));
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
callbackDir = new File(System.getProperty("besu.plugins.dir", "plugins"));
this.callbackDir = new File(System.getProperty("besu.plugins.dir", "plugins"));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

fab-10 added 2 commits March 13, 2025 09:33
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@fab-10 fab-10 requested a review from pinges March 13, 2025 08:53
@fab-10 fab-10 marked this pull request as draft March 17, 2025 09:38
@relu91
Copy link
Contributor

relu91 commented Mar 19, 2025

With different semantics it is already possible to filter transactions before they enter in the TxPool. In particular, after #8365, you can filter transactions using the permissioning API. As far as I can tell here we have a different goal, which is transaction validation rather than apply a policy (e.g. account X is not permitted to create a contract), but we have to explain to the end user the different pupose of these methods ane when to prefer one to the other.

Otherwise, we can explore a solution that works for both use cases.

@fab-10
Copy link
Contributor Author

fab-10 commented Mar 19, 2025

@relu91 I still need to review #8365, so I do not have the full picture yet, so I ca just add some context to help shaping the API

1 Scope of this PR is to expose the standard txpool validation done by Besu so that plugins can leverage it to implement custom txpools, delegating to Besu the standard txpool validation avoiding code duplication.
2 Besu plugins already have a way to extend the standard txpool validation via the PluginTransactionPoolValidator that is invoked here, we extensively use this approach to augment the txpool validation for the current requirement of Linea

So, if your requirement to extend the standard txpool validation, we can see how to merge it with the API listed at the 2nd point, otherwise if the requirement is to be able to validate according to standard rules txs that are not meant to be kept by the Besu txpool the the 1st point applies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants