Skip to content

add the right prunable hash per tx to pruned get_blocks.bin call #10137

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
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add the right prunable hash per tx to pruned get_blocks.bin call #10137

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
29 changes: 25 additions & 4 deletions src/blockchain_db/lmdb/db_lmdb.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3251,8 +3251,10 @@ bool BlockchainLMDB::get_blocks_from(uint64_t start_height, size_t min_block_cou
RCURSOR(blocks);
RCURSOR(tx_indices);
RCURSOR(txs_pruned);
if (!pruned)
if (pruned)
{
RCURSOR(txs_prunable_hash);
} else {
RCURSOR(txs_prunable);
}

Expand Down Expand Up @@ -3324,14 +3326,33 @@ bool BlockchainLMDB::get_blocks_from(uint64_t start_height, size_t min_block_cou
throw0(DB_ERROR(lmdb_error("Error attempting to retrieve transaction data from the db: ", result).c_str()));
tx_blob.assign((const char*)v.mv_data, v.mv_size);

if (!pruned)
{
if (pruned) {
// get the prunable hash

// Look up each transaction's tx_id
Copy link
Contributor

@jeffro256 jeffro256 Oct 5, 2025

Choose a reason for hiding this comment

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

We can skip the TXID -> tx index lookup by using already existing tx_id (terrible name 😢) variable. Tx on-chain indices are assigned in ascending order by 1) block, then 2) txs in block in specified order.

Copy link
Author

@akildemir akildemir Oct 6, 2025

Choose a reason for hiding this comment

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

So I'm trying to understand this and your next #10137 (comment) . if you mean it would be enough just to do result = mdb_cursor_get(m_cur_txs_prunable_hash, &val_tx_id, &v, op); instead of first fetching the tx index, that doesn't work for txs_prunable_hash table like it would for txs_pruned or txs_prunabletables. it gives the same prunable hash over over for all txs.

Copy link
Contributor

@jeffro256 jeffro256 Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I mean that you should do a MDB_SET op first to first set the cursor to the entry at tx_id for the first transaction in the list, then do MDB_NEXT afterwards.

Copy link
Author

@akildemir akildemir Oct 13, 2025

Choose a reason for hiding this comment

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

Sorry for the late reply. Had a busy week. So it looks like I miss the MDB_SET that happens inside this if block

monero/src/blockchain_db/lmdb/db_lmdb.cpp

Line 3301 in d32b5bf

if (skip_coinbase)
I think the confusion arose from the fact that MDB_SET for tables txs_prunable and txs_pruned is inside the if block. I checked BlockchainLMDB::get_blocks_from is always called with skip_coinbase is true and it looks like the function won't return as intended if that were to be passed as false since those tables would also be calling MDB_NEXT before MDB_SET. Indeed, when I compiled it with false, monero-wallet-cli couldn't scan the chain. It immediately throws exception Error: refresh failed: internal error: Exception in thread pool. Blocks received: 0.

So I believe there is a bug there. If skip_coinbase is necessary for that function to work properly it shouldn't be a parameter? I think the original intention was to actually skip the coinbase tx in each block by retrieving the tx and ignoring the value of the call so that cursor moves to the next tx when calling with MDB_NEXT and that works as intended if we wanted to skip the coinbase txs but otherwise it misses the MDB_SET op to those tables. So a fix would be to always call MDB_SETand only ignore the value if skip_coinbase is true.

Copy link
Contributor

@jeffro256 jeffro256 Oct 14, 2025

Choose a reason for hiding this comment

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

Yes, the skip_coinbase=false thing is a bug. I opened #10166 to fix it. Perhaps you could rebase this PR on top of that? But yes, generally, you should do MDB_SET once and then MDB_NEXT afterwards for sequential keys.

Copy link
Author

@akildemir akildemir Oct 16, 2025

Choose a reason for hiding this comment

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

Ok I can rebase once that pr is merged 👍

MDB_val_set(tx_hash_val, tx_hash);
result = mdb_cursor_get(m_cur_tx_indices, (MDB_val *)&zerokval, &tx_hash_val, MDB_GET_BOTH);
if (result)
throw0(DB_ERROR(lmdb_error("Error retrieving tx index for prunable hash: ", result).c_str()));

const txindex *tip = (const txindex *)tx_hash_val.mv_data;
MDB_val_set(val_tx_id_for_hash, tip->data.tx_id);

result = mdb_cursor_get(m_cur_txs_prunable_hash, &val_tx_id_for_hash, &v, MDB_SET);
if (result)
throw0(DB_ERROR(lmdb_error("Error attempting to retrieve transaction data from the db: ", result).c_str()));

crypto::hash prunable_hash = *(const crypto::hash*)v.mv_data;
current_block.second.push_back(std::make_pair(prunable_hash, std::move(tx_blob)));
Copy link
Contributor

@jeffro256 jeffro256 Oct 5, 2025

Choose a reason for hiding this comment

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

NACK replacing the TXID with the prunable hash, this silently breaks the API of BlockchainDB. If we must update the API here, the function signature should be a breaking change so that downstream fails to compile. That, or we can write a new get_blocks_from() method override and implement the older override with the newer override so that it's non-breaking.

} else {
// get the prunable data
result = mdb_cursor_get(m_cur_txs_prunable, &val_tx_id, &v, op);
if (result)
throw0(DB_ERROR(lmdb_error("Error attempting to retrieve transaction data from the db: ", result).c_str()));
tx_blob.append(reinterpret_cast<const char*>(v.mv_data), v.mv_size);
current_block.second.push_back(std::make_pair(tx_hash, std::move(tx_blob)));
}
current_block.second.push_back(std::make_pair(tx_hash, std::move(tx_blob)));

size += current_block.second.back().second.size();
}

Expand Down
2 changes: 1 addition & 1 deletion src/rpc/core_rpc_server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -781,7 +781,7 @@ namespace cryptonote
res.blocks.back().txs.reserve(bd.second.size());
for (std::vector<std::pair<crypto::hash, cryptonote::blobdata>>::iterator i = bd.second.begin(); i != bd.second.end(); ++i)
{
res.blocks.back().txs.push_back({std::move(i->second), crypto::null_hash});
res.blocks.back().txs.push_back({std::move(i->second), req.prune ? i->first: crypto::null_hash});
i->second.clear();
i->second.shrink_to_fit();
size += res.blocks.back().txs.back().blob.size();
Expand Down
Loading