Second (control) mempool implementation #885

viquezclaudio · 2022-06-14T21:46:54Z

Introduced a second mempool, used for control transactions
When blocks are produced, first we try to include control txns
If there is space left, we include regular txns
This means that control txns have a higher priority over reg txns.

codecov · 2022-06-14T22:23:34Z

Codecov Report

Merging #885 (e4f9a72) into albatross (bce6247) will increase coverage by 0.10%.
The diff coverage is 78.55%.

❗ Current head e4f9a72 differs from pull request most recent head f75c6fd. Consider uploading reports for the commit f75c6fd to get more accurate results

@@              Coverage Diff              @@
##           albatross     #885      +/-   ##
=============================================
+ Coverage      64.16%   64.26%   +0.10%     
=============================================
  Files            357      359       +2     
  Lines          42028    42264     +236     
=============================================
+ Hits           26968    27163     +195     
- Misses         15060    15101      +41

Flag	Coverage Δ
unittests	`64.26% <78.55%> (+0.10%)`	⬆️

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

Impacted Files	Coverage Δ
consensus/src/consensus/mod.rs	`76.85% <0.00%> (-1.09%)`	⬇️
lib/src/config/config.rs	`29.73% <0.00%> (-0.16%)`	⬇️
lib/src/config/config_file/mod.rs	`20.80% <0.00%> (-0.43%)`	⬇️
mempool/src/lib.rs	`100.00% <ø> (ø)`
mempool/src/mempool_metrics.rs	`59.09% <ø> (ø)`
mempool/src/verify.rs	`65.67% <ø> (ø)`
mempool/src/mempool_state.rs	`71.09% <71.09%> (ø)`
validator/src/validator.rs	`58.17% <73.33%> (+0.35%)`	⬆️
mempool/src/mempool.rs	`73.23% <79.44%> (+0.12%)`	⬆️
mempool/src/mempool_transactions.rs	`92.38% <92.38%> (ø)`
... and 13 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 bce6247...f75c6fd. Read the comment docs.

consensus/src/consensus/mod.rs

mempool/src/executor.rs

mempool/src/mempool.rs

validator/src/micro.rs

validator/src/validator.rs

viquezclaudio · 2022-06-20T20:00:44Z

@sisou any more comments?

nibhar

I created #929 to some smaller refactoring and to move some code pieces to their own files. Feel free to look through it and cherry pick if necessary.

Other than that I have little to add to what @sisou already provided. I do agree that control transactions should always come first, as there is no reason to do it the other way around and it does not introduce any complexity as far as I can tell.

nibhar · 2022-07-07T10:25:15Z

mempool/src/executor.rs

+            filter,
+            network,
+            network_id: Arc::new(blockchain.read().network_id),
+            verification_tasks: Arc::new(AtomicU32::new(0)),


More of a question than an actual comment:

So each mempool has their ow transaction verification task limit. That doubles the amount of verification tasks from what it was previously. Is that intended?

Yes, I think we never really hit that limit under normal operation, should I set it to half of it, so we maintain the same total amount of verification tasks as before?

nibhar · 2022-07-11T14:19:41Z

mempool/src/mempool.rs

