tx_memory_pool: quick abort on block templates for too-young FCMP txs#220
tx_memory_pool: quick abort on block templates for too-young FCMP txs#220jeffro256 wants to merge 19 commits intoseraphis-migration:fcmp++-stagefrom
Conversation
This replaces `ver_rct_non_semantics_simple_cached()` with an API that offloads the responsibility of tracking input verification successes to the caller. The main caller of this function in the codebase, `cryptonote::Blockchain()` instead keeps track of the verification results for transaction in the mempool by storing a "verification ID" in the mempool metadata table (with `txpool_tx_meta_t`). This has several benefits, including: * When the mempool is large (>8192 txs), we no longer experience cache misses and unnecessarily re-verify ring signatures. This greatly improves block propagation time for FCMP++ blocks under load * For the same reason, reorg handling can be sped up by storing verification IDs of transactions popped from the chain * Speeds up re-validating every mempool transaction on fork change (monerod revalidates the whole tx-pool on HFs monero-project#10142) * Caches results for every single type of Monero transaction, not just latest RCT type * Cache persists over a node restart * Uses 512KiB less RAM (8192*2*32B) * No additional storage or DB migration required since `txpool_tx_meta_t` already had padding allocated * Moves more verification logic out of `cryptonote::Blockchain` Furthermore, this opens the door to future multi-threaded block verification speed-ups. Right now, transactions' input proof verification is limited to one transaction at a time. However, one can imagine a scenario with verification IDs where input proofs are optimistically multi-threaded in advance of block processing. Then, even though ring member fetching and verification is single-threaded inside of `cryptonote::Blockchain::check_tx_inputs()`, the single thread can skip the CPU-intensive cryptographic code if the verification ID allows it. Also changes the default log category in `tx_verification_utils.cpp` from "blockchain" to "verify".
Co-authored-by: j-berman <justinberman@protonmail.com>
Co-authored-by: jeffro256 <jeffro256@tutanota.com> Co-authored-by: Luke Parker <lukeparker5132@gmail.com> Co-authored-by: SyntheticBird45 <someoneelse.is_on.github.rio7x@simplelogin.com>
Otherwise we can end up double counting txs towards the weight, which can over-state the pool weight. E.g. relay tx to node in stem phase, add its weight to pool weight, then receive tx from another node, then bump the pool weight again. That double counts the tx towards the pool weight. If the weight exceeds the max, the node will "prune" txs from the pool. Thus, over-counting is probably a cause of, but perhaps not the only cause of: seraphis-migration#148
Curve Trees: handle get_max_concurrency() == 0
tx pool: only increment m_txpool_weight for newly added pool txs
Fixes pruning the database under FCMP++ and prevents future corruption by checking the version value inside the properties table.
Log total pool weight
…prune blockchain_prune: add FCMP tables and check DB version
…ver-ids Fix FCMP++ batch verification collecting ver ID's
fcmp++ rust: ASAN support
Also add documention.
…arrot_devs carrot_impl: refactor scanning_tools to use Carrot devices
src/cryptonote_core/tx_pool.cpp
Outdated
| if (txd.hard_height_requirement) | ||
| { | ||
| const uint64_t curr_height = m_blockchain.get_current_blockchain_height(); | ||
| if (txd.max_used_block_height + CRYPTONOTE_DEFAULT_TX_SPENDABLE_AGE > curr_height) |
There was a problem hiding this comment.
Idea: if you have max_used_block_height=reference_block for FCMP++ txs, you can just check txd.max_used_block_height >= curr_height here
We wouldn't need the hard_height_requirement field, and can call this for any tx. It also avoids any issues surrounding off-by-1 logic
It introduces a difference on what max_used_block_height means for pre and post FCMP++ txs (which affects the if for // enforce min output age in blockchain.cpp), so I would accept if you prefer not to do that
There was a problem hiding this comment.
We wouldn't need the hard_height_requirement field, and can call this for any tx. It also avoids any issues surrounding off-by-1 logic
We can't use that height without re-checking for ring signature transactions because a reorg can change the height of referenced ring members. It's different for FCMP++ txs because the reference height is explicitly a height value and isn't allowed by consensus any lower than at least 1 block after the reference block height.
For example. if the top ring member gets re-orged from height H to height H-1, this code would skip over this transaction in the block template for the next block. Eventually, this would be eligible again, so maybe it's okay?
It introduces a difference on what max_used_block_height means for pre and post FCMP++ txs (which affects the if for // enforce min output age in blockchain.cpp), so I would accept if you prefer not to do that
This is also one of the main reasons why I chose to do it this way: I wanted to behavior to match what's happening with the ring signature transactions.
There was a problem hiding this comment.
Good point, you're right. As is is ok with me, will review closer for off-by-1
There was a problem hiding this comment.
Looks good, always tricky
j-berman
left a comment
j-berman
left a comment
There was a problem hiding this comment.
Small nit on passing db height into is_transaction_meta_ready_to_go, otherwise LGTM
src/cryptonote_core/tx_pool.cpp
Outdated
| return ret; | ||
| } | ||
| //--------------------------------------------------------------------------------- | ||
| bool tx_memory_pool::is_transaction_meta_ready_to_go(const txpool_tx_meta_t& txd) const |
There was a problem hiding this comment.
Could pass in the db height to prevent the db read for height for all txs in a loop
| *pmax_used_block_height = tx.rct_signatures.p.reference_block | ||
| - std::min<std::uint64_t>(tx.rct_signatures.p.reference_block, CRYPTONOTE_DEFAULT_TX_SPENDABLE_AGE - 1); |
There was a problem hiding this comment.
Note for self:
I use a similar pattern in db_lmdb.cpp here
I think it would be good to use a helper function for this logic just so it's consolidated somewhere / makes it easier to manage if there is some change down the road
I can do it in a separate PR
src/cryptonote_core/tx_pool.cpp
Outdated
| if (txd.hard_height_requirement) | ||
| { | ||
| const uint64_t curr_height = m_blockchain.get_current_blockchain_height(); | ||
| if (txd.max_used_block_height + CRYPTONOTE_DEFAULT_TX_SPENDABLE_AGE > curr_height) |
There was a problem hiding this comment.
Looks good, always tricky
2 participants
No description provided.