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.

Sign up for GitHub

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

Jump to bottom

tx_pool: update internal data structure to boost::bimap. #9376

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

tx_pool: update internal data structure to boost::bimap. #9376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 19 additions & 17 deletions src/cryptonote_core/tx_pool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
// Parts of this file are originally copyright (c) 2012-2013 The Cryptonote developers

#include <algorithm>
#include <boost/bimap/support/iterator_type_by.hpp>
#include <boost/filesystem.hpp>
#include <unordered_set>
#include <vector>
Expand All @@ -42,6 +43,7 @@
#include "blockchain_db/blockchain_db.h"
#include "int-util.h"
#include "misc_language.h"
#include "misc_log_ex.h"
#include "warnings.h"
#include "common/perf_timer.h"
#include "crypto/hash.h"
Expand Down Expand Up @@ -464,7 +466,7 @@ namespace cryptonote
break;
try
{
const crypto::hash &txid = it->second;
const crypto::hash &txid = it->get_right();
txpool_tx_meta_t meta;
if (!m_blockchain.get_txpool_tx_meta(txid, meta))
{
Expand All @@ -491,11 +493,11 @@ namespace cryptonote
return;
}
// remove first, in case this throws, so key images aren't removed
MINFO("Pruning tx " << txid << " from txpool: weight: " << meta.weight << ", fee/byte: " << it->first.first);
MINFO("Pruning tx " << txid << " from txpool: weight: " << meta.weight << ", fee/byte: " << it->get_left().first);
m_blockchain.remove_txpool_tx(txid);
reduce_txpool_weight(meta.weight);
remove_transaction_keyimages(tx, txid);
MINFO("Pruned tx " << txid << " from txpool: weight: " << meta.weight << ", fee/byte: " << it->first.first);
MINFO("Pruned tx " << txid << " from txpool: weight: " << meta.weight << ", fee/byte: " << it->get_left().first);

auto it_prev = it;
--it_prev;
Expand Down Expand Up @@ -751,13 +753,9 @@ namespace cryptonote
m_remove_stuck_tx_interval.do_call([this](){return remove_stuck_transactions();});
}
//---------------------------------------------------------------------------------
sorted_tx_container::iterator tx_memory_pool::find_tx_in_sorted_container(const crypto::hash& id) const
sorted_tx_container::iterator tx_memory_pool::find_tx_in_sorted_container(const crypto::hash& id)
Copy link
Contributor

Choose a reason for hiding this comment

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

I looked at the documentation, and it looks like this function can remain const? What broke this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed that iterator has to be returned, so presumably the reason is find and project_up return const_iterator here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes you are right. If it remains const then the find will return const_iterator and that is causing a lot headache. We want to use that iterator in erase which doesn't accept const_iterator, and no easy way of turning const_iterator to iterator in boost.

