Skip to content

Implementation of Mempool Syncer - #2832

Open
Eligioo wants to merge 16 commits into
albatrossfrom
stefan/mempool-syncer
Open

Implementation of Mempool Syncer#2832
Eligioo wants to merge 16 commits into
albatrossfrom
stefan/mempool-syncer

Conversation

@Eligioo

@Eligioo Eligioo commented Aug 15, 2024

Copy link
Copy Markdown
Member

What's in this pull request?

The Mempool Syncer is responsible for acquiring the transactions that are currently in the mempool of other nodes directly after reaching consensus.

The mempool syncers get initiated once the Mempool Executors are spawned. One Syncer per Executor is started in order to sync the regular and control transactions separately. First the Syncer starts to discover which transaction hashes other peers with a mempool have and compare those with what we have locally. Then we distribute those unknown hashes among the peers we know have those transactions and retrieve the actual transaction. Lastly for every received transaction we do a full verification which should add it to our local mempool if everything checks out.

Open design questions

  • Define the parameters for the Mempool Syncer: max transactions to sync, transactions per request, etc.
    Max transactions we acquire per peer: 25.000
    Amount of transaction per request: 500
  • Should the Mempool Syncer end its stream after x amount of time
    After 10 minutes

This fixes #2625 .

Pull request checklist

  • All tests pass. The project builds and runs.
  • I have resolved any merge conflicts.
  • I have resolved all clippy and rustfmt warnings.

@Eligioo Eligioo changed the title Implementation of a Mempool Syncer Implementation of Mempool Syncer Aug 15, 2024
@Eligioo
Eligioo force-pushed the stefan/mempool-syncer branch 2 times, most recently from 010d273 to e129d1c Compare August 19, 2024 11:25
Comment thread mempool/src/executor.rs
@Eligioo
Eligioo force-pushed the stefan/mempool-syncer branch 3 times, most recently from eabf6dd to 4f5c75a Compare August 24, 2024 13:42
@Eligioo
Eligioo force-pushed the stefan/mempool-syncer branch 3 times, most recently from 0f84490 to c43f4ae Compare September 11, 2024 08:28
@Eligioo
Eligioo marked this pull request as ready for review September 11, 2024 08:48
@Eligioo
Eligioo force-pushed the stefan/mempool-syncer branch 3 times, most recently from 2011b40 to b09686b Compare October 9, 2024 11:01
@Eligioo Eligioo added the testnet-restart Change breaks the protocol and requires a testnet restart label Oct 11, 2024
@Eligioo
Eligioo force-pushed the stefan/mempool-syncer branch 2 times, most recently from 95e9128 to 0b03d91 Compare November 7, 2024 10:55
@sisou

sisou commented Dec 9, 2024

Copy link
Copy Markdown
Member

Why is this a breaking change (testnet-restart label)?

@Eligioo

Eligioo commented Dec 10, 2024

Copy link
Copy Markdown
Member Author

Why is this a breaking change (testnet-restart label)?

Because it introduces some new network request messages. Technically it is behind a feature flag because the syncer utilizes an yet unused service flag so these new request messages would only go to peers that also have this new component. So even though it's breaking, it shouldn't be too difficult to get in but for bookkeeping I kept the label around.

@Eligioo
Eligioo force-pushed the stefan/mempool-syncer branch from 0b03d91 to 0bc22d5 Compare December 11, 2024 13:42
@Eligioo
Eligioo force-pushed the stefan/mempool-syncer branch from 0bc22d5 to 20da134 Compare January 27, 2025 15:35
Comment thread consensus/src/sync/syncer.rs Outdated
Comment thread consensus/src/sync/syncer.rs Outdated
Comment thread consensus/src/sync/syncer.rs Outdated
Comment thread consensus/src/sync/syncer.rs Outdated
Comment thread consensus/src/sync/syncer_proxy.rs Outdated
Comment thread mempool/src/sync/mod.rs Outdated
Comment thread mempool/src/sync/mod.rs Outdated
Comment thread mempool/src/sync/mod.rs
Comment thread mempool/src/sync/mod.rs
}

info!(num = %transactions.len(), "Synced mempool transactions");
self.transactions.extend(transactions);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about the response missing transactions that we requested?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I my opinion that's OK when you don't receive (all) transactions you are asking for. By the time the peer received your request, the particular transaction(s) could already be adopted in a block.

Comment thread mempool/src/executor.rs
@hrxi

hrxi commented Feb 12, 2025

Copy link
Copy Markdown
Contributor

Why is this a breaking change (testnet-restart label)?

Because it introduces some new network request messages. Technically it is behind a feature flag because the syncer utilizes an yet unused service flag so these new request messages would only go to peers that also have this new component. So even though it's breaking, it shouldn't be too difficult to get in but for bookkeeping I kept the label around.

I think I agree with @sisou that it's not breaking, precisely because it uses a previously unused feature flag.

@hrxi hrxi removed the testnet-restart Change breaks the protocol and requires a testnet restart label Feb 12, 2025
@Eligioo
Eligioo force-pushed the stefan/mempool-syncer branch from 20da134 to 72dba68 Compare March 12, 2025 11:17
Eligioo added 15 commits March 21, 2025 10:43
The syncers get initiated once the Mempool Executors are spawned. One Syncer per Executor is started in order to sync the regular and control transactions separately.
First the Syncer starts to discover which transaction hashes other nodes with a mempool have and compare those with what we have locally.
Then we distribute those unknown hashes among the peers we know have those transactions and retrieve the actual transactions.
Lastly for every received transaction we do a full verification which should add it to our local mempool if everything checks out.
…ain a collection of peers that are in live sync
…ConsensusProxy rather than the Consensus directly
- Only try to construct mempool transaction requests if new hashes are discovered
- Check the mempool state first before checking the blockchain if a transaction is already known/included
- Bound the amount of received hashes that will be processed per peer
- Fix tests
@Eligioo
Eligioo force-pushed the stefan/mempool-syncer branch from bc8a325 to 4cdd033 Compare March 21, 2025 10:25
@Eligioo
Eligioo force-pushed the stefan/mempool-syncer branch from 4cdd033 to 0b134ad Compare March 21, 2025 10:28
@Eligioo

Eligioo commented Mar 21, 2025

Copy link
Copy Markdown
Member Author

@hrxi either your comments are resolved or has received a reply

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.

Add a mechanism to sync the mempool

3 participants