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(mempool)!: gossip reference to tx and request if needed #6675

Merged
merged 9 commits into from
Nov 11, 2024

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Nov 7, 2024

Description

  • Gossip full transaction if less than 64Kb
  • Otherwise, gossip new transaction notification
  • Batch notifications and request transactions using mempool sync in batches every interval
  • Raised the message size limit for base node GRPC

Motivation and Context

Transactions routinely are too large for gossipsub.
A new protocol has been added to mempool sync. Many transactions may be gossiped in quick succession and therefore it is inefficient to request each time a transaction notification is received. This protocol aims to address this by collecting notifications and periodically requesting many transactions in batched.

Details:

  1. NewTransaction gossip message is received,
  2. If it contains the full transaction (it fits into the max message size for gossipsub) the transaction is added as before.
  3. Otherwise, the references to new transactions, the propagation peer and message id are collected
  4. Every interval the batch is processed, requesting all transactions in a single request
  5. Transactions are streamed back and validated.
  6. The result of each of these is collected and the propagation message is either accepted, rejected or ignored depending on the result. This ensures that further propagation of the transaction message only occurs for valid transactions.

Base node GRPC decode and encode limit were raised from 4MiB to 10MiB, this is because some blocks within the legal weight limit were not transferable via GRPC when mining (~6MiB) causing GRPC errors and the inability to mine.

How Has This Been Tested?

Manually, submitting transactions and observing logs

What process can a PR reviewer use to test or verify this change?

Submit transactions to one node and observe the mempool of other nodes on the network

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify
    BREAKING CHANGE: Changes to the mempool sync protocol are not compatible with previous versions

Copy link

github-actions bot commented Nov 7, 2024

Test Results (CI)

    3 files    126 suites   12m 6s ⏱️
1 160 tests 1 160 ✅ 0 💤 0 ❌
3 480 runs  3 480 ✅ 0 💤 0 ❌

Results for commit ad35c63.

♻️ This comment has been updated with latest results.

@sdbondi sdbondi force-pushed the libp2p-tx-propagation branch 2 times, most recently from 06b80c3 to e491a40 Compare November 7, 2024 13:50
Copy link

github-actions bot commented Nov 7, 2024

Test Results (Integration tests)

 2 files   1 errors  9 suites   1h 34m 47s ⏱️
18 tests 13 ✅ 0 💤  5 ❌
27 runs  14 ✅ 0 💤 13 ❌

For more details on these parsing errors and failures, see this check.

Results for commit c78fac2.

♻️ This comment has been updated with latest results.

@sdbondi sdbondi force-pushed the libp2p-tx-propagation branch 2 times, most recently from 9a111a1 to dec7698 Compare November 8, 2024 05:55
@sdbondi sdbondi marked this pull request as ready for review November 8, 2024 05:56
@sdbondi sdbondi requested a review from a team as a code owner November 8, 2024 05:56
@sdbondi sdbondi changed the title feat(mempool): gossip reference to tx and request if needed feat(mempool)!: gossip reference to tx and request if needed Nov 8, 2024
Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

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

This looks good, some questions about propagation.
We have to be careful about what messages we want to send out. This becomes a big issue with large networks. Libp2p will ensure that a single gossip message is sent to the entire network. So if we will send a message based on that later on, we should not forward the message as this will lead us to send an exponentially increasing number of messages, something akin to (peers ) *(peers-1) for every single ,message if my math is correct here

base_layer/core/src/mempool/service/service.rs Outdated Show resolved Hide resolved
base_layer/core/src/mempool/service/service.rs Outdated Show resolved Hide resolved
base_layer/core/src/mempool/service/service.rs Outdated Show resolved Hide resolved
hansieodendaal
hansieodendaal previously approved these changes Nov 8, 2024
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

Nice work! Just some small comments noted.

I ran 2x coin-split system-level stress tests containing huge transactions and 1x interactive transaction system-level stress test. Throughout the mempools stayed in sync and all transactions were propagated to mempool peers as expected. Interrogation of the trace logs also unveiled no undue issues.

ACK

base_layer/core/src/mempool/service/inbound_handlers.rs Outdated Show resolved Hide resolved
base_layer/core/src/mempool/sync_protocol/mod.rs Outdated Show resolved Hide resolved
base_layer/core/src/mempool/sync_protocol/mod.rs Outdated Show resolved Hide resolved
@SWvheerden SWvheerden merged commit 3072882 into tari-project:feat-libp2p Nov 11, 2024
12 checks passed
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.

3 participants