{
return std::find_if( m_txs_by_fee_and_receive_time.begin(), m_txs_by_fee_and_receive_time.end()
, [&](const sorted_tx_container::value_type& a){
return a.second == id;
}
);
return m_txs_by_fee_and_receive_time.project_up(m_txs_by_fee_and_receive_time.right.find(id));
}
//---------------------------------------------------------------------------------
//TODO: investigate whether boolean return is appropriate
Expand Down Expand Up @@ -1644,15 +1642,15 @@ namespace cryptonote
for (; sorted_it != m_txs_by_fee_and_receive_time.end(); ++sorted_it)
{
txpool_tx_meta_t meta;
if (!m_blockchain.get_txpool_tx_meta(sorted_it->second, meta))
if (!m_blockchain.get_txpool_tx_meta(sorted_it->get_right(), meta))
{
static bool warned = false;
if (!warned)
MERROR(" failed to find tx meta: " << sorted_it->second << " (will only print once)");
MERROR(" failed to find tx meta: " << sorted_it->get_right() << " (will only print once)");
warned = true;
continue;
}
LOG_PRINT_L2("Considering " << sorted_it->second << ", weight " << meta.weight << ", current block weight " << total_weight << "/" << max_total_weight << ", current coinbase " << print_money(best_coinbase) << ", relay method " << (unsigned)meta.get_relay_method());
LOG_PRINT_L2("Considering " << sorted_it->get_right() << ", weight " << meta.weight << ", current block weight " << total_weight << "/" << max_total_weight << ", current coinbase " << print_money(best_coinbase) << ", relay method " << (unsigned)meta.get_relay_method());

if (!meta.matches(relay_category::legacy) && !(m_mine_stem_txes && meta.get_relay_method() == relay_method::stem))
{
Expand Down Expand Up @@ -1702,7 +1700,7 @@ namespace cryptonote
}

// "local" and "stem" txes are filtered above
cryptonote::blobdata txblob = m_blockchain.get_txpool_tx_blob(sorted_it->second, relay_category::all);
cryptonote::blobdata txblob = m_blockchain.get_txpool_tx_blob(sorted_it->get_right(), relay_category::all);

cryptonote::transaction tx;

Expand All @@ -1713,7 +1711,7 @@ namespace cryptonote
bool ready = false;
try
{
ready = is_transaction_ready_to_go(meta, sorted_it->second, txblob, tx);
ready = is_transaction_ready_to_go(meta, sorted_it->get_right(), txblob, tx);
}
catch (const std::exception &e)
{
Expand All @@ -1724,7 +1722,7 @@ namespace cryptonote
{
try
{
m_blockchain.update_txpool_tx(sorted_it->second, meta);
m_blockchain.update_txpool_tx(sorted_it->get_right(), meta);
}
catch (const std::exception &e)
{
Expand All @@ -1743,7 +1741,7 @@ namespace cryptonote
continue;
}

bl.tx_hashes.push_back(sorted_it->second);
bl.tx_hashes.push_back(sorted_it->get_right());
total_weight += meta.weight;
fee += meta.fee;
best_coinbase = coinbase;
Expand Down Expand Up @@ -1854,7 +1852,11 @@ namespace cryptonote
m_txs_by_fee_and_receive_time.erase(sorted_it);
}
}
m_txs_by_fee_and_receive_time.emplace(std::pair<double, time_t>(fee, receive_time), txid);
auto insert_result = m_txs_by_fee_and_receive_time.insert(sorted_tx_container::value_type(std::pair<double, time_t>(fee, receive_time), txid));

if (!insert_result.second) {
MERROR("Failed to add txid " << txid << " to the m_txs_by_fee_and_receive_time");
}

// Don't check for "resurrected" txs in case of reorgs i.e. don't check in 'm_removed_txs_by_time'
// whether we have that txid there and if yes remove it; this results in possible duplicates
Expand Down
30 changes: 21 additions & 9 deletions src/cryptonote_core/tx_pool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@
#include <queue>
#include <boost/serialization/version.hpp>
#include <boost/utility.hpp>
#include <boost/bimap.hpp>
#include <boost/bimap/set_of.hpp>
#include <boost/bimap/multiset_of.hpp>

#include "span.h"
#include "string_tools.h"
Expand All @@ -62,24 +65,33 @@ namespace cryptonote
//! pair of <transaction fee, transaction hash> for organization
typedef std::pair<std::pair<double, std::time_t>, crypto::hash> tx_by_fee_and_receive_time_entry;
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure that typedef is not needed anymore and can be deleted, in the interest of clean code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for your comment. Yes. it was not needed. But didn't want to make any unnecessary changes.


class txCompare
class txFeeCompare
{
public:
bool operator()(const tx_by_fee_and_receive_time_entry& a, const tx_by_fee_and_receive_time_entry& b) const
bool operator()(const std::pair<double, std::time_t>& a, const std::pair<double, std::time_t>& b) const
{
// sort by greatest first, not least
if (a.first.first > b.first.first) return true;
if (a.first.first < b.first.first) return false;
if (a.first > b.first) return true;
if (a.first < b.first) return false;

if (a.first.second < b.first.second) return true;
if (a.first.second > b.first.second) return false;
if (a.second < b.second) return true;
return false;
}
};

return memcmp(a.second.data, b.second.data, sizeof(crypto::hash)) < 0;
class hashCompare
{
public:
bool operator()(const crypto::hash& a, const crypto::hash& b) const
{
return memcmp(a.data, b.data, sizeof(crypto::hash)) < 0;
}
};

//! container for sorting transactions by fee per unit size
typedef std::set<tx_by_fee_and_receive_time_entry, txCompare> sorted_tx_container;
typedef boost::bimap<boost::bimaps::multiset_of<std::pair<double, std::time_t>, txFeeCompare>,
boost::bimaps::set_of<crypto::hash, hashCompare>> sorted_tx_container;
Copy link
Contributor

Choose a reason for hiding this comment

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

What about using unordered_set_of here? This could accelerate the lookup, but would likely require more memory (for the table), and perhaps resizing the table would be slow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you think? At this point it comes to the design choices, I am perfectly happy to change it to unordered_set_of.

Copy link
Contributor

Choose a reason for hiding this comment

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

Search times are a bit lower with an unordered_map, but insertion times may be slower with a large number of keys. I think we have more searches than insertions, but I am not certain. So just leave it for now, unless. you want to run another test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks a lot for detailed explanation.



/**
* @brief Transaction pool, handles transactions which are not part of a block
Expand Down Expand Up @@ -653,7 +665,7 @@ namespace cryptonote
*
* @return an iterator, possibly to the end of the container if not found
*/
sorted_tx_container::iterator find_tx_in_sorted_container(const crypto::hash& id) const;
sorted_tx_container::iterator find_tx_in_sorted_container(const crypto::hash& id);

//! cache/call Blockchain::check_tx_inputs results
bool check_tx_inputs(const std::function<cryptonote::transaction&(void)> &get_tx, const crypto::hash &txid, uint64_t &max_used_block_height, crypto::hash &max_used_block_id, tx_verification_context &tvc, bool kept_by_block = false) const;
Expand Down
Loading