cryptonote_core: only verify txpool when the hardfork value has changed. #9404

0xFFFC0000 · 2024-07-22T10:01:10Z

Detailed discussion here.

Co-authored-by: Boog900 <[email protected]>

SChernykh · 2024-07-24T20:10:32Z

src/cryptonote_core/cryptonote_core.cpp

@@ -679,9 680,21 @@ namespace cryptonote
 r = m_mempool.init(max_txpool_weight, m_nettype == FAKECHAIN);
 CHECK_AND_ASSERT_MES(r, false, "Failed to initialize memory pool");

+ uint64_t session_start_height = m_blockchain_storage.get_current_blockchain_height();
+ uint8_t hardfork_version_start = m_blockchain_storage.get_hard_fork_version(session_start_height);
+ uint8_t hardfork_version_current = m_blockchain_storage.get_current_hard_fork_version();


Will these two values ever be different at this point? This is core::init, the node hasn't started syncing at this point and is only loading existing blocks that it synced during last run.

The discussion mentions that it can change if the node itself is updated in between runs, but it needs to be tested. So far it's not obvious that these two versions will be different even in this case.

heights contains the HFs and current_for_index is the index of our current HF in the list.

The case we want to verify is when we shutdown at boundary, and start up in next hf.

What if someone shuts down a few heights above the boundary (they were late to the fork) and restarts with the new version?

They would be on an invalid chain, I have thought about this and I don't think there is a good solution to this while we support voting for hardforks.

You would need to check all past HFs, or at least the ones outside the checkpoints and pop blocks if any blocks have incorrect major versions.

This is a separate issue though IMO

@SChernykh I was wrong, we do handle this here:

monero/src/cryptonote_core/blockchain.cpp

Lines 391 to 429 in caa62bc

while (!m_db->is_read_only())

{

uint64_t top_height;

const crypto::hash top_id = m_db->top_block_hash(&top_height);

const block top_block = m_db->get_top_block();

const uint8_t ideal_hf_version = get_ideal_hard_fork_version(top_height);

if (ideal_hf_version <= 1 || ideal_hf_version == top_block.major_version)

{

if (num_popped_blocks > 0)

MGINFO("Initial popping done, top block: " << top_id << ", top height: " << top_height << ", block version: " << (uint64_t)top_block.major_version);

break;

}

else

{

if (num_popped_blocks == 0)

MGINFO("Current top block " << top_id << " at height " << top_height << " has version " << (uint64_t)top_block.major_version << " which disagrees with the ideal version " << (uint64_t)ideal_hf_version);

if (num_popped_blocks % 100 == 0)

MGINFO("Popping blocks... " << top_height);

num_popped_blocks;

block popped_block;

std::vector<transaction> popped_txs;

try

{

m_db->pop_block(popped_block, popped_txs);

}

// anything that could cause this to throw is likely catastrophic,

// so we re-throw

catch (const std::exception& e)

{

MERROR("Error popping block from blockchain: " << e.what());

throw;

}

catch (...)

{

MERROR("Error popping block from blockchain, throwing!");

throw;

}

}

}

The way we handle this though pretty much removes the possibility of using the voting for hardforks feature. We pop blocks until the minimum possible height of the HF, not the height it should activate according to votes, so even if the fork isn't active we will pop blocks.

SChernykh · 2024-07-24T20:31:13Z

Second question, what about 3rd-party tools changing the database between monerod runs? By introducing this code, you assume that nothing ever touches the txpool table except monerod.

0xFFFC0000 · 2024-07-24T20:42:32Z

Second question, what about 3rd-party tools changing the database between monerod runs? By introducing this code, you assume that nothing ever touches the txpool table except monerod.

IMHO that is an edge case. Where we can have a flag like --verify-txpool and user would verify its txpool. Average user doesn't need to verify txpool each time during monerod start up.

I might be wrong. Please don't hesitate to let me know.

Boog900 · 2024-07-24T22:25:12Z

