cryptonote_core: cache input verification results directly in mempool #10157
Conversation
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
j-berman
reviewed
Oct 10, 2025
iamamyth
reviewed
Oct 10, 2025
Contributor
Author
|
The way that this would work together with the FCMP++ integration is that two new functions would be created in |
This was referenced Oct 11, 2025
8 tasks
jeffro256
commented
Oct 16, 2025
jeffro256
commented
Oct 16, 2025
j-berman
reviewed
Oct 16, 2025
Collaborator
j-berman
left a comment
j-berman
left a comment
There was a problem hiding this comment.
This is clean. I also appreciate the new verification utils. I have only nits on first pass (looks like you're also still working on tests), the design looks good to me. I also wrote some sanity tests for my own comfort.
Contributor
Author
|
Here was my results of the benchmark on against Here's my results against this PR: This puts this PR as having a 33% faster block propagation time. |
Contributor
Author
|
I expect the result to be higher than 33% for FCMP++ transactions since their input verification A) takes much more CPU time, and B) require many fewer random DB reads (just 1 for FCMP root). So the ratio of CPU time in doing input verification to total block propagation time should be higher. This benchmark I made also "only" gets up to 16K transactions in the mempool, whereas stressnet is at >50K, with a lot of transactions having more than 1 input. |
j-berman
reviewed
Oct 17, 2025
jeffro256
force-pushed
the
ivc_in_mempool
branch
from
1c8c578 to
58a4d56
Compare
jeffro256
added a commit
to jeffro256/monero
that referenced
this pull request
Oct 17, 2025
Co-authored-by: j-berman <[email protected]>
Contributor
Author
|
Added debug statements in |
j-berman
approved these changes
Oct 20, 2025
Collaborator
j-berman
left a comment
j-berman
left a comment
There was a problem hiding this comment.
Left a minor comment on the ver_rct_non_semantics_simple_cached.cpp unit test, but the PR generally looks good to me
jeffro256
added a commit
to jeffro256/monero
that referenced
this pull request
Oct 20, 2025
Co-authored-by: j-berman <[email protected]>
jeffro256
force-pushed
the
ivc_in_mempool
branch
from
58a4d56 to
b55a6c5
Compare
Contributor
Author
|
Resolved @j-berman comments and added new unit tests for |
j-berman
approved these changes
Oct 20, 2025
SChernykh
reviewed
Oct 21, 2025
| else | ||
| { | ||
| MDEBUG("Previously valid verID for tx " << txid << " does not match current. Perhaps there was a reorg? " | ||
| "Continuing to input verification even though this is not likely to succeed..."); |
Contributor
There was a problem hiding this comment.
This can also happen because you are reusing padding bytes, so valid_input_verification_id_inout can have some garbage in it for the old DB entries.
Contributor
Author
There was a problem hiding this comment.
Actually the bytes should be 0 since the padding is an explicit field of the struct and txpool_tx_meta_t is value-initialized when added to the DB:
monero/src/cryptonote_core/tx_pool.cpp
Line 226 in d32b5bf
Contributor
There was a problem hiding this comment.
Indeed, and that line of code has been there for a long time. All good then.
SChernykh
reviewed
Oct 21, 2025
| }; | ||
|
|
||
| static_assert(sizeof(txpool_tx_meta_t) == 192, "possible DB migration needed for changes to txpool_tx_meta_t"); | ||
| static_assert(offsetof(txpool_tx_meta_t, valid_input_verification_id) % 32 == 0, "verif ID wrong alignment"); |
Contributor
There was a problem hiding this comment.
Why % 32 when the comment above says til 160 bytes? Maybe just check that the offset is exactly 160 bytes?
jeffro256
added a commit
to jeffro256/monero
that referenced
this pull request
Oct 24, 2025
Co-authored-by: j-berman <[email protected]>
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 <[email protected]>
jeffro256
force-pushed
the
ivc_in_mempool
branch
from
b55a6c5 to
b4ccefd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
This replaces
ver_rct_non_semantics_simple_cached()with an API that offloadsthe responsibility of tracking input verification successes to the caller. The
main caller of this function in the codebase,
cryptonote::Blockchain()insteadkeeps 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:
txpool_tx_meta_talready had padding allocatedcryptonote::BlockchainFurthermore, 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(), thesingle thread can skip the CPU-intensive cryptographic code if the verification
ID allows it.
Also changes the default log category in
tx_verification_utils.cppfrom "blockchain" to "verify".To run the benchmarks for large mempool block propagation, simply change the
ENABLEDvariable inbench_p2p_heavy_block_propagation_speed()toTrue, then run thep2pfunctional test. You can thencompare performance against the
masterbranch.Depends on #10156, #10172