Skip to content

Commit

Permalink
refactor: make GetTxPayload return an Optional T instead of taking in…
Browse files Browse the repository at this point in the history 
… a T& return (#5733)

## Issue being fixed or feature implemented
We should avoid return by reference; especially return by reference with
a bool flag indicating validity.

## What was done?
Instead we use a std::optional

## How Has This Been Tested?
Unit tests pass

## Breaking Changes
Should be none

## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_

---------

Co-authored-by: Konstantin Akimov <[email protected]>
Co-authored-by: UdjinM6 <[email protected]>
  • Loading branch information
@PastaPastaPasta @knst @UdjinM6
3 people authored Jan 12, 2024
1 parent a7eeda5 commit ca77d06
Show file tree
Hide file tree
Showing 21 changed files with 288 additions and 326 deletions.
41 changes: 18 additions & 23 deletions src/bloom.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,12 +126,11 @@ bool CBloomFilter::CheckSpecialTransactionMatchesAndUpdate(const CTransaction &t
}
switch(tx.nType) {
case(TRANSACTION_PROVIDER_REGISTER): {
CProRegTx proTx;
if (GetTxPayload(tx, proTx)) {
if(contains(proTx.collateralOutpoint) ||
contains(proTx.keyIDOwner) ||
contains(proTx.keyIDVoting) ||
CheckScript(proTx.scriptPayout)) {
if (const auto opt_proTx = GetTxPayload<CProRegTx>(tx)) {
if(contains(opt_proTx->collateralOutpoint) ||
contains(opt_proTx->keyIDOwner) ||
contains(opt_proTx->keyIDVoting) ||
CheckScript(opt_proTx->scriptPayout)) {
if ((nFlags & BLOOM_UPDATE_MASK) == BLOOM_UPDATE_ALL)
insert(tx.GetHash());
return true;
Expand All @@ -140,47 +139,43 @@ bool CBloomFilter::CheckSpecialTransactionMatchesAndUpdate(const CTransaction &t
return false;
}
case(TRANSACTION_PROVIDER_UPDATE_SERVICE): {
CProUpServTx proTx;
if (GetTxPayload(tx, proTx)) {
if(contains(proTx.proTxHash)) {
if (const auto opt_proTx = GetTxPayload<CProUpServTx>(tx)) {
if(contains(opt_proTx->proTxHash)) {
return true;
}
if(CheckScript(proTx.scriptOperatorPayout)) {
if(CheckScript(opt_proTx->scriptOperatorPayout)) {
if ((nFlags & BLOOM_UPDATE_MASK) == BLOOM_UPDATE_ALL)
insert(proTx.proTxHash);
insert(opt_proTx->proTxHash);
return true;
}
}
return false;
}
case(TRANSACTION_PROVIDER_UPDATE_REGISTRAR): {
CProUpRegTx proTx;
if (GetTxPayload(tx, proTx)) {
if(contains(proTx.proTxHash))
if (const auto opt_proTx = GetTxPayload<CProUpRegTx>(tx)) {
if(contains(opt_proTx->proTxHash))
return true;
if(contains(proTx.keyIDVoting) ||
CheckScript(proTx.scriptPayout)) {
if(contains(opt_proTx->keyIDVoting) ||
CheckScript(opt_proTx->scriptPayout)) {
if ((nFlags & BLOOM_UPDATE_MASK) == BLOOM_UPDATE_ALL)
insert(proTx.proTxHash);
insert(opt_proTx->proTxHash);
return true;
}
}
return false;
}
case(TRANSACTION_PROVIDER_UPDATE_REVOKE): {
CProUpRevTx proTx;
if (GetTxPayload(tx, proTx)) {
if(contains(proTx.proTxHash))
if (const auto opt_proTx = GetTxPayload<CProUpRevTx>(tx)) {
if(contains(opt_proTx->proTxHash))
return true;
}
return false;
}
case(TRANSACTION_ASSET_LOCK): {
// inputs of Asset Lock transactions are standard. But some outputs are special
CAssetLockPayload assetLockTx;
if (GetTxPayload(tx, assetLockTx)) {
if (const auto opt_assetlockTx = GetTxPayload<CAssetLockPayload>(tx)) {
bool fFound = false;
const auto& extraOuts = assetLockTx.getCreditOutputs();
const auto& extraOuts = opt_assetlockTx->getCreditOutputs();
for (unsigned int i = 0; i < extraOuts.size(); ++i)
{
fFound = ProcessTxOut(extraOuts[i], tx.GetHash(), i) || fFound;
Expand Down
36 changes: 18 additions & 18 deletions src/core_write.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -266,40 +266,40 @@ void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry,
}

if (tx.nType == TRANSACTION_PROVIDER_REGISTER) {
if (CProRegTx proTx; GetTxPayload(tx, proTx)) {
entry.pushKV("proRegTx", proTx.ToJson());
if (const auto opt_proTx = GetTxPayload<CProRegTx>(tx)) {
entry.pushKV("proRegTx", opt_proTx->ToJson());
}
} else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_SERVICE) {
if (CProUpServTx proTx; GetTxPayload(tx, proTx)) {
entry.pushKV("proUpServTx", proTx.ToJson());
if (const auto opt_proTx = GetTxPayload<CProUpServTx>(tx)) {
entry.pushKV("proUpServTx", opt_proTx->ToJson());
}
} else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REGISTRAR) {
if (CProUpRegTx proTx; GetTxPayload(tx, proTx)) {
entry.pushKV("proUpRegTx", proTx.ToJson());
if (const auto opt_proTx = GetTxPayload<CProUpRegTx>(tx)) {
entry.pushKV("proUpRegTx", opt_proTx->ToJson());
}
} else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REVOKE) {
if (CProUpRevTx proTx; GetTxPayload(tx, proTx)) {
entry.pushKV("proUpRevTx", proTx.ToJson());
if (const auto opt_proTx = GetTxPayload<CProUpRevTx>(tx)) {
entry.pushKV("proUpRevTx", opt_proTx->ToJson());
}
} else if (tx.nType == TRANSACTION_COINBASE) {
if (CCbTx cbTx; GetTxPayload(tx, cbTx)) {
entry.pushKV("cbTx", cbTx.ToJson());
if (const auto opt_cbTx = GetTxPayload<CCbTx>(tx)) {
entry.pushKV("cbTx", opt_cbTx->ToJson());
}
} else if (tx.nType == TRANSACTION_QUORUM_COMMITMENT) {
if (llmq::CFinalCommitmentTxPayload qcTx; GetTxPayload(tx, qcTx)) {
entry.pushKV("qcTx", qcTx.ToJson());
if (const auto opt_qcTx = GetTxPayload<llmq::CFinalCommitmentTxPayload>(tx)) {
entry.pushKV("qcTx", opt_qcTx->ToJson());
}
} else if (tx.nType == TRANSACTION_MNHF_SIGNAL) {
if (MNHFTxPayload mnhfTx; GetTxPayload(tx, mnhfTx)) {
entry.pushKV("mnhfTx", mnhfTx.ToJson());
if (const auto opt_mnhfTx = GetTxPayload<MNHFTxPayload>(tx)) {
entry.pushKV("mnhfTx", opt_mnhfTx->ToJson());
}
} else if (tx.nType == TRANSACTION_ASSET_LOCK) {
if (CAssetLockPayload assetLockTx; GetTxPayload(tx, assetLockTx)) {
entry.pushKV("assetLockTx", assetLockTx.ToJson());
if (const auto opt_assetLockTx = GetTxPayload<CAssetLockPayload>(tx)) {
entry.pushKV("assetLockTx", opt_assetLockTx->ToJson());
}
} else if (tx.nType == TRANSACTION_ASSET_UNLOCK) {
if (CAssetUnlockPayload assetUnlockTx; GetTxPayload(tx, assetUnlockTx)) {
entry.pushKV("assetUnlockTx", assetUnlockTx.ToJson());
if (const auto opt_assetUnlockTx = GetTxPayload<CAssetUnlockPayload>(tx)) {
entry.pushKV("assetUnlockTx", opt_assetUnlockTx->ToJson());
}
}

Expand Down
21 changes: 11 additions & 10 deletions src/evo/assetlocktx.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,21 +60,21 @@ bool CheckAssetLockTx(const CTransaction& tx, TxValidationState& state)

if (returnAmount == 0) return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-assetlocktx-no-return");

CAssetLockPayload assetLockTx;
if (!GetTxPayload(tx, assetLockTx)) {
const auto opt_assetLockTx = GetTxPayload<CAssetLockPayload>(tx);
if (!opt_assetLockTx.has_value()) {
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-assetlocktx-payload");
}

if (assetLockTx.getVersion() == 0 || assetLockTx.getVersion() > CAssetLockPayload::CURRENT_VERSION) {
if (opt_assetLockTx->getVersion() == 0 || opt_assetLockTx->getVersion() > CAssetLockPayload::CURRENT_VERSION) {
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-assetlocktx-version");
}

if (assetLockTx.getCreditOutputs().empty()) {
if (opt_assetLockTx->getCreditOutputs().empty()) {
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-assetlocktx-emptycreditoutputs");
}

CAmount creditOutputsAmount = 0;
for (const CTxOut& out : assetLockTx.getCreditOutputs()) {
for (const CTxOut& out : opt_assetLockTx->getCreditOutputs()) {
if (out.nValue == 0 || !MoneyRange(out.nValue) || !MoneyRange(creditOutputsAmount + out.nValue)) {
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-assetlocktx-credit-outofrange");
}
Expand Down Expand Up @@ -158,10 +158,11 @@ bool CheckAssetUnlockTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-assetunlocktx-too-many-outs");
}

CAssetUnlockPayload assetUnlockTx;
if (!GetTxPayload(tx, assetUnlockTx)) {
const auto opt_assetUnlockTx = GetTxPayload<CAssetUnlockPayload>(tx);
if (!opt_assetUnlockTx) {
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-assetunlocktx-payload");
}
auto& assetUnlockTx = *opt_assetUnlockTx;

if (assetUnlockTx.getVersion() == 0 || assetUnlockTx.getVersion() > CAssetUnlockPayload::CURRENT_VERSION) {
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-assetunlocktx-version");
Expand All @@ -187,11 +188,11 @@ bool CheckAssetUnlockTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*

bool GetAssetUnlockFee(const CTransaction& tx, CAmount& txfee, TxValidationState& state)
{
CAssetUnlockPayload assetUnlockTx;
if (!GetTxPayload(tx, assetUnlockTx)) {
const auto opt_assetUnlockTx = GetTxPayload<CAssetUnlockPayload>(tx);
if (!opt_assetUnlockTx) {
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-assetunlocktx-payload");
}
const CAmount txfee_aux = assetUnlockTx.getFee();
const CAmount txfee_aux = opt_assetUnlockTx->getFee();
if (txfee_aux == 0 || !MoneyRange(txfee_aux)) {
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-assetunlock-fee-outofrange");
}
Expand Down
53 changes: 25 additions & 28 deletions src/evo/cbtx.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,11 @@ bool CheckCbTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValidati
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-cbtx-invalid");
}

CCbTx cbTx;
if (!GetTxPayload(tx, cbTx)) {
const auto opt_cbTx = GetTxPayload<CCbTx>(tx);
if (!opt_cbTx) {
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-cbtx-payload");
}
const auto& cbTx = *opt_cbTx;

if (cbTx.nVersion == CCbTx::Version::INVALID || cbTx.nVersion >= CCbTx::Version::UNKNOWN) {
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-cbtx-version");
Expand Down Expand Up @@ -68,10 +69,11 @@ bool CheckCbTxMerkleRoots(const CBlock& block, const CBlockIndex* pindex, const

int64_t nTime1 = GetTimeMicros();

CCbTx cbTx;
if (!GetTxPayload(*block.vtx[0], cbTx)) {
const auto opt_cbTx = GetTxPayload<CCbTx>(*block.vtx[0]);
if (!opt_cbTx) {
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-payload");
}
auto cbTx = *opt_cbTx;

int64_t nTime2 = GetTimeMicros(); nTimePayload += nTime2 - nTime1;
LogPrint(BCLog::BENCHMARK, " - GetTxPayload: %.2fms [%.2fs]\n", 0.001 * (nTime2 - nTime1), nTimePayload * 0.000001);
Expand Down Expand Up @@ -255,23 +257,23 @@ bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPre
const auto& tx = block.vtx[i];

if (tx->nVersion == 3 && tx->nType == TRANSACTION_QUORUM_COMMITMENT) {
llmq::CFinalCommitmentTxPayload qc;
if (!GetTxPayload(*tx, qc)) {
const auto opt_qc = GetTxPayload<llmq::CFinalCommitmentTxPayload>(*tx);
if (!opt_qc) {
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-qc-payload-calc-cbtx-quorummerkleroot");
}
if (qc.commitment.IsNull()) {
if (opt_qc->commitment.IsNull()) {
// having null commitments is ok but we don't use them here, move to the next tx
continue;
}
const auto& llmq_params_opt = llmq::GetLLMQParams(qc.commitment.llmqType);
const auto& llmq_params_opt = llmq::GetLLMQParams(opt_qc->commitment.llmqType);
if (!llmq_params_opt.has_value()) {
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-qc-commitment-type-calc-cbtx-quorummerkleroot");
}
const auto& llmq_params = llmq_params_opt.value();
auto qcHash = ::SerializeHash(qc.commitment);
auto qcHash = ::SerializeHash(opt_qc->commitment);
if (llmq::utils::IsQuorumRotationEnabled(llmq_params, pindexPrev)) {
auto& map_indexed_hashes = qcIndexedHashes[qc.commitment.llmqType];
map_indexed_hashes[qc.commitment.quorumIndex] = qcHash;
auto& map_indexed_hashes = qcIndexedHashes[opt_qc->commitment.llmqType];
map_indexed_hashes[opt_qc->commitment.quorumIndex] = qcHash;
} else {
auto& vec_hashes = qcHashes[llmq_params.type];
if (vec_hashes.size() == size_t(llmq_params.signingActiveQuorumCount)) {
Expand Down Expand Up @@ -328,17 +330,17 @@ bool CheckCbTxBestChainlock(const CBlock& block, const CBlockIndex* pindex, cons
return true;
}

CCbTx cbTx;
if (!GetTxPayload(*block.vtx[0], cbTx)) {
const auto opt_cbTx = GetTxPayload<CCbTx>(*block.vtx[0]);
if (!opt_cbTx) {
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-payload");
}

if (cbTx.nVersion < CCbTx::Version::CLSIG_AND_BALANCE) {
if (opt_cbTx->nVersion < CCbTx::Version::CLSIG_AND_BALANCE) {
return true;
}

auto best_clsig = chainlock_handler.GetBestChainLock();
if (best_clsig.getHeight() == pindex->nHeight - 1 && cbTx.bestCLHeightDiff == 0 && cbTx.bestCLSignature == best_clsig.getSig()) {
if (best_clsig.getHeight() == pindex->nHeight - 1 && opt_cbTx->bestCLHeightDiff == 0 && opt_cbTx->bestCLSignature == best_clsig.getSig()) {
// matches our best clsig which still hold values for the previous block
return true;
}
Expand All @@ -347,29 +349,29 @@ bool CheckCbTxBestChainlock(const CBlock& block, const CBlockIndex* pindex, cons
// If std::optional prevBlockCoinbaseChainlock is empty, then up to the previous block, coinbase Chainlock is null.
if (prevBlockCoinbaseChainlock.has_value()) {
// Previous block Coinbase has a non-null Chainlock: current block's Chainlock must be non-null and at least as new as the previous one
if (!cbTx.bestCLSignature.IsValid()) {
if (!opt_cbTx->bestCLSignature.IsValid()) {
// IsNull() doesn't exist for CBLSSignature: we assume that a non valid BLS sig is null
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-null-clsig");
}
int prevBlockCoinbaseCLHeight = pindex->nHeight - static_cast<int>(prevBlockCoinbaseChainlock.value().second) - 1;
int curBlockCoinbaseCLHeight = pindex->nHeight - static_cast<int>(cbTx.bestCLHeightDiff) - 1;
int curBlockCoinbaseCLHeight = pindex->nHeight - static_cast<int>(opt_cbTx->bestCLHeightDiff) - 1;
if (curBlockCoinbaseCLHeight < prevBlockCoinbaseCLHeight) {
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-older-clsig");
}
}

// IsNull() doesn't exist for CBLSSignature: we assume that a valid BLS sig is non-null
if (cbTx.bestCLSignature.IsValid()) {
int curBlockCoinbaseCLHeight = pindex->nHeight - static_cast<int>(cbTx.bestCLHeightDiff) - 1;
if (best_clsig.getHeight() == curBlockCoinbaseCLHeight && best_clsig.getSig() == cbTx.bestCLSignature) {
if (opt_cbTx->bestCLSignature.IsValid()) {
int curBlockCoinbaseCLHeight = pindex->nHeight - static_cast<int>(opt_cbTx->bestCLHeightDiff) - 1;
if (best_clsig.getHeight() == curBlockCoinbaseCLHeight && best_clsig.getSig() == opt_cbTx->bestCLSignature) {
// matches our best (but outdated) clsig, no need to verify it again
return true;
}
uint256 curBlockCoinbaseCLBlockHash = pindex->GetAncestor(curBlockCoinbaseCLHeight)->GetBlockHash();
if (!chainlock_handler.VerifyChainLock(llmq::CChainLockSig(curBlockCoinbaseCLHeight, curBlockCoinbaseCLBlockHash, cbTx.bestCLSignature))) {
if (!chainlock_handler.VerifyChainLock(llmq::CChainLockSig(curBlockCoinbaseCLHeight, curBlockCoinbaseCLBlockHash, opt_cbTx->bestCLSignature))) {
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-invalid-clsig");
}
} else if (cbTx.bestCLHeightDiff != 0) {
} else if (opt_cbTx->bestCLHeightDiff != 0) {
// Null bestCLSignature is allowed only with bestCLHeightDiff = 0
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-cldiff");
}
Expand Down Expand Up @@ -450,12 +452,7 @@ std::optional<CCbTx> GetCoinbaseTx(const CBlockIndex* pindex)
}

CTransactionRef cbTx = block.vtx[0];
CCbTx cbTxPayload;
if (!GetTxPayload(*cbTx, cbTxPayload)) {
return std::nullopt;
}

return cbTxPayload;
return GetTxPayload<CCbTx>(*cbTx);
}

std::optional<std::pair<CBLSSignature, uint32_t>> GetNonNullCoinbaseChainlock(const CBlockIndex* pindex)
Expand Down
23 changes: 11 additions & 12 deletions src/evo/creditpool.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,13 @@ std::unique_ptr<CCreditPoolManager> creditPoolManager;

static bool GetDataFromUnlockTx(const CTransaction& tx, CAmount& toUnlock, uint64_t& index, TxValidationState& state)
{
CAssetUnlockPayload assetUnlockTx;
if (!GetTxPayload(tx, assetUnlockTx)) {
const auto opt_assetUnlockTx = GetTxPayload<CAssetUnlockPayload>(tx);
if (!opt_assetUnlockTx.has_value()) {
return state.Invalid(TxValidationResult::TX_CONSENSUS, "failed-creditpool-unlock-payload");
}

index = assetUnlockTx.getIndex();
toUnlock = assetUnlockTx.getFee();
index = opt_assetUnlockTx->getIndex();
toUnlock = opt_assetUnlockTx->getFee();
for (const CTxOut& txout : tx.vout) {
if (!MoneyRange(txout.nValue)) {
return state.Invalid(TxValidationResult::TX_CONSENSUS, "failed-creditpool-unlock-txout-outofrange");
Expand Down Expand Up @@ -143,14 +143,13 @@ CCreditPool CCreditPoolManager::ConstructCreditPool(const CBlockIndex* const blo
AddToCache(block_index->GetBlockHash(), block_index->nHeight, emptyPool);
return emptyPool;
}
CAmount locked{0};
{
CCbTx cbTx;
if (!GetTxPayload(block->vtx[0]->vExtraPayload, cbTx)) {
throw std::runtime_error(strprintf("%s: failed-getcreditpool-cbtx-payload", __func__));
CAmount locked = [&, func=__func__]() {
const auto opt_cbTx = GetTxPayload<CCbTx>(block->vtx[0]->vExtraPayload);
if (!opt_cbTx) {
throw std::runtime_error(strprintf("%s: failed-getcreditpool-cbtx-payload", func));
}
locked = cbTx.creditPoolBalance;
}
return opt_cbTx->creditPoolBalance;
}();

// We use here sliding window with LimitBlocksToTrace to determine
// current limits for asset unlock transactions.
Expand Down Expand Up @@ -235,7 +234,7 @@ CCreditPoolDiff::CCreditPoolDiff(CCreditPool starter, const CBlockIndex *pindexP

bool CCreditPoolDiff::Lock(const CTransaction& tx, TxValidationState& state)
{
if (CAssetLockPayload assetLockTx; !GetTxPayload(tx, assetLockTx)) {
if (const auto opt_assetLockTx = GetTxPayload<CAssetLockPayload>(tx); !opt_assetLockTx) {
return state.Invalid(TxValidationResult::TX_CONSENSUS, "failed-creditpool-lock-payload");
}

Expand Down
Loading

0 comments on commit ca77d06

Please sign in to comment.