Second question, what about 3rd-party tools changing the database between monerod runs? By introducing this code, you assume that nothing ever touches the txpool table except monerod.

If tools are touching the DB it is up to them to make sure they are doing it correctly, there are many ways to currently mess up the blockchain by messing with the database.

There are also checks currently ran on the way to the txpool but not in txpool::validate, so we are currently relying on 3rd-party tools adding txs to the pool to not violate those rules.

sneurlax

These changes LGTM, but I did not test by simulating a hardfork.

cryptonote_core: only verify txpool when the hardfork value has changed.

12ce046

Co-authored-by: Boog900 <[email protected]>

0xFFFC0000 added enhancement pending review easy labels Jul 22, 2024

SChernykh reviewed Jul 24, 2024

View reviewed changes

0xFFFC0000 removed the easy label Jul 24, 2024

Rucknium mentioned this pull request Jul 26, 2024

Monero Research Lab Meeting - Wed 24 July 2024, 17:00 UTC monero-project/meta#1044

Closed

sneurlax approved these changes Aug 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cryptonote_core: only verify txpool when the hardfork value has changed. #9404

cryptonote_core: only verify txpool when the hardfork value has changed. #9404

0xFFFC0000 commented Jul 22, 2024

SChernykh Jul 24, 2024

SChernykh Jul 24, 2024

0xFFFC0000 Jul 24, 2024

SChernykh Jul 24, 2024

Boog900 Jul 24, 2024

Boog900 Aug 9, 2024

SChernykh commented Jul 24, 2024

0xFFFC0000 commented Jul 24, 2024

Boog900 commented Jul 24, 2024

sneurlax left a comment

	while (!m_db->is_read_only())
	{
	uint64_t top_height;
	const crypto::hash top_id = m_db->top_block_hash(&top_height);
	const block top_block = m_db->get_top_block();
	const uint8_t ideal_hf_version = get_ideal_hard_fork_version(top_height);
	if (ideal_hf_version <= 1 \|\| ideal_hf_version == top_block.major_version)
	{
	if (num_popped_blocks > 0)
	MGINFO("Initial popping done, top block: " << top_id << ", top height: " << top_height << ", block version: " << (uint64_t)top_block.major_version);
	break;
	}
	else
	{
	if (num_popped_blocks == 0)
	MGINFO("Current top block " << top_id << " at height " << top_height << " has version " << (uint64_t)top_block.major_version << " which disagrees with the ideal version " << (uint64_t)ideal_hf_version);
	if (num_popped_blocks % 100 == 0)
	MGINFO("Popping blocks... " << top_height);
	num_popped_blocks;
	block popped_block;
	std::vector<transaction> popped_txs;
	try
	{
	m_db->pop_block(popped_block, popped_txs);
	}
	// anything that could cause this to throw is likely catastrophic,
	// so we re-throw
	catch (const std::exception& e)
	{
	MERROR("Error popping block from blockchain: " << e.what());
	throw;
	}
	catch (...)
	{
	MERROR("Error popping block from blockchain, throwing!");
	throw;
	}
	}
	}

cryptonote_core: only verify txpool when the hardfork value has changed. #9404

Are you sure you want to change the base?

cryptonote_core: only verify txpool when the hardfork value has changed. #9404

Conversation

0xFFFC0000 commented Jul 22, 2024

SChernykh Jul 24, 2024

Choose a reason for hiding this comment

SChernykh Jul 24, 2024

Choose a reason for hiding this comment

0xFFFC0000 Jul 24, 2024

Choose a reason for hiding this comment

SChernykh Jul 24, 2024

Choose a reason for hiding this comment

Boog900 Jul 24, 2024

Choose a reason for hiding this comment

Boog900 Aug 9, 2024

Choose a reason for hiding this comment

SChernykh commented Jul 24, 2024

0xFFFC0000 commented Jul 24, 2024

Boog900 commented Jul 24, 2024

sneurlax left a comment

Choose a reason for hiding this comment