FCMP++: cleaner/safer get_outs_by_last_locked_block - #460
Conversation
6297358 to
feb33db
Compare
feb33db to
463a9db
Compare
| 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"); |
There was a problem hiding this comment.
This throw would leave the db in an invalid state, with new block added but fcmp unhandled. Maybe debug_assert?
There was a problem hiding this comment.
The db would be in an invalid state if it doesn't throw and proceeds
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
On the other hand handle_block_to_main_chain has the same validity properties, so I'll retract my complaint.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Would that be appropriate to include here? I'm not well versed in db stuff.
There was a problem hiding this comment.
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
| std::vector<std::reference_wrapper<const transaction>> tx_refs; | ||
| tx_refs.reserve(1 + tx_pairs.size()); | ||
| tx_refs.push_back(std::cref(miner_tx)); |
There was a problem hiding this comment.
Since you set up the API to do so, it would be nice to collect without this intermediate allocate + copy.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yeah, I meant skipping allocating vectors of references
There was a problem hiding this comment.
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.
3 participants
While reviewing monero-project#11088, @selsta pointed out that this
std::move(bl)here comes beforehandle_fcmp_treethat readstx_refshere. Thosetx_refsalso include a ref to thebl.miner_tx, so if theblactually gets moved before reading the tx refs, we'd have UB.Technically the
blis const, so thestd::moveis actually just a copy.But it seems sketchy enough that I think some hardening here is in order.
I decided not to expect external callers to pass tx refs around (and manage them correctly), and instead just pass in the txs directly by reference. I figure this approach is simpler to reason through and avoid mistakes.