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

Mempool txn prioritization #920

Merged
merged 1 commit into from
Jul 14, 2022
Merged

Conversation

viquezclaudio
Copy link
Member

@viquezclaudio viquezclaudio commented Jul 5, 2022

  • Introduces some priorities (low, medium, high) to transactions when they are added into the mempool.
  • Exposes this functionality via RPC
  • The unpark transaction is included into the mempool with high priority

@viquezclaudio viquezclaudio added the WIP This pull request is still work in progress label Jul 5, 2022
@codecov
Copy link

codecov bot commented Jul 5, 2022

Codecov Report

Merging #920 (cf35d46) into albatross (f75c6fd) will increase coverage by 0.00%.
The diff coverage is 60.31%.

@@            Coverage Diff             @@
##           albatross     #920   +/-   ##
==========================================
  Coverage      64.27%   64.27%           
==========================================
  Files            359      359           
  Lines          42264    42306   +42     
==========================================
+ Hits           27167    27194   +27     
- Misses         15097    15112   +15     
Flag Coverage Δ
unittests 64.27% <60.31%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mempool/src/lib.rs 100.00% <ø> (ø)
rpc-interface/src/mempool.rs 0.00% <ø> (ø)
rpc-server/src/dispatchers/mempool.rs 0.00% <0.00%> (ø)
spammer/src/main.rs 0.24% <0.00%> (ø)
validator/src/validator.rs 57.87% <0.00%> (-0.30%) ⬇️
mempool/src/mempool_transactions.rs 92.43% <95.45%> (+0.05%) ⬆️
mempool/src/executor.rs 94.44% <100.00%> (+0.10%) ⬆️
mempool/src/mempool.rs 73.73% <100.00%> (+0.50%) ⬆️
mempool/src/mempool_state.rs 72.25% <100.00%> (+1.15%) ⬆️
network-libp2p/src/connection_pool/behaviour.rs 51.78% <0.00%> (-0.26%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f75c6fd...cf35d46. Read the comment docs.

@viquezclaudio viquezclaudio force-pushed the viquezcl/mempool_txn_priority branch from ca7297e to 317458b Compare July 13, 2022 00:28
@viquezclaudio viquezclaudio removed the WIP This pull request is still work in progress label Jul 13, 2022
@viquezclaudio viquezclaudio force-pushed the viquezcl/mempool_txn_priority branch from 317458b to af7660b Compare July 13, 2022 00:29
Copy link
Member

@nibhar nibhar left a comment

Choose a reason for hiding this comment

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

Good to go! My comments just to save some code and a question for trannsactions of reverted blocks.

Comment on lines 676 to 683
if let Some(priority) = tx_priority {
RwLockUpgradableReadGuard::upgrade(mempool_state_lock)
.put(&transaction, priority);
} else {
//If the priority is not provided we default to medium
RwLockUpgradableReadGuard::upgrade(mempool_state_lock)
.put(&transaction, TxPriority::MediumPriority);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if let Some(priority) = tx_priority {
RwLockUpgradableReadGuard::upgrade(mempool_state_lock)
.put(&transaction, priority);
} else {
//If the priority is not provided we default to medium
RwLockUpgradableReadGuard::upgrade(mempool_state_lock)
.put(&transaction, TxPriority::MediumPriority);
}
RwLockUpgradableReadGuard::upgrade(mempool_state_lock)
.put(&transaction, tx_priority.unwrap_or(TxPriority::MediumPriority));

@@ -520,7 +521,7 @@ impl Mempool {
let pending_balance = tx.total_value() + sender_total;

if pending_balance <= sender_balance {
mempool_state.put(tx);
mempool_state.put(tx, TxPriority::MediumPriority);
Copy link
Member

Choose a reason for hiding this comment

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

All reverted transactions are added as MediumPriority. The fact they might have been added as HighPriority is lost of course, but potentially we could still add unpark transactions as HighPriority.

This commit allows one to specify a transaction priority
when txns are being addded to the mempool, such that
txns with higher priority are returned first.
This is exposed to the RPC interface
It is also used to prioritize our own unpark transactions
@viquezclaudio viquezclaudio force-pushed the viquezcl/mempool_txn_priority branch from af7660b to cf35d46 Compare July 14, 2022 18:12
@viquezclaudio viquezclaudio merged commit 4132bad into albatross Jul 14, 2022
@viquezclaudio viquezclaudio deleted the viquezcl/mempool_txn_priority branch July 14, 2022 20:09
@sisou
Copy link
Member

sisou commented Jul 15, 2022

When querying mempool state, e.g. via RPC mempoolContent() method, are the transactions ordered by priority? Is the priority part of the returned object (if I set include_transactions to true)?

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