Skip to content

FCMP++: cleaner/safer get_outs_by_last_locked_block #460

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
j-berman wants to merge 1 commit into
base: fcmp++-stage
Choose a base branch
from
Open

FCMP++: cleaner/safer get_outs_by_last_locked_block #460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/blockchain_utilities/blockchain_import.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,13 +490,15 @@ int import_from_file(cryptonote::core& core, const std::string& import_file_path

const uint64_t first_unified_id = core.get_blockchain_storage().get_db().num_outputs();
std::unordered_map<uint64_t, rct::key> transparent_amount_commitments;
const auto tx_refs = cryptonote::collect_transparent_amount_commitments(b.miner_tx, txs, transparent_amount_commitments);
cryptonote::collect_transparent_amount_commitments(b.miner_tx, txs, transparent_amount_commitments);
OutsByLastLockedBlockMeta new_locked_outs = cryptonote::get_outs_by_last_locked_block(b.miner_tx, txs, transparent_amount_commitments, first_unified_id, h);

try
{
uint64_t long_term_block_weight = core.get_blockchain_storage().get_next_long_term_block_weight(block_weight);
const uint64_t new_height = core.get_blockchain_storage().get_db().add_block(std::make_pair(b, block_to_blob(b)), block_weight, long_term_block_weight, cumulative_difficulty, coins_generated, txs, transparent_amount_commitments);
cryptonote::handle_fcmp_tree(&core.get_blockchain_storage().get_db(), new_height - 1, first_unified_id, tx_refs, transparent_amount_commitments);
CHECK_AND_ASSERT_THROW_MES(h == (new_height-1), "Unexpected height");

@UkoeHB UkoeHB Aug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This throw would leave the db in an invalid state, with new block added but fcmp unhandled. Maybe debug_assert?

@j-berman j-berman Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The db would be in an invalid state if it doesn't throw and proceeds

@UkoeHB UkoeHB Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could make the whole sequence atomic but that may not be worth the effort. If h == (new_height-1) is an invariant of add_block then debug_assert should be adequate. And BlockchainDB::add_block should explicitly call out that invariant in the header comment.

Like if the comment said "Returns the previous height + 1." then the assert wouldn't be necessary.

@UkoeHB UkoeHB Aug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand handle_block_to_main_chain has the same validity properties, so I'll retract my complaint.

@j-berman j-berman Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW handle_block_to_main_chain is guaranteed always atomic because it's always called in the context of a batch db txn (prepare_handle starts the db txn, cleanup_handle commits it)

@UkoeHB UkoeHB Aug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that be appropriate to include here? I'm not well versed in db stuff.

@j-berman j-berman Aug 22, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does it here when use_batch is true. This is the blockchain import tool which I expect is a pretty rarely used tool, so I don't think worth spending a whole bunch of time on it

cryptonote::handle_fcmp_tree(&core.get_blockchain_storage().get_db(), new_height-1, first_unified_id, transparent_amount_commitments, std::move(new_locked_outs));
}
catch (const std::exception& e)
{
Expand Down
58 changes: 44 additions & 14 deletions src/cryptonote_basic/cryptonote_format_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,29 @@ namespace cryptonote
return tx.vout.size();
}
//---------------------------------------------------------------
static OutsByLastLockedBlockMeta get_outs_by_last_locked_block(
const std::vector<std::reference_wrapper<const cryptonote::transaction>> &txs,
const std::unordered_map<uint64_t, rct::key> &transparent_amount_commitments,
const uint64_t first_unified_id,
const uint64_t block_idx)
{
OutsByLastLockedBlockMeta outs;
outs.next_unified_id = first_unified_id;

for (const auto &tx : txs)
{
outs.next_unified_id += set_tx_outs_by_last_locked_block(
tx.get(),
transparent_amount_commitments,
outs.next_unified_id,
block_idx,
outs.outs_by_last_locked_block,
outs.timelocked_outputs);
}

return outs;
}
//---------------------------------------------------------------
}

