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

tx_pool: full tx revalidation on fork boundaries #7169

Merged
merged 1 commit into from
Nov 10, 2021
Merged

tx_pool: full tx revalidation on fork boundaries #7169

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
21 changes: 21 additions & 0 deletions src/cryptonote_core/blockchain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,7 @@ block Blockchain::pop_block_from_blockchain()

CHECK_AND_ASSERT_THROW_MES(m_db->height() > 1, "Cannot pop the genesis block");

const uint8_t previous_hf_version = get_current_hard_fork_version();
try
{
m_db->pop_block(popped_block, popped_txs);
Expand Down Expand Up @@ -650,6 +651,13 @@ block Blockchain::pop_block_from_blockchain()
m_tx_pool.on_blockchain_dec(top_block_height, top_block_hash);
invalidate_block_template_cache();

const uint8_t new_hf_version = get_current_hard_fork_version();
if (new_hf_version != previous_hf_version)
{
MINFO("Validating txpool for v" << (unsigned)new_hf_version);
m_tx_pool.validate(new_hf_version);
}

return popped_block;
}
//------------------------------------------------------------------
Expand Down Expand Up @@ -4392,6 +4400,19 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&
get_difficulty_for_next_block(); // just to cache it
invalidate_block_template_cache();

const uint8_t new_hf_version = get_current_hard_fork_version();
if (new_hf_version != hf_version)
{
// the genesis block is added before everything's setup, and the txpool is empty
// when we start from scratch, so we skip this
const bool is_genesis_block = new_height == 1;
if (!is_genesis_block)
{
MGINFO("Validating txpool for v" << (unsigned)new_hf_version);
m_tx_pool.validate(new_hf_version);
}
}

send_miner_notifications(id, already_generated_coins);

