Page MenuHomePhabricator

Fix sporadic policyestimator_tests failure due to unspecified operand evaluation order
ClosedPublic

Authored by freetrader on Oct 20 2017, 14:30.

Details

Summary

This code was a little buggy, which caused some sporadic failures of this unit test.

The first bug was in evaluation of this expression

mpool.estimateSmartFee(i, &answerFound).GetFeePerK() > origFeeEst[answerFound - 1] - deltaFee

The result incorrectly depends on prior evaluation of answerFound in LHS which is not guaranteed to happen before evaluation of RHS. (http://en.cppreference.com/w/cpp/language/eval_order)

The second bug was that due to the first, the use of answerFound could occur before it has even been initialized (on entry to the loop).

The fix here ensures that answerFound is properly initialized before further use (provided estimateSmartFee returns it with value > 0) , and the result of the comparison becomes determinate.

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
fix_unspecified_eval_order_bug_in_policyestimator_tests
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 1018
Build 1018: arc lint + arc unit

Event Timeline

freetrader created this revision.Oct 20 2017, 14:30
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptOct 20 2017, 14:30
deadalnix edited edge metadata.Oct 20 2017, 22:27

Why is the test flacky ? Why does this fixes it ?

freetrader added a comment.EditedOct 24 2017, 11:19

Why is the test flacky ? Why does this fixes it ?

The first bug is in evaluation of this expression

mpool.estimateSmartFee(i, &answerFound).GetFeePerK() >origFeeEst[answerFound - 1] - deltaFee

The result incorrectly depends on prior evaluation of answerFound in LHS which is not guaranteed to happen before evaluation of RHS. (*)

The second bug is that due to the first, the use of answerFound may occur before it has even been initialized (on entry to the loop).

The fix here ensures that answerFound is properly initialized before further use (provided estimateSmartFee returns it with value > 0) , and the result of the comparison becomes determinate.

Order of evaluation of the operands of almost all C++ operators (including the order of evaluation of function arguments in a function-call expression and the order of evaluation of the subexpressions within any expression) is unspecified. The compiler can evaluate operands in any order, and may choose another order when the same expression is evaluated again.

(*) http://en.cppreference.com/w/cpp/language/eval_order

deadalnix accepted this revision.Oct 26 2017, 17:02
This revision is now accepted and ready to land.Oct 26 2017, 17:02

You should put the explaination in the summery of the diff.

freetrader edited the summary of this revision. (Show Details)Oct 26 2017, 18:09
This revision was automatically updated to reflect the committed changes.