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

feat: add mempool txs to payload #575

Merged
merged 60 commits into from
Oct 4, 2024
Merged

feat: add mempool txs to payload #575

merged 60 commits into from
Oct 4, 2024

Conversation

fmoletta
Copy link
Contributor

@fmoletta fmoletta commented Sep 27, 2024

Motivation
Fetching transactions from the mempool during the block building process (engine_GetPayload).
This code can and should be improved in the future, but it sets a basis for block building

Description

  • Add method build_payload, which fetches transactions from the mempool and adds them to the payload.
  • Add a way to remove and filter transactions from the mempool
  • (Misc) Simplify state_trie related StoreEngine api. The StoreEngine now only creates the trie, leaving block and state root fetching to the Store. This removes duplicated code

LEFTOVER WORK:

  • Save timestamp when a tx is added to the mempool
  • process blobs in block building pipeline (we are now ignoring them)

TESTING_STATUS:
The pipeline for block_building tests is ForkChoiceUpdated -> (SendRawTransaction) -> GetPayload -> NewPayload -> (Fetch blocks/receipts).
Tests are currently failing at NewPayload because of different blob versioned hashes.

Closes None, but is part of #344

Copy link
Contributor

@fkrause98 fkrause98 left a comment

Choose a reason for hiding this comment

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

Looks good overall, left some comments to discuss, and btw, how are we testing these changes?

crates/blockchain/Cargo.toml Show resolved Hide resolved
crates/blockchain/payload.rs Outdated Show resolved Hide resolved
crates/blockchain/payload.rs Outdated Show resolved Hide resolved
crates/blockchain/payload.rs Outdated Show resolved Hide resolved
crates/core/types/transaction.rs Outdated Show resolved Hide resolved
crates/core/types/transaction.rs Outdated Show resolved Hide resolved
crates/rpc/engine/payload.rs Outdated Show resolved Hide resolved
crates/storage/engines/in_memory.rs Outdated Show resolved Hide resolved
crates/storage/engines/libmdbx.rs Outdated Show resolved Hide resolved
crates/storage/engines/libmdbx.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ilitteri ilitteri left a comment

Choose a reason for hiding this comment

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

The PR looks good. I agree with @fkrause98 comments. After creating issues for the TODOs it'd be great to link them as a comment in the code where appropriate.

@fkrause98
Copy link
Contributor

Looks good, let me know when the CI is fixed and I'll take another look! @fmoletta

@fmoletta
Copy link
Contributor Author

fmoletta commented Oct 3, 2024

Looks good, let me know when the CI is fixed and I'll take another look! @fmoletta

Should be working now 🚀

@jrchatruc jrchatruc enabled auto-merge October 4, 2024 18:43
@jrchatruc jrchatruc added this pull request to the merge queue Oct 4, 2024
Merged via the queue into main with commit b17a7ef Oct 4, 2024
7 checks passed
@jrchatruc jrchatruc deleted the payload-add-txs branch October 4, 2024 19:00
github-merge-queue bot pushed a commit that referenced this pull request Oct 8, 2024
> [!WARNING]
> This PR depends on #575 and must be merged after it.

**Motivation**

<!-- Why does this pull request exist? What are its goals? -->

**Description**

<!-- A clear and concise general description of the changes this PR
introduces -->

- Adds block producer module
- Implements `Engine` to advance the blockchain

---------

Co-authored-by: fmoletta <fedemoletta@hotmail.com>
Co-authored-by: Manuel Bilbao <manuel.bilbao@lambdaclass.com>
Co-authored-by: Manuel Iñaki Bilbao <bilbaomanuel98@gmail.com>
mpaulucci pushed a commit to mpaulucci/lambda_ethereum_rust that referenced this pull request Oct 16, 2024
**Motivation**
Fetching transactions from the mempool during the block building process
(engine_GetPayload).
This code can and should be improved in the future, but it sets a basis
for block building
<!-- Why does this pull request exist? What are its goals? -->

**Description**
* Add method `build_payload`, which fetches transactions from the
mempool and adds them to the payload.
* Add a way to remove and filter transactions from the mempool
* (Misc) Simplify state_trie related `StoreEngine` api. The
`StoreEngine` now only creates the trie, leaving block and state root
fetching to the `Store`. This removes duplicated code

LEFTOVER WORK:
* Save timestamp when a tx is added to the mempool
* process blobs in block building pipeline (we are now ignoring them)

<!-- A clear and concise general description of the changes this PR
introduces -->

<!-- Link to issues: Resolves lambdaclass#111, Resolves lambdaclass#222 -->