namespace cryptonote
Expand Down Expand Up @@ -1986,26 +2009,33 @@ namespace cryptonote
}
//---------------------------------------------------------------
OutsByLastLockedBlockMeta get_outs_by_last_locked_block(
const std::vector<std::reference_wrapper<const cryptonote::transaction>> &txs,
const cryptonote::transaction &miner_tx,
const std::vector<cryptonote::transaction> &txs,
const std::unordered_map<uint64_t, rct::key> &transparent_amount_commitments,
const uint64_t first_unified_id,
const uint64_t block_idx)
{
OutsByLastLockedBlockMeta outs_by_last_locked_block_meta_out;
outs_by_last_locked_block_meta_out.next_unified_id = first_unified_id;

std::vector<std::reference_wrapper<const transaction>> tx_refs;
tx_refs.reserve(1 + txs.size());
tx_refs.push_back(std::cref(miner_tx));
for (const auto &tx : txs)
{
outs_by_last_locked_block_meta_out.next_unified_id += set_tx_outs_by_last_locked_block(
tx.get(),
transparent_amount_commitments,
outs_by_last_locked_block_meta_out.next_unified_id,
block_idx,
outs_by_last_locked_block_meta_out.outs_by_last_locked_block,
outs_by_last_locked_block_meta_out.timelocked_outputs);
}
tx_refs.push_back(std::cref(tx));
return get_outs_by_last_locked_block(tx_refs, transparent_amount_commitments, first_unified_id, block_idx);
}

return outs_by_last_locked_block_meta_out;
OutsByLastLockedBlockMeta get_outs_by_last_locked_block(
const cryptonote::transaction &miner_tx,
const std::vector<std::pair<transaction, blobdata>> &tx_pairs,
const std::unordered_map<uint64_t, rct::key> &transparent_amount_commitments,
const uint64_t first_unified_id,
const uint64_t block_idx)
{
std::vector<std::reference_wrapper<const transaction>> tx_refs;
tx_refs.reserve(1 + tx_pairs.size());
tx_refs.push_back(std::cref(miner_tx));
Comment on lines +2033 to +2035

@jeffro256 jeffro256 Aug 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you set up the API to do so, it would be nice to collect without this intermediate allocate + copy.

@j-berman j-berman Aug 22, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I see what you mean, it's using reference wrappers to avoid allocating and copying the actual transactions, while allowing the distinct vectors holding transactions

@jeffro256 jeffro256 Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I meant skipping allocating vectors of references

@j-berman j-berman Aug 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it as a tradeoff of duplicating a good bit of internal logic versus allocating vectors of references. The latter seemed the desirable tradeoff to me and is an insignificant perf hit.

@UkoeHB UkoeHB Aug 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No impl Iterator here sadly.

for (const auto &tx : tx_pairs)
tx_refs.push_back(std::cref(tx.first));
return get_outs_by_last_locked_block(tx_refs, transparent_amount_commitments, first_unified_id, block_idx);
}
//---------------------------------------------------------------
fcmp_pp::OutputPair to_output_pair(const cryptonote::txout_target_v &tx_out, const rct::key &commitment)
Expand Down
10 changes: 9 additions & 1 deletion src/cryptonote_basic/cryptonote_format_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,15 @@ namespace cryptonote
};

OutsByLastLockedBlockMeta get_outs_by_last_locked_block(
const std::vector<std::reference_wrapper<const cryptonote::transaction>> &txs,
const cryptonote::transaction &miner_tx,
const std::vector<cryptonote::transaction> &txs,
const std::unordered_map<uint64_t, rct::key> &transparent_amount_commitments,
const uint64_t first_unified_id,
const uint64_t block_idx);

