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.

Sign up for GitHub

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

Jump to bottom

BlockTransactionSelector refactoring #5931

Merged
merged 12 commits into from
Sep 28, 2023
Merged

BlockTransactionSelector refactoring #5931

merged 12 commits into from
Sep 28, 2023

Conversation

Gabriel-Trintinalia
Copy link
Contributor

@Gabriel-Trintinalia Gabriel-Trintinalia commented Sep 25, 2023 •
edited
Loading

PR description

BlockTransactionSelector refactoring:

  • Extract TransactionSelectionResults to its own class.
  • Extract Transaction Selection Criteria and selection logic out of the BlockTransactionSelector to their own classes
  • BlockTransactionSelector will use a list of selectors to evaluate the transaction
  • Similar to selectors that will evaluate a transaction

Next steps:

  • Expose TransactionProcessingResult (or at least a part of it the to Plugin interface)
  • Abstract the transaction selectors and external transaction selectors so they share the same logic
  • Add a way to get the OperationTracer from the TPR and pass it to the plugin

@Gabriel-Trintinalia
first implementation of block refactor
dec21c8 
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
@Gabriel-Trintinalia
Block Transaction Selector Refactoring
5720dec 
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
@Gabriel-Trintinalia
Merge main
eefe6f1 
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
@Gabriel-Trintinalia
Fix API known hash
032aa04 
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
@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

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 25, 2023
View reviewed changes
...perledger/besu/ethereum/blockcreation/txselection/selectors/AbstractTransactionSelector.java
*/
public TransactionSelectionResult selectTransaction(
final Transaction transaction,
final TransactionSelectionResults blockTransactionResults,

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'blockTransactionResults' is never used.
@Gabriel-Trintinalia
Fix condition
883a20d 
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
@Gabriel-Trintinalia Gabriel-Trintinalia marked this pull request as ready for review September 25, 2023 06:27
macfarla
macfarla reviewed Sep 25, 2023
View reviewed changes
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

think it makes sense. would be good to get @fab-10 to take a look also

...n/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java Outdated Show resolved Hide resolved
.../hyperledger/besu/ethereum/blockcreation/txselection/selectors/PriceTransactionSelector.java Outdated Show resolved Hide resolved
.../hyperledger/besu/ethereum/blockcreation/txselection/selectors/PriceTransactionSelector.java Outdated Show resolved Hide resolved
...n/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java Outdated Show resolved Hide resolved
...edger/besu/ethereum/blockcreation/txselection/selectors/BlockTransactionSelectorFactory.java Outdated
private final List<TransactionSelectorFactory> transactionSelectorFactory;

public BlockTransactionSelectorFactory(
final List<TransactionSelectorFactory> transactionSelectorFactory) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a list because we're planning to add plugin factory here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the next step is to change how we pass the factories to the block transaction selector

...n/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java Outdated Show resolved Hide resolved
@Gabriel-Trintinalia
Accept PR comments
9620dc6 
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
fab-10
fab-10 reviewed Sep 25, 2023
View reviewed changes
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

I share the goal of the PR, since it adds more flexibility to the customization to the way we select txs for different use cases, I have some comments related to code readability, renaming to better express the intent of some methods, and trying to make it less error prone for future changes.

...n/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java Outdated Show resolved Hide resolved
...n/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java Show resolved Hide resolved
...n/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java Outdated Show resolved Hide resolved
...n/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java Show resolved Hide resolved
...perledger/besu/ethereum/blockcreation/txselection/selectors/AbstractTransactionSelector.java Outdated Show resolved Hide resolved
...perledger/besu/ethereum/blockcreation/txselection/selectors/AbstractTransactionSelector.java Outdated Show resolved Hide resolved
...erledger/besu/ethereum/blockcreation/txselection/selectors/BlobPriceTransactionSelector.java Outdated Show resolved Hide resolved
...edger/besu/ethereum/blockcreation/txselection/selectors/BlockTransactionSelectorFactory.java Outdated Show resolved Hide resolved
...perledger/besu/ethereum/blockcreation/txselection/selectors/AbstractTransactionSelector.java Outdated Show resolved Hide resolved
@Gabriel-Trintinalia
Accept PR recommendations
0e02103 
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
@Gabriel-Trintinalia
Fix todo identation
5b2171d 
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
@Gabriel-Trintinalia
Make naming consistent
1f8887d 
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
@Gabriel-Trintinalia
Update known hash
898b2a7 
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
macfarla
macfarla approved these changes Sep 26, 2023
View reviewed changes
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

ok with me

@Gabriel-Trintinalia
Merge main into branch
2acac83 
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
@Gabriel-Trintinalia Gabriel-Trintinalia enabled auto-merge (squash) September 27, 2023 23:36
@Gabriel-Trintinalia
Rename methods for consistency
f965beb 
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
@Gabriel-Trintinalia Gabriel-Trintinalia enabled auto-merge (squash) September 28, 2023 00:29
@Gabriel-Trintinalia Gabriel-Trintinalia merged commit c4f66c4 into hyperledger:main Sep 28, 2023
jflo pushed a commit to jflo/besu that referenced this pull request Nov 10, 2023
@Gabriel-Trintinalia @jflo
BlockTransactionSelector refactoring (hyperledger#5931)
cb7accc 
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Signed-off-by: Justin Florentine <justin+github@florentine.us>
NickSneo pushed a commit to NickSneo/besu that referenced this pull request Nov 12, 2023
@Gabriel-Trintinalia @NickSneo
BlockTransactionSelector refactoring (hyperledger#5931)
1b6e6ea 
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fab-10 fab-10 fab-10 approved these changes

@macfarla macfarla macfarla approved these changes

Assignees
No one assigned
Labels
mainnet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Gabriel-Trintinalia @macfarla @fab-10