TESTING_STATUS:
The pipeline for block_building tests is ForkChoiceUpdated ->
(SendRawTransaction) -> GetPayload -> NewPayload -> (Fetch
blocks/receipts).
Tests are currently failing at NewPayload because of different blob
versioned hashes.

Closes None, but is part of lambdaclass#344
mpaulucci pushed a commit to mpaulucci/lambda_ethereum_rust that referenced this pull request Oct 16, 2024
> [!WARNING]
> This PR depends on lambdaclass#575 and must be merged after it.

**Motivation**

<!-- Why does this pull request exist? What are its goals? -->

**Description**

<!-- A clear and concise general description of the changes this PR
introduces -->

- Adds block producer module
- Implements `Engine` to advance the blockchain

---------

Co-authored-by: fmoletta <fedemoletta@hotmail.com>
Co-authored-by: Manuel Bilbao <manuel.bilbao@lambdaclass.com>
Co-authored-by: Manuel Iñaki Bilbao <bilbaomanuel98@gmail.com>
mpaulucci pushed a commit to mpaulucci/lambda_ethereum_rust that referenced this pull request Oct 16, 2024
**Motivation**
Fetching transactions from the mempool during the block building process
(engine_GetPayload).
This code can and should be improved in the future, but it sets a basis
for block building
<!-- Why does this pull request exist? What are its goals? -->

**Description**
* Add method `build_payload`, which fetches transactions from the
mempool and adds them to the payload.
* Add a way to remove and filter transactions from the mempool
* (Misc) Simplify state_trie related `StoreEngine` api. The
`StoreEngine` now only creates the trie, leaving block and state root
fetching to the `Store`. This removes duplicated code

LEFTOVER WORK:
* Save timestamp when a tx is added to the mempool
* process blobs in block building pipeline (we are now ignoring them)

<!-- A clear and concise general description of the changes this PR
introduces -->

<!-- Link to issues: Resolves lambdaclass#111, Resolves lambdaclass#222 -->

TESTING_STATUS:
The pipeline for block_building tests is ForkChoiceUpdated ->
(SendRawTransaction) -> GetPayload -> NewPayload -> (Fetch
blocks/receipts).
Tests are currently failing at NewPayload because of different blob
versioned hashes.

Closes None, but is part of lambdaclass#344
mpaulucci pushed a commit to mpaulucci/lambda_ethereum_rust that referenced this pull request Oct 16, 2024
> [!WARNING]
> This PR depends on lambdaclass#575 and must be merged after it.

**Motivation**

<!-- Why does this pull request exist? What are its goals? -->

**Description**

<!-- A clear and concise general description of the changes this PR
introduces -->

- Adds block producer module
- Implements `Engine` to advance the blockchain

---------

Co-authored-by: fmoletta <fedemoletta@hotmail.com>
Co-authored-by: Manuel Bilbao <manuel.bilbao@lambdaclass.com>
Co-authored-by: Manuel Iñaki Bilbao <bilbaomanuel98@gmail.com>
mpaulucci pushed a commit that referenced this pull request Oct 16, 2024
**Motivation**
Fetching transactions from the mempool during the block building process
(engine_GetPayload).
This code can and should be improved in the future, but it sets a basis
for block building
<!-- Why does this pull request exist? What are its goals? -->

**Description**
* Add method `build_payload`, which fetches transactions from the
mempool and adds them to the payload.
* Add a way to remove and filter transactions from the mempool
* (Misc) Simplify state_trie related `StoreEngine` api. The
`StoreEngine` now only creates the trie, leaving block and state root
fetching to the `Store`. This removes duplicated code

LEFTOVER WORK:
* Save timestamp when a tx is added to the mempool
* process blobs in block building pipeline (we are now ignoring them)

<!-- A clear and concise general description of the changes this PR
introduces -->

<!-- Link to issues: Resolves #111, Resolves #222 -->

TESTING_STATUS:
The pipeline for block_building tests is ForkChoiceUpdated ->
(SendRawTransaction) -> GetPayload -> NewPayload -> (Fetch
blocks/receipts).
Tests are currently failing at NewPayload because of different blob
versioned hashes.

Closes None, but is part of #344
mpaulucci pushed a commit that referenced this pull request Oct 16, 2024
> [!WARNING]
> This PR depends on #575 and must be merged after it.

**Motivation**

<!-- Why does this pull request exist? What are its goals? -->

**Description**

<!-- A clear and concise general description of the changes this PR
introduces -->

- Adds block producer module
- Implements `Engine` to advance the blockchain

---------

Co-authored-by: fmoletta <fedemoletta@hotmail.com>
Co-authored-by: Manuel Bilbao <manuel.bilbao@lambdaclass.com>
Co-authored-by: Manuel Iñaki Bilbao <bilbaomanuel98@gmail.com>
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.

4 participants