OutsByLastLockedBlockMeta get_outs_by_last_locked_block(
const cryptonote::transaction &miner_tx,
const std::vector<std::pair<transaction, blobdata>> &tx_pairs,
const std::unordered_map<uint64_t, rct::key> &transparent_amount_commitments,
const uint64_t first_unified_id,
const uint64_t block_idx);
Expand Down
15 changes: 7 additions & 8 deletions src/cryptonote_core/blockchain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2846,11 +2846,8 @@ static bool batch_verify_fcmp_pp_txs(const BlockchainDB *db,
return true;
}
//------------------------------------------------------------------
void cryptonote::handle_fcmp_tree(BlockchainDB *db, const uint64_t block_idx, const uint64_t first_unified_id, const std::vector<std::reference_wrapper<const transaction>> &tx_refs, const std::unordered_map<uint64_t, rct::key> &transparent_amount_commitments)
void cryptonote::handle_fcmp_tree(BlockchainDB *db, const uint64_t block_idx, const uint64_t first_unified_id, const std::unordered_map<uint64_t, rct::key> &transparent_amount_commitments, OutsByLastLockedBlockMeta &&new_locked_outs)
{
// Collect outs by last locked block to add to the db
OutsByLastLockedBlockMeta new_locked_outs = cryptonote::get_outs_by_last_locked_block(tx_refs, transparent_amount_commitments, first_unified_id, block_idx);

// Get the outputs with default last locked block
const uint64_t default_last_locked_block = cryptonote::get_default_last_locked_block_index(block_idx);
auto new_default_locked_outs_it = new_locked_outs.outs_by_last_locked_block.find(default_last_locked_block);
Expand Down Expand Up @@ -4856,7 +4853,7 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&
TIME_MEASURE_START(tac);

// Collect all remaining transparent amount commitments
const auto tx_refs = collect_transparent_amount_commitments(bl.miner_tx, txs, transparent_amount_commitments);
collect_transparent_amount_commitments(bl.miner_tx, txs, transparent_amount_commitments);

TIME_MEASURE_FINISH(tac);

Expand Down Expand Up @@ -4890,7 +4887,9 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&
if(precomputed)
block_processing_time += m_fake_pow_calc_time;

// Collect new locked outputs we're adding to the db
const uint64_t first_unified_id = m_db->num_outputs();
OutsByLastLockedBlockMeta new_locked_outs = cryptonote::get_outs_by_last_locked_block(bl.miner_tx, txs, transparent_amount_commitments, first_unified_id, blockchain_height);

rtxn_guard.stop();
TIME_MEASURE_START(addblock);
Expand Down Expand Up @@ -4926,9 +4925,9 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&
LOG_ERROR("Blocks that failed verification should not reach here");
}

if (new_height == 0)
if (new_height == 0 || (new_height-1) != blockchain_height)
{
LOG_ERROR("handle_block_to_main_chain: unexpected new_height == 0");
LOG_ERROR("handle_block_to_main_chain: unexpected new_height: " << new_height << " , expected: " << blockchain_height+1);
bvc.m_verifivation_failed = true;
return false;
}
Expand All @@ -4945,7 +4944,7 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&

TIME_MEASURE_START(advance_tree);

try { handle_fcmp_tree(m_db, new_height-1, first_unified_id, tx_refs, transparent_amount_commitments); }
try { handle_fcmp_tree(m_db, new_height-1, first_unified_id, transparent_amount_commitments, std::move(new_locked_outs)); }
catch (const std::exception& e)
{
LOG_ERROR("Failed to advance tree at block with hash: " << id << ", what = " << e.what());
Expand Down
4 changes: 2 additions & 2 deletions src/cryptonote_core/blockchain.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,8 @@ namespace cryptonote
void handle_fcmp_tree(BlockchainDB *db,
uint64_t block_idx,
uint64_t first_unified_id,
const std::vector<std::reference_wrapper<const transaction>> &tx_refs,
const std::unordered_map<uint64_t, rct::key> &transparent_amount_commitments);
const std::unordered_map<uint64_t, rct::key> &transparent_amount_commitments,
OutsByLastLockedBlockMeta &&new_locked_outs);