+    pub const DEFAULT_SIZE_LIMIT: usize = 12_000_000;
+
+    /// Default total size limit of control transactions in the mempool (bytes)
+    pub const DEFAULT_CONTROL_SIZE_LIMIT: usize = 6_000_000;

    /// Creates a new mempool
    pub fn new(blockchain: Arc<RwLock<Blockchain>>, config: MempoolConfig) -> Self {
        let state = MempoolState {


Maybe add a MempoolState::new(size_limit: usize) -> MempoolState { ... }

nibhar · 2022-07-11T15:00:48Z

mempool/src/mempool.rs

+    /// Returns a vector with accepted control transactions from the mempool.
+    ///
+    /// Returns the highest fee per byte up to max_bytes transactions and removes them from the mempool
+    pub fn get_control_transactions_for_block(


I moved the logic for the expired transactions into the MempoolState. I did not do it for get_transactions_for_block because I am unsure wether or not more fine grained control over the byte size in particular is required.
However I believe even then a function like:

impl MempoolState { pub fn get_transactions_for_block(&self, max_bytes: (usize, usize)) -> Vec<Transactions>) { ... } }

might be the way to go here. One thing in particular that I see is that for both the regular and the control transactions a self.state.upgradable_read() and a subsequent upgrade of the lock are required. Moving the logic to the MempoolState would remove that. The code is also pretty much identical, aside from which field to access. Abstracting what transactions shall be retrieved might also be a good idea.

If that is not desirable, I probably would still move both these functions in the more lightweight MempoolState and just call them from here after taking the lock.

if we move to the pub fn get_transactions_for_block(&self, max_bytes: (usize, usize)) format, how would you specify what we are currently doing? i.e.: we know how much available bytes we have in the block, and we want to fill the block with control txns and if there is space left, fill the remaining with regular txns?

I still agree about the other benefits, the code its pretty much identical and it would be a good idea to move it to the MempoolState

- Introduced a second mempool, used for control transactions - When blocks are produced, first we try to include control txns - If there is space left, we include regular txns - This means that control txns have a higher priority over reg txns.

Create a new config parameter to limit the amount of transactions that can be kept in the control mempool

* Move some structs to their own modules All logic remains unchanged, this commit only moves code around. * Only use a single MempoolExecutor by using generic Topic * Add private fn start_executor and reuse where applicable * Move expired_transactions function to MempoolState

- Small warning/compilation fixes - Return control txns first, regular txns second - Share the max number of verif tasks among multiple executors - create constructor for mempool state

viquezclaudio added the WIP This pull request is still work in progress label Jun 14, 2022

viquezclaudio force-pushed the viquezcl/second_mempool branch from 314cdb8 to 2abf491 Compare June 14, 2022 22:59

sisou requested changes Jun 15, 2022

View reviewed changes

viquezclaudio force-pushed the viquezcl/second_mempool branch from 2abf491 to 5eb0e38 Compare June 15, 2022 20:02

viquezclaudio force-pushed the viquezcl/second_mempool branch from c10ba04 to 34ed994 Compare June 20, 2022 21:34

sisou approved these changes Jun 20, 2022

View reviewed changes

viquezclaudio removed the WIP This pull request is still work in progress label Jun 22, 2022

viquezclaudio force-pushed the viquezcl/second_mempool branch 2 times, most recently from ef1e197 to bafa796 Compare June 30, 2022 19:08

viquezclaudio changed the title ~~Implementation of second mempool~~ Second (control) mempool implementation Jul 5, 2022

viquezclaudio force-pushed the viquezcl/second_mempool branch from bafa796 to 20a0980 Compare July 5, 2022 19:44

viquezclaudio mentioned this pull request Jul 5, 2022

Mempool txn prioritization #920

Merged

nibhar force-pushed the viquezcl/second_mempool branch from 20a0980 to df8fd9d Compare July 11, 2022 14:07

nibhar reviewed Jul 11, 2022

View reviewed changes

viquezclaudio added the WIP This pull request is still work in progress label Jul 11, 2022

viquezclaudio force-pushed the viquezcl/second_mempool branch 3 times, most recently from df6114a to 77e1b99 Compare July 12, 2022 20:27

viquezclaudio removed the WIP This pull request is still work in progress label Jul 12, 2022

viquezclaudio force-pushed the viquezcl/second_mempool branch from 77e1b99 to e4f9a72 Compare July 12, 2022 21:28

viquezclaudio and others added 4 commits July 12, 2022 18:03

Implementation of second mempool

f8792ec

- Introduced a second mempool, used for control transactions - When blocks are produced, first we try to include control txns - If there is space left, we include regular txns - This means that control txns have a higher priority over reg txns.

Include control mempool config option

7f09673

Create a new config parameter to limit the amount of transactions that can be kept in the control mempool

Small fixes to mempool code

f75c6fd

- Small warning/compilation fixes - Return control txns first, regular txns second - Share the max number of verif tasks among multiple executors - create constructor for mempool state

viquezclaudio force-pushed the viquezcl/second_mempool branch from e4f9a72 to f75c6fd Compare July 13, 2022 00:04

viquezclaudio merged commit f75c6fd into albatross Jul 13, 2022

viquezclaudio deleted the viquezcl/second_mempool branch July 13, 2022 00:04

jsdanielh mentioned this pull request Aug 19, 2022

Create a second mempool for control transactions #897

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Second (control) mempool implementation #885

Second (control) mempool implementation #885

viquezclaudio commented Jun 14, 2022

codecov bot commented Jun 14, 2022 •

edited

Loading

viquezclaudio commented Jun 20, 2022

nibhar left a comment

nibhar Jul 7, 2022

viquezclaudio Jul 11, 2022

nibhar Jul 11, 2022

nibhar Jul 11, 2022

viquezclaudio Jul 11, 2022

viquezclaudio Jul 11, 2022

Second (control) mempool implementation #885

Second (control) mempool implementation #885

Conversation

viquezclaudio commented Jun 14, 2022

codecov bot commented Jun 14, 2022 • edited Loading

Codecov Report

viquezclaudio commented Jun 20, 2022

nibhar left a comment

Choose a reason for hiding this comment

nibhar Jul 7, 2022

Choose a reason for hiding this comment

viquezclaudio Jul 11, 2022

Choose a reason for hiding this comment

nibhar Jul 11, 2022

Choose a reason for hiding this comment

nibhar Jul 11, 2022

Choose a reason for hiding this comment

viquezclaudio Jul 11, 2022

Choose a reason for hiding this comment

viquezclaudio Jul 11, 2022

Choose a reason for hiding this comment

codecov bot commented Jun 14, 2022 •

edited

Loading