tx relay v2 [fcmp++-stage]#288
Closed
j-berman wants to merge 77 commits intoseraphis-migration:fcmp++-stagefrom
Closed
tx relay v2 [fcmp++-stage]#288j-berman wants to merge 77 commits intoseraphis-migration:fcmp++-stagefrom
j-berman wants to merge 77 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
…rint cryptonote_basic: remove BP+ clawback debug print in weight func
…d_designator carrot_impl: specify all fields in aggregate init (1)
process_new_scanned_transaction perf timer is very noisy
…::view_key_scalar_mult_x25519
…ddr_dev carrot_impl: fix infinite loop in cryptonote_hierarchy_address_device…
txpool: don't drop cnxn that relays high FCMP++ ref block
Reduces logging noise and speeds up some algos
carrot_impl: filter 0-amount inputs before input selection
txpool: don't process txs that would push pool over capacity
- Replaced OutputPairType with the OutputPair variant. - Moved OutputPair into fcmp_pp_types.h and its serialization fn's into fcmp_pp_serialization.h. - Consolidated logic for determining if an output has been/should be checked for torsion, AND the logic for if we should use the biased hash to point for the key image generator, into cryptonote_format_utils. This provides a stronger guarantee we'll correctly update the logic as needed in the future, and keeps the logic organized neatly in one place. - Used inlines in cryptonote_format_utils.h so that the fcmp_pp lib does not have to link cryptonote_basic, triggering a circular dep: cryptonote_basic->ringct_basic->fcmp_pp->cryptonote_basic
Cleaner structure for biased key gen/torsion checking
Unbiased hash-to-point integration
* Tx Relay v1: New Relay Logic with threshold-based peer dropping. Co-authored-by: Boog900 <boog900@tutanota.com> * Fixed extra delete directives. Only delete methods that compiler generates implicitly. * tx-relay-v2: * Removed extra classes. * Used on_idle as callback mechanism. * tx-relay-v2: add maximum in_flight requests for each connection * change internal txrequest representation to multiindex hash map * Addressing second round of comments * fix 9933#discussion_r2366464294 * fix for 9933#discussion_r2366462339 and 9933#discussion_r2366458244 * fix for 9933#discussion_r2366295474 * fix for 9933#discussion_r2366482602 * fix for 9933#discussion_r2366489004 * fix for 9933#discussion_r2366493055 * improvements, fix the deadline logic, better logging * new sets of fixes and improvements * addressing first round of jberman comments * Bulk changes for tx relay v2 - Only requests a specific tx hash from a single peer at a time. - When we receive a tx hash not yet known to us, but connection is at its in-flight limit, we queue it to request later. - The tx request checker runs in a 5s interval now, faster than the timeout, in order to fire off these requests in the queue that we're able to (see `fly_available_requests`). - We never request more than the `max_n_txs_per_packet` at a time from a connection, avoiding connection drops from doing so. - Handles adding a request and all associated metadata with one `add_request` function. - Makes sure to remove tx requests from all peers for a specific tx hash as soon as we add the tx to our db. - Handles removing a request and all associated metadata with one `remove_request` function. - Modifies `handle_incoming_tx` to return the calculated tx hash. - This avoids a circumstance where the node fails to verify a tx with something that's a no-drop-offense (i.e. an expected failure), and we don't mark the tx request as received. - Also avoids the extra parsing & hashing. - Deletes lots of unused logic/metadata/functions. - Places `m_connection_stats` back in the request manager. - I think this initial decision by @0Xff was correct. - This drastically simplifies synchronized updating of stats relevant to the request manager, and clearly demonstrates what logic the request manager needs to track. - If `remove_peer` does not execute upon dropping a peer, then the request data won't get cleaned up also. So we make sure to clean up in the same place. - The extra uuid mapping is insignificant on memory. - Pass tx_hashes directly into send_txs/relay_txs. - Add unit tests for the request manager. * @Boog900 review comments * @Boog900 review rd2 --------- Co-authored-by: Boog900 <boog900@tutanota.com> Co-authored-by: j-berman <justinberman@protonmail.com>
Collaborator
|
I think that the upstream PR monero-project#9933 has had recent changes |
jeffro256
reviewed
Mar 7, 2026
Comment on lines
-875
to
-877
| // Avoid tripping the packet size limit by serving txs in batches | ||
| static const std::size_t SLICE_SIZE = context.get_max_bytes(NOTIFY_NEW_TRANSACTIONS::ID) / get_max_tx_size(); | ||
| if (SLICE_SIZE == 0) |
Collaborator
There was a problem hiding this comment.
So we do want to relay an incomplete pool versus doing it in slices?
Collaborator
Author
There was a problem hiding this comment.
@Boog900 requested the change here, and raised DoS concerns here. I think the change is a reasonable one because the connection code really isn't designed to write many huge sliced responses to a single request: this section shows that could cause issues by tripping the limiter in place. Missing some txs here seems better than ending up dropping the connection.
Collaborator
Author
|
Merged into |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#184