/** Declares ways in which the BlockchainDB backend should be told to sync
*
Expand Down
56 changes: 27 additions & 29 deletions src/cryptonote_core/tx_verification_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,28 @@ static bool collect_fcmp_pp_tx_verify_input(cryptonote::transaction &tx,
return true;
}

static void collect_transparent_amount_commitments_static(
const std::vector<std::reference_wrapper<const transaction>> &tx_refs,
std::unordered_map<uint64_t, rct::key> &transparent_amount_commitments_inout)
{
// Note: we do not clear transparent_amount_commitments_inout because it may be a rolling cache

for (const auto &tx_ref : tx_refs)
{
const auto &tx = tx_ref.get();

// We only need commitments for transparent amounts, which are tx version 1 || coinbase txs
if (tx.version > 1 && !cryptonote::is_coinbase(tx))
continue;
for (const auto &tx_out : tx.vout)
{
const uint64_t amount = tx_out.amount;
if (transparent_amount_commitments_inout.find(amount) == transparent_amount_commitments_inout.end())
transparent_amount_commitments_inout[amount] = rct::zeroCommitVartime(amount);
}
}
}

////////////////////////////////////////////////////////////////////////////////////////////////////

namespace cryptonote
Expand Down Expand Up @@ -504,28 +526,6 @@ bool collect_points_for_torsion_check(const transaction& tx,
}

void collect_transparent_amount_commitments(
const std::vector<std::reference_wrapper<const transaction>> &txs,
std::unordered_map<uint64_t, rct::key> &transparent_amount_commitments_inout)
{
// Note: we do not clear transparent_amount_commitments_inout because it may be a rolling cache

for (const auto &tx_ref : txs)
{
const auto &tx = tx_ref.get();

// We only need commitments for transparent amounts, which are tx version 1 || coinbase txs
if (tx.version > 1 && !cryptonote::is_coinbase(tx))
continue;
for (const auto &tx_out : tx.vout)
{
const uint64_t amount = tx_out.amount;
if (transparent_amount_commitments_inout.find(amount) == transparent_amount_commitments_inout.end())
transparent_amount_commitments_inout[amount] = rct::zeroCommitVartime(amount);
}
}
}

std::vector<std::reference_wrapper<const transaction>> collect_transparent_amount_commitments(
const transaction &miner_tx,
const std::vector<std::pair<transaction, blobdata>> &tx_pairs,
std::unordered_map<uint64_t, rct::key> &transparent_amount_commitments_inout)
Expand All @@ -535,11 +535,10 @@ std::vector<std::reference_wrapper<const transaction>> collect_transparent_amoun
tx_refs.push_back(std::cref(miner_tx));
for (const auto &tx : tx_pairs)
tx_refs.push_back(std::cref(tx.first));
collect_transparent_amount_commitments(tx_refs, transparent_amount_commitments_inout);
return tx_refs;
collect_transparent_amount_commitments_static(tx_refs, transparent_amount_commitments_inout);
}

std::vector<std::reference_wrapper<const transaction>> collect_transparent_amount_commitments(
void collect_transparent_amount_commitments(
const transaction &miner_tx,
const std::vector<transaction> &txs,
std::unordered_map<uint64_t, rct::key> &transparent_amount_commitments_inout)
Expand All @@ -549,8 +548,7 @@ std::vector<std::reference_wrapper<const transaction>> collect_transparent_amoun
tx_refs.push_back(std::cref(miner_tx));
for (const auto &tx : txs)
tx_refs.push_back(std::cref(tx));
collect_transparent_amount_commitments(tx_refs, transparent_amount_commitments_inout);
return tx_refs;
collect_transparent_amount_commitments_static(tx_refs, transparent_amount_commitments_inout);
}

void collect_transparent_amount_commitments(
Expand All @@ -561,7 +559,7 @@ void collect_transparent_amount_commitments(
tx_refs.reserve(txs_by_txid.size());
for (const auto &tx_pair : txs_by_txid)
tx_refs.push_back(std::cref(tx_pair.second.first));
collect_transparent_amount_commitments(tx_refs, transparent_amount_commitments_inout);
collect_transparent_amount_commitments_static(tx_refs, transparent_amount_commitments_inout);
}

uint64_t get_non_coinbase_tx_weight_limit(const uint8_t hf_version)
Expand Down Expand Up @@ -925,7 +923,7 @@ bool ver_non_input_consensus(const transaction& tx, tx_verification_context& tvc
{
// Get tx's transparent amount commitments
std::unordered_map<uint64_t, rct::key> transparent_amount_commitments;
collect_transparent_amount_commitments({std::cref(tx)}, transparent_amount_commitments);
collect_transparent_amount_commitments_static({std::cref(tx)}, transparent_amount_commitments);

return ver_non_input_consensus_templated(&tx, &tx + 1, transparent_amount_commitments, tvc, hf_version);
}
Expand Down
6 changes: 1 addition & 5 deletions src/cryptonote_core/tx_verification_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,11 @@ bool collect_points_for_torsion_check(const transaction& tx,
std::vector<rct::key> &pubkeys_and_commitments_inout);

void collect_transparent_amount_commitments(
const std::vector<std::reference_wrapper<const transaction>> &txs,
std::unordered_map<uint64_t, rct::key> &transparent_amount_commitments_inout);

std::vector<std::reference_wrapper<const transaction>> collect_transparent_amount_commitments(
const transaction &miner_tx,
const std::vector<std::pair<transaction, blobdata>> &tx_pairs,
std::unordered_map<uint64_t, rct::key> &transparent_amount_commitments_inout);

std::vector<std::reference_wrapper<const transaction>> collect_transparent_amount_commitments(
void collect_transparent_amount_commitments(
const transaction &miner_tx,
const std::vector<transaction> &txs,
std::unordered_map<uint64_t, rct::key> &transparent_amount_commitments_inout);
Expand Down
4 changes: 2 additions & 2 deletions src/wallet/wallet2.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3205,8 +3205,8 @@ static void prepare_tree_state_change(const TreeSyncStartParams &tree_sync_start
new_block_hashes_out.push_back(parsed_blocks[i].block.hash);

// Slow: collect transparent amount commitments
const auto tx_refs = cryptonote::collect_transparent_amount_commitments(parsed_blocks[i].block.miner_tx, parsed_blocks[i].txes, transparent_amount_commitments);
auto res = cryptonote::get_outs_by_last_locked_block(tx_refs, transparent_amount_commitments, first_unified_id, created_block_idx);
cryptonote::collect_transparent_amount_commitments(parsed_blocks[i].block.miner_tx, parsed_blocks[i].txes, transparent_amount_commitments);
auto res = cryptonote::get_outs_by_last_locked_block(parsed_blocks[i].block.miner_tx, parsed_blocks[i].txes, transparent_amount_commitments, first_unified_id, created_block_idx);

outs_by_last_locked_blocks.emplace_back(std::move(res.outs_by_last_locked_block));
first_unified_id = res.next_unified_id;
Expand Down
4 changes: 2 additions & 2 deletions tests/core_tests/fcmp_pp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,8 +323,8 @@ bool gen_fcmp_pp_tx_validation_base::generate_with(std::vector<test_event_entry>
blk_idx == pre_rct_tx_block_idx ? std::vector<transaction>{pre_rct_tx}
: blk_idx == bpp_block_idx ? std::vector<transaction>{bpp_tx}
: std::vector<transaction>{};
const auto tx_refs = cryptonote::collect_transparent_amount_commitments(blk.miner_tx, txs, transparent_amount_commitments);
auto outs_meta = cryptonote::get_outs_by_last_locked_block(tx_refs, transparent_amount_commitments, first_unified_id, blk_idx);
cryptonote::collect_transparent_amount_commitments(blk.miner_tx, txs, transparent_amount_commitments);
auto outs_meta = cryptonote::get_outs_by_last_locked_block(blk.miner_tx, txs, transparent_amount_commitments, first_unified_id, blk_idx);
outs_by_last_locked_blocks.emplace_back(std::move(outs_meta.outs_by_last_locked_block));
first_unified_id = outs_meta.next_unified_id;
}
Expand Down
Loading