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

Use PendingTransaction in BlockTransactionSelector #5966

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

fab-10
Copy link
Contributor

@fab-10 fab-10 commented Sep 28, 2023

PR description

This is just a refactoring, no functionality change, to have the PendingTransaction object during the selection of transactions, since in the PendingTransaction there are additional metadata about the candidate transaction that can be used by the selection process, and more will be added in a next PR to better support priority senders.

@github-actions
Copy link

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

@@ -28,7 +28,8 @@
* Tracks the additional metadata associated with transactions to enable prioritization for mining
* and deciding which transactions to drop when the transaction pool reaches its size limit.
*/
public abstract class PendingTransaction {
public abstract class PendingTransaction

Check notice

Code scanning / CodeQL

Class has same name as super class

PendingTransaction has the same name as its supertype [org.hyperledger.besu.datatypes.PendingTransaction](1).
@fab-10 fab-10 force-pushed the tx-selector-pending-tx branch from 7d1bcf1 to 68cbc2c Compare September 28, 2023 09:47
@fab-10 fab-10 marked this pull request as ready for review September 28, 2023 10:01
@fab-10 fab-10 mentioned this pull request Sep 28, 2023
Copy link
Contributor

@Gabriel-Trintinalia Gabriel-Trintinalia left a comment

Choose a reason for hiding this comment

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

LGTM, I am wondering if it makes sense to have it named PooledTransaction instead of PendingTransaction, since PendingTransaction are transactions that are in the transaction pool and not confirmed yet. That would also match the methods GetPooledTransactions and NewPooledTransactionHashes as well as the EncodingContext in the transaction encoder

@fab-10 fab-10 force-pushed the tx-selector-pending-tx branch from 68cbc2c to 5e62c01 Compare October 2, 2023 08:10
@fab-10 fab-10 enabled auto-merge (squash) October 2, 2023 08:10
@fab-10
Copy link
Contributor Author

fab-10 commented Oct 2, 2023

@Gabriel-Trintinalia it seems that pending and pooled are both used as synonyms, here for example, but I have no objections if you want to evaluate a renaming

@fab-10 fab-10 merged commit 987d33c into hyperledger:main Oct 2, 2023
@fab-10 fab-10 deleted the tx-selector-pending-tx branch October 2, 2023 09:32
jflo pushed a commit to jflo/besu that referenced this pull request Nov 10, 2023
NickSneo pushed a commit to NickSneo/besu that referenced this pull request Nov 12, 2023
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.

2 participants