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

[akildemir]

Fixes #10120 .

This approach uses the already existing bs return type from find_blockchain_supplement function call instead of fetching the prunable hashes separately. The second of the bs type std::vector<std::pair<crypto::hash, cryptonote::blobdata>> It appears to be used for tx hash and pruned tx blob if pruned is true. However that tx hash is never used, so this approach uses that place for prunable hash and passes it to the return type accordingly.

This behavior might be documented or another approach might have been fetching prunable hashes separately after the find_blockchain_supplement call. This approach is chosen not to make that second call to the db and for its simplicity.

Comments are welcome!

[jeffro256]

This adds >32B per transaction for wallet2 which doesn't currently have code implemented to check the TXID using the prunable hash. Before this gets merged, I'd like to see the check on the wallet side implemented, perhaps in a dependent PR. For backwards compatibility, it could skip the check if --allow-mismatched-daemon-version is specified. I could do it but am sort of busy at the moment.

Also, the RPC minor version should to be bumped since a feature was added.

[kayabaNerve]

The current get_blocks.bin pruned response always includes the prunable_hash field, even if not set @jeffro256. Setting it to the correct value, instead of the null hash, would not increase bandwidth and I'd argue it a bug fix, not a feature (though it may still justify a minor version bump).

[jeffro256]

Sorry, I was under the impression that null hashes don't get serialized in block_complete_entry, but that's KV_SERIALIZE_VAL_POD_AS_BLOB_OPT, not KV_SERIALIZE_VAL_POD_AS_BLOB. So yeah this takes up the same bandwidth in that case.

[jeffro256]

Thank you for taking this on! Once the below issues are addressed, I will be willing to approve.

[jeffro256]

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.

[jeffro256]

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.

[akildemir]

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.

[jeffro256]

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.

[akildemir]

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.

[jeffro256]

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.

[akildemir]

Ok I can rebase once that pr is merged 👍