Skip to content
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.

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

Avoid dividing by zero in EstimateMedianVal (policy)/ConnectTip (validation)/CreateTransaction (wallet) #14239

Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -374,14 +374,18 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal,
failBucket.leftMempool = failNum;
}

LogPrint(BCLog::ESTIMATEFEE, "FeeEst: %d %s%.0f%% decay %.5f: feerate: %g from (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n",
double pass_bucket_sum = passBucket.totalConfirmed + passBucket.inMempool + passBucket.leftMempool;
double fail_bucket_sum = failBucket.totalConfirmed + failBucket.inMempool + failBucket.leftMempool;
if (pass_bucket_sum > 0 && fail_bucket_sum > 0) {
LogPrint(BCLog::ESTIMATEFEE, "FeeEst: %d %s%.0f%% decay %.5f: feerate: %g from (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n",
confTarget, requireGreater ? ">" : "<", 100.0 * successBreakPoint, decay,
median, passBucket.start, passBucket.end,
100 * passBucket.withinTarget / (passBucket.totalConfirmed + passBucket.inMempool + passBucket.leftMempool),
100 * passBucket.withinTarget / pass_bucket_sum,
passBucket.withinTarget, passBucket.totalConfirmed, passBucket.inMempool, passBucket.leftMempool,
failBucket.start, failBucket.end,
100 * failBucket.withinTarget / (failBucket.totalConfirmed + failBucket.inMempool + failBucket.leftMempool),
100 * failBucket.withinTarget / fail_bucket_sum,
failBucket.withinTarget, failBucket.totalConfirmed, failBucket.inMempool, failBucket.leftMempool);
}


if (result) {
@@ -1825,6 +1825,8 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
uint256 hashPrevBlock = pindex->pprev == nullptr ? uint256() : pindex->pprev->GetBlockHash();
assert(hashPrevBlock == view.GetBestBlock());

nBlocksTotal++;

This comment has been minimized.

Copy link
@laanwj

laanwj Feb 10, 2019

Member

uuuuhm, what is this change? I don't think it's supposed to touch validation at all!
I think you're doing an accidental reversion here.

This comment has been minimized.

Copy link
@practicalswift

practicalswift Feb 10, 2019

Author Member

See #15283 for a description of the problem:

During the loading of the genesis block, the bench print lines in ConnectTip divide by zero due to early return in ConnectBlock.

Do you mean it has been fixed elsewhere since being discussed in #15283?

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Feb 10, 2019

Contributor

nBlocksTotal appears to be only referenced otherwise from bench output. And I assume what this is trying to do is avoid the genesis block early terminate from leaving the bench output with a zero. I believe this is a reasonable change, though it will make bench results slightly incomparable (with small numbers of blocks processed) across versions.

This change would have been better done-- easier to review for correctness, more stable against "regression"-- by simply changing the existing initialization constant for nBlocksTotal from 0 to 1.


// Special case for the genesis block, skipping connection of its transactions
// (its coinbase is unspendable)
if (block.GetHash() == chainparams.GetConsensus().hashGenesisBlock) {
@@ -1833,8 +1835,6 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
return true;
}

nBlocksTotal++;

bool fScriptChecks = true;
if (!hashAssumeValid.IsNull()) {
// We've been configured with the hash of a block which has been externally verified to have a valid history.
@@ -3018,14 +3018,18 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
}
}

WalletLogPrintf("Fee Calculation: Fee:%d Bytes:%u Needed:%d Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n",
double pass_bucket_sum = feeCalc.est.pass.totalConfirmed + feeCalc.est.pass.inMempool + feeCalc.est.pass.leftMempool;
double fail_bucket_sum = feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool;
if (pass_bucket_sum > 0 && fail_bucket_sum > 0) {
WalletLogPrintf("Fee Calculation: Fee:%d Bytes:%u Needed:%d Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n",
nFeeRet, nBytes, nFeeNeeded, feeCalc.returnedTarget, feeCalc.desiredTarget, StringForFeeReason(feeCalc.reason), feeCalc.est.decay,
feeCalc.est.pass.start, feeCalc.est.pass.end,
100 * feeCalc.est.pass.withinTarget / (feeCalc.est.pass.totalConfirmed + feeCalc.est.pass.inMempool + feeCalc.est.pass.leftMempool),
100 * feeCalc.est.pass.withinTarget / pass_bucket_sum,
feeCalc.est.pass.withinTarget, feeCalc.est.pass.totalConfirmed, feeCalc.est.pass.inMempool, feeCalc.est.pass.leftMempool,
feeCalc.est.fail.start, feeCalc.est.fail.end,
100 * feeCalc.est.fail.withinTarget / (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool),
100 * feeCalc.est.fail.withinTarget / fail_bucket_sum,
feeCalc.est.fail.withinTarget, feeCalc.est.fail.totalConfirmed, feeCalc.est.fail.inMempool, feeCalc.est.fail.leftMempool);
}
return true;
}

@@ -1,9 +1,6 @@
alignment:move.h
alignment:prevector.h
bool:wallet/wallet.cpp
float-divide-by-zero:policy/fees.cpp
float-divide-by-zero:validation.cpp
float-divide-by-zero:wallet/wallet.cpp
unsigned-integer-overflow:arith_uint256.h
unsigned-integer-overflow:basic_string.h
unsigned-integer-overflow:bench/bench.h
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.