for (const auto& notifier: m_block_notifiers)
Expand Down
88 changes: 43 additions & 45 deletions src/cryptonote_core/tx_pool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1568,61 +1568,59 @@ namespace cryptonote
{
CRITICAL_REGION_LOCAL(m_transactions_lock);
CRITICAL_REGION_LOCAL1(m_blockchain);
size_t tx_weight_limit = get_transaction_weight_limit(version);
std::unordered_set<crypto::hash> remove;

m_txpool_weight = 0;
m_blockchain.for_all_txpool_txes([this, &remove, tx_weight_limit](const crypto::hash &txid, const txpool_tx_meta_t &meta, const cryptonote::blobdata_ref*) {
m_txpool_weight += meta.weight;
if (meta.weight > tx_weight_limit) {
LOG_PRINT_L1("Transaction " << txid << " is too big (" << meta.weight << " bytes), removing it from pool");
remove.insert(txid);
}
else if (m_blockchain.have_tx(txid)) {
LOG_PRINT_L1("Transaction " << txid << " is in the blockchain, removing it from pool");
remove.insert(txid);
}
MINFO("Validating txpool contents for v" << (unsigned)version);

LockedTXN lock(m_blockchain.get_db());

struct tx_entry_t
{
crypto::hash txid;
txpool_tx_meta_t meta;
};

// get all txids
std::vector<tx_entry_t> txes;
m_blockchain.for_all_txpool_txes([this, &txes](const crypto::hash &txid, const txpool_tx_meta_t &meta, const cryptonote::blobdata_ref*) {
if (!meta.pruned) // skip pruned txes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, my reading of this is that at the fork, pruned nodes won't have any validation logic a number of their tx's meet the fork's new rules (even on tx weight). So at fork time, pruned nodes would be relying on non-pruned nodes to avoid any issues, which seems alright? Am I reading that right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In normal "cruise" usage, transactions get pruned after 5500 blocks.
The only way a pruned tx can get into the txpool is when it is an old tx that was requested by a syncing node. In that case, this only gets requested if the node has a known block hash to compare it with, and this test will not matter.

txes.push_back({txid, meta});
return true;
}, false, relay_category::all);

size_t n_removed = 0;
if (!remove.empty())
// take them all out and add them back in, some might fail
size_t added = 0;
for (auto &e: txes)
{
LockedTXN lock(m_blockchain.get_db());
for (const crypto::hash &txid: remove)
try
{
try
{
cryptonote::blobdata txblob = m_blockchain.get_txpool_tx_blob(txid, relay_category::all);
cryptonote::transaction tx;
if (!parse_and_validate_tx_from_blob(txblob, tx)) // remove pruned ones on startup, they're meant to be temporary
{
MERROR("Failed to parse tx from txpool");
continue;
}
// remove tx from db first
m_blockchain.remove_txpool_tx(txid);
m_txpool_weight -= get_transaction_weight(tx, txblob.size());
remove_transaction_keyimages(tx, txid);
auto sorted_it = find_tx_in_sorted_container(txid);
if (sorted_it == m_txs_by_fee_and_receive_time.end())
{
LOG_PRINT_L1("Removing tx " << txid << " from tx pool, but it was not found in the sorted txs container!");
}
else
{
m_txs_by_fee_and_receive_time.erase(sorted_it);
}
++n_removed;
}
catch (const std::exception &e)
size_t weight;
uint64_t fee;
cryptonote::transaction tx;
cryptonote::blobdata blob;
bool relayed, do_not_relay, double_spend_seen, pruned;
if (!take_tx(e.txid, tx, blob, weight, fee, relayed, do_not_relay, double_spend_seen, pruned))
MERROR("Failed to get tx " << e.txid << " from txpool for re-validation");

cryptonote::tx_verification_context tvc{};
relay_method tx_relay = e.meta.get_relay_method();
if (!add_tx(tx, e.txid, blob, e.meta.weight, tvc, tx_relay, relayed, version))
{
MERROR("Failed to remove invalid tx from pool");
// continue
MINFO("Failed to re-validate tx " << e.txid << " for v" << (unsigned)version << ", dropped");
continue;
}
m_blockchain.update_txpool_tx(e.txid, e.meta);
++added;
}
catch (const std::exception &e)
{
MERROR("Failed to re-validate tx from pool");
continue;
}
lock.commit();
}

lock.commit();

const size_t n_removed = txes.size() - added;
if (n_removed > 0)
++m_cookie;
return n_removed;
Expand Down
15 changes: 10 additions & 5 deletions tests/unit_tests/long_term_block_weight.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,16 @@ static uint32_t lcg()

}

struct BlockchainAndPool
{
cryptonote::tx_memory_pool txpool;
cryptonote::Blockchain bc;
BlockchainAndPool(): txpool(bc), bc(txpool) {}
};

#define PREFIX_WINDOW(hf_version,window) \
std::unique_ptr<cryptonote::Blockchain> bc; \
cryptonote::tx_memory_pool txpool(*bc); \
bc.reset(new cryptonote::Blockchain(txpool)); \
BlockchainAndPool bap; \
cryptonote::Blockchain *bc = &bap.bc; \
struct get_test_options { \
const std::pair<uint8_t, uint64_t> hard_forks[3]; \
const cryptonote::test_options test_options = { \
Expand All @@ -118,8 +124,7 @@ static uint32_t lcg()
}; \
get_test_options(): hard_forks{std::make_pair(1, (uint64_t)0), std::make_pair((uint8_t)hf_version, (uint64_t)1), std::make_pair((uint8_t)0, (uint64_t)0)} {} \
} opts; \
cryptonote::Blockchain *blockchain = bc.get(); \
bool r = blockchain->init(new TestDB(), cryptonote::FAKECHAIN, true, &opts.test_options, 0, NULL); \
bool r = bc->init(new TestDB(), cryptonote::FAKECHAIN, true, &opts.test_options, 0, NULL); \
ASSERT_TRUE(r)

#define PREFIX(hf_version) PREFIX_WINDOW(hf_version, TEST_LONG_TERM_BLOCK_WEIGHT_WINDOW)
Expand Down