[DONT MERGE] cryptonote_core: move validation to after sync operation. #13
Conversation
src/cryptonote_core/tx_pool.cpp
Outdated
| @@ -1060,6 +1060,48 @@ namespace cryptonote | |||
| return true; | |||
| } | |||
| //------------------------------------------------------------------ | |||
| void tx_memory_pool::update_txpool_from_chain(size_t start_chain_height, size_t end_chain_height){ | |||
| MGINFO_YELLOW("Starting from block " << start_chain_height << " till block " << end_chain_height << " and removing already mined txis from txpool"); | |||
| std::vector<crypto::hash> txvector; | |||
There was a problem hiding this comment.
As far as I can see MGINFO_YELLOW is hardly used, or hardly used anymore, in the Monero codebase. Usually you choose from the "semantic" macros MINFO, MDEBUG, MERROR and so on, so people can use the set_log command and choose the detail / severity level they want to see.
There was a problem hiding this comment.
Thanks for the comment. This is just for debugging and temporary.
There was a problem hiding this comment.
Well, if you make it MDEBUG you don't even have to remove it. Somebody else may want to debug something here in a year's time, and the message may be useful again, no?
The codebase is full with such messages, and I do think they are useful and can stay.
There was a problem hiding this comment.
Yes, you are right. Final PR will of course have detailed debug messages ( and no MGINFO_YELLOW ).
| @@ -679,9 +679,14 @@ namespace cryptonote | |||
| r = m_mempool.init(max_txpool_weight, m_nettype == FAKECHAIN); | |||
| CHECK_AND_ASSERT_MES(r, false, "Failed to initialize memory pool"); | |||
|
|
|||
| session_start_height = get_current_blockchain_height(); | |||
| MGINFO_YELLOW("Current height of the chain : " << session_start_height); | |||
There was a problem hiding this comment.
I was thinking about what happens if I start the daemon, let it sync for a while, but before it reaches top of chain, I exit and then restart.
I am pretty sure nothing really bad happens. The daemon just can remove away less transactions from the pool, because the session_start_hight of the first, incomplete run is of course lost, and the second start height gets used fro pool cleaning.
|
Code and approach look good to me. |
0xFFFC0000
force-pushed
the
dev/0xfffc/sync-then-verify-monerod
branch
4 times, most recently
from
11f1d30 to
9723277
Compare
|
I spoke with @0xFFFC0000 about this but I want to put my thoughts here as well. The txpool is currently heavily relied on during syncing, when handling an incoming batch of blocks the transactions are always added to the tx pool first, which will then do consensus checks, some of which are not done elsewhere. So by always not re-validating the whole txpool on startup we run into this possible situation:
This wouldn't be easy to exploit, needing to know when a node restarts, what txs it has in the pool, and the node must restart at a HF boundary. However my main concern would be the potential unknown consequences of this change, with how critical the txpool currently is in the sync process, plus this is another thing we will have to be mindful of when adding new consensus rules. This potential issue would be mitigated by: monero-project#9135 which removes the txpool from the sync process. Another solution to this problem (slow startup) would be to skip validation if the HF hasn't changed from the top blocks HF, this would also be a much smaller change to review. The only problem with this is if a new relay rule is added the txs in the pool at startup which violate this rule would not be removed, I don't think this a big issue as they were already in the pool before the restart so most likely have already been passed around the network, if not they will be removed from the nodes pool after ~3 days. |
This will help to reduce the validation overhead when starting up monerod.
0xFFFC0000
force-pushed
the
dev/0xfffc/sync-then-verify-monerod
branch
from
9723277 to
9f94382
Compare
This will help to reduce the validation overhead when starting up monerod.