Skip to content

Commit

Permalink
SCP: don't set h if not compatible with b
Browse files Browse the repository at this point in the history
  • Loading branch information
@MonsieurNicolas
MonsieurNicolas committed Aug 28, 2018
1 parent 43e1e0d commit 0640fde
Show file tree
Hide file tree
Showing 3 changed files with 138 additions and 56 deletions.
81 changes: 57 additions & 24 deletions src/scp/BallotProtocol.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -369,10 +369,11 @@ BallotProtocol::bumpState(Value const& value, uint32 n)

newb.counter = n;

if (mHighBallot)
if (mValueOverride)
{
// can only bump the counter if we committed to something already
newb.value = mHighBallot->value;
// we use the value that we saw confirmed prepared
// or that we at least voted to commit to
newb.value = *mValueOverride;
}
else
{
Expand Down Expand Up @@ -483,6 +484,12 @@ BallotProtocol::bumpToBallot(SCPBallot const& ballot, bool check)

mCurrentBallot = std::make_unique<SCPBallot>(ballot);

// invariant: h.value = b.value
if (mHighBallot && !areBallotsCompatible(*mCurrentBallot, *mHighBallot))
{
mHighBallot.reset();
}

if (gotBumped)
{
mHeardFromQuorum = false;
Expand Down Expand Up @@ -556,7 +563,6 @@ BallotProtocol::createStatement(SCPStatementType const& type)
{
auto& c = statement.pledges.confirm();
c.quorumSetHash = getLocalNode()->getQuorumSetHash();
dbgAssert(areBallotsLessAndCompatible(*mCommit, *mHighBallot));
c.ballot = *mCurrentBallot;
c.nPrepared = mPrepared->counter;
c.nCommit = mCommit->counter;
Expand All @@ -565,7 +571,6 @@ BallotProtocol::createStatement(SCPStatementType const& type)
break;
case SCPStatementType::SCP_ST_EXTERNALIZE:
{
dbgAssert(areBallotsLessAndCompatible(*mCommit, *mHighBallot));
auto& e = statement.pledges.externalize();
e.commit = *mCommit;
e.nH = mHighBallot->counter;
Expand Down Expand Up @@ -643,6 +648,11 @@ BallotProtocol::checkInvariants()
{
dbgAssert(areBallotsLessAndIncompatible(*mPreparedPrime, *mPrepared));
}
if (mHighBallot)
{
dbgAssert(mCurrentBallot);
dbgAssert(areBallotsLessAndCompatible(*mHighBallot, *mCurrentBallot));
}
if (mCommit)
{
dbgAssert(mCurrentBallot);
Expand Down Expand Up @@ -770,13 +780,16 @@ BallotProtocol::getPrepareCandidates(SCPStatement const& hint)
return candidates;
}

void
BallotProtocol::updateCurrentIfNeeded()
bool
BallotProtocol::updateCurrentIfNeeded(SCPBallot const& h)
{
if (!mCurrentBallot || compareBallots(*mCurrentBallot, *mHighBallot) < 0)
bool didWork = false;
if (!mCurrentBallot || compareBallots(*mCurrentBallot, h) < 0)
{
bumpToBallot(*mHighBallot, true);
bumpToBallot(h, true);
didWork = true;
}
return didWork;
}

bool
Expand Down Expand Up @@ -961,6 +974,11 @@ BallotProtocol::attemptPreparedConfirmed(SCPStatement const& hint)
{
break;
}
// c and h must be compatible
if (!areBallotsLessAndCompatible(*cur, newH))
{
continue;
}
bool ratified = federatedRatify(
std::bind(&BallotProtocol::hasPreparedBallot, ballot, _1));
if (ratified)
Expand Down Expand Up @@ -1023,25 +1041,37 @@ BallotProtocol::setPreparedConfirmed(SCPBallot const& newC,

bool didWork = false;

if (!mHighBallot || compareBallots(newH, *mHighBallot) > 0)
{
didWork = true;
mHighBallot = std::make_unique<SCPBallot>(newH);
}
// remember newH's value
mValueOverride = std::make_unique<Value>(newH.value);

if (newC.counter != 0)
// we don't set c/h if we're not on a compatible ballot
if (!mCurrentBallot || areBallotsCompatible(*mCurrentBallot, newH))
{
dbgAssert(!mCommit);
mCommit = std::make_unique<SCPBallot>(newC);
didWork = true;
if (!mHighBallot || compareBallots(newH, *mHighBallot) > 0)
{
didWork = true;
mHighBallot = std::make_unique<SCPBallot>(newH);
}

if (newC.counter != 0)
{
dbgAssert(!mCommit);
mCommit = std::make_unique<SCPBallot>(newC);
didWork = true;
}

if (didWork)
{
mSlot.getSCPDriver().confirmedBallotPrepared(mSlot.getSlotIndex(),
newH);
}
}

// always perform step (8) with the computed value of h
didWork = updateCurrentIfNeeded(newH) || didWork;

if (didWork)
{
updateCurrentIfNeeded();

mSlot.getSCPDriver().confirmedBallotPrepared(mSlot.getSlotIndex(),
newH);
emitCurrentStateStatement();
}

Expand Down Expand Up @@ -1272,6 +1302,9 @@ BallotProtocol::setAcceptCommit(SCPBallot const& c, SCPBallot const& h)

bool didWork = false;

// remember h's value
mValueOverride = std::make_unique<Value>(h.value);

if (!mHighBallot || !mCommit || compareBallots(*mHighBallot, h) != 0 ||
compareBallots(*mCommit, c) != 0)
{
Expand All @@ -1295,7 +1328,7 @@ BallotProtocol::setAcceptCommit(SCPBallot const& c, SCPBallot const& h)

if (didWork)
{
updateCurrentIfNeeded();
updateCurrentIfNeeded(*mHighBallot);

mSlot.getSCPDriver().acceptedCommit(mSlot.getSlotIndex(), h);
emitCurrentStateStatement();
Expand Down Expand Up @@ -1473,7 +1506,7 @@ BallotProtocol::setConfirmCommit(SCPBallot const& c, SCPBallot const& h)

mCommit = std::make_unique<SCPBallot>(c);
mHighBallot = std::make_unique<SCPBallot>(h);
updateCurrentIfNeeded();
updateCurrentIfNeeded(*mHighBallot);

mPhase = SCP_PHASE_EXTERNALIZE;

Expand Down
3 changes: 2 additions & 1 deletion src/scp/BallotProtocol.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ class BallotProtocol
std::unique_ptr<SCPBallot> mCommit; // c
std::map<NodeID, SCPEnvelope> mLatestEnvelopes; // M
SCPPhase mPhase; // Phi
std::unique_ptr<Value> mValueOverride; // z

int mCurrentMessageLevel; // number of messages triggered in one run

Expand Down Expand Up @@ -164,7 +165,7 @@ class BallotProtocol
std::set<SCPBallot> getPrepareCandidates(SCPStatement const& hint);

// helper to perform step (8) from the paper
void updateCurrentIfNeeded();
bool updateCurrentIfNeeded(SCPBallot const& h);

// An interval is [low,high] represented as a pair
using Interval = std::pair<uint32, uint32>;
Expand Down
110 changes: 79 additions & 31 deletions src/scp/SCPTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -612,7 +612,7 @@ TEST_CASE("ballot protocol core5", "[scp][ballotprotocol]")
{
REQUIRE(scp.mEnvs.size() == i);
}
if (checkUpcoming)
if (checkUpcoming && !delayedQuorum)
{
REQUIRE(scp.hasBallotTimerUpcoming());
}
Expand All @@ -622,9 +622,17 @@ TEST_CASE("ballot protocol core5", "[scp][ballotprotocol]")
{
REQUIRE(scp.mEnvs.size() == i);
}
if (checkUpcoming && delayedQuorum)
{
REQUIRE(scp.hasBallotTimerUpcoming());
}

};
// doesn't check timers
auto recvQuorumChecks = std::bind(recvQuorumChecksEx, _1, _2, _3, false);
// checks enabled, no delayed quorum
auto recvQuorumEx = std::bind(recvQuorumChecksEx, _1, true, false, _2);
// checks enabled, no delayed quorum, no check timers
auto recvQuorum = std::bind(recvQuorumEx, _1, false);

auto nodesAllPledgeToCommit = [&]() {
Expand Down Expand Up @@ -1016,6 +1024,12 @@ TEST_CASE("ballot protocol core5", "[scp][ballotprotocol]")
verifyPrepare(scp.mEnvs[5], v0SecretKey, qSetHash0,
0, A2, &B2, 0, 2, &A2);
REQUIRE(!scp.hasBallotTimerUpcoming());

recvQuorum(makePrepareGen(qSetHash, B2, &B2, 2, 2));
REQUIRE(scp.mEnvs.size() == 7);
verifyConfirm(scp.mEnvs[6], v0SecretKey, qSetHash0,
0, 2, B2, 2, 2);
REQUIRE(!scp.hasBallotTimerUpcoming());
}
SECTION("higher counter")
{
Expand Down Expand Up @@ -1093,6 +1107,14 @@ TEST_CASE("ballot protocol core5", "[scp][ballotprotocol]")
0, 0, &A1);
REQUIRE(!scp.hasBallotTimerUpcoming());
}
SECTION("switch prepare B1")
{
recvQuorumChecks(makePrepareGen(qSetHash, B1), true, true);
REQUIRE(scp.mEnvs.size() == 3);
verifyPrepare(scp.mEnvs[2], v0SecretKey, qSetHash0, 0, A1, &B1,
0, 0, &A1);
REQUIRE(!scp.hasBallotTimerUpcoming());
}
}
SECTION("prepared B (v-blocking)")
{
Expand All @@ -1101,6 +1123,12 @@ TEST_CASE("ballot protocol core5", "[scp][ballotprotocol]")
verifyPrepare(scp.mEnvs[1], v0SecretKey, qSetHash0, 0, A1, &B1);
REQUIRE(!scp.hasBallotTimer());
}
SECTION("prepare B (quorum)")
{
recvQuorumChecksEx(makePrepareGen(qSetHash, B1), true, true, true);
REQUIRE(scp.mEnvs.size() == 2);
verifyPrepare(scp.mEnvs[1], v0SecretKey, qSetHash0, 0, A1, &B1);
}
SECTION("confirm (v-blocking)")
{
SECTION("via CONFIRM")
Expand Down Expand Up @@ -1513,22 +1541,20 @@ TEST_CASE("ballot protocol core5", "[scp][ballotprotocol]")
}
SECTION("mixed B2")
{
// causes h=B2
// but c = 0, as p >!~ h
// causes computed_h=B2 ~ not set as h ~!= b
// -> noop
scp.bumpTimerOffset();
scp.receiveEnvelope(
makePrepare(v3SecretKey, qSetHash, 0, A2, &B2));

REQUIRE(scp.mEnvs.size() == 6);
verifyPrepare(scp.mEnvs[5], v0SecretKey, qSetHash0, 0,
A2, &A2, 0, 2, &B2);
REQUIRE(scp.mEnvs.size() == 5);
REQUIRE(!scp.hasBallotTimerUpcoming());

scp.bumpTimerOffset();
scp.receiveEnvelope(
makePrepare(v4SecretKey, qSetHash, 0, B2, &B2));

REQUIRE(scp.mEnvs.size() == 6);
REQUIRE(scp.mEnvs.size() == 5);
REQUIRE(!scp.hasBallotTimerUpcoming());
}
}
Expand Down Expand Up @@ -1983,6 +2009,32 @@ TEST_CASE("ballot protocol core5", "[scp][ballotprotocol]")
verifyPrepare(scp.mEnvs[3], v0SecretKey, qSetHash0, 0, newbx, &bx,
bx.counter, bx.counter);
}
SECTION("timeout when h exists but can't be set -> vote for h")
{
// start with (1,y)
SCPBallot by(1, yValue);
REQUIRE(scp.bumpState(0, yValue));
REQUIRE(scp.mEnvs.size() == 1);

SCPBallot bx(1, xValue);
// but quorum goes with (1,x)
// v-blocking -> prepared
recvVBlocking(makePrepareGen(qSetHash, bx, &bx));
REQUIRE(scp.mEnvs.size() == 2);
verifyPrepare(scp.mEnvs[1], v0SecretKey, qSetHash0, 0, by, &bx);
// quorum -> confirm prepared (no-op as b > h)
recvQuorumChecks(makePrepareGen(qSetHash, bx, &bx), false, false);
REQUIRE(scp.mEnvs.size() == 2);

REQUIRE(scp.bumpState(0, yValue));
REQUIRE(scp.mEnvs.size() == 3);
SCPBallot newbx(2, xValue);
// on timeout:
// * we should move to the quorum's h value
// * c can't be set yet as b > h
verifyPrepare(scp.mEnvs[2], v0SecretKey, qSetHash0, 0, newbx, &bx, 0,
bx.counter);
}

SECTION("timeout from multiple nodes")
{
Expand Down Expand Up @@ -2224,42 +2276,38 @@ TEST_CASE("ballot protocol core3", "[scp][ballotprotocol]")
recvQuorumChecks(makePrepareGen(qSetHash, B1), true, true);
REQUIRE(scp.mEnvs.size() == 2);
verifyPrepare(scp.mEnvs[1], v0SecretKey, qSetHash0, 0, A1, &B1);
REQUIRE(scp.hasBallotTimer());
REQUIRE(scp.hasBallotTimerUpcoming());
SECTION("quorum prepared B1")
{
scp.bumpTimerOffset();
recvQuorumChecks(makePrepareGen(qSetHash, B1, &B1), true, false);
#ifdef GOOD
REQUIRE(scp.mEnvs.size() == 3);
#else
// BAD (should be noop)
REQUIRE(scp.mEnvs.size() == 3);
verifyPrepare(scp.mEnvs[2], v0SecretKey, qSetHash0, 0, A1, &B1, 0,
1);
// REQUIRE(!scp.hasBallotTimer());
#endif
SECTION("quorum bumps to A2")
recvQuorumChecks(makePrepareGen(qSetHash, B1, &B1), false, false);
REQUIRE(scp.mEnvs.size() == 2);
// nothing happens:
// computed_h = B1 (2)
// does not actually update h as b > computed_h
// also skips (3)
REQUIRE(!scp.hasBallotTimerUpcoming());
SECTION("quorum bumps to A1")
{
scp.bumpTimerOffset();
recvQuorumChecksEx2(makePrepareGen(qSetHash, A2, &B1), false,
recvQuorumChecksEx2(makePrepareGen(qSetHash, A1, &B1), false,
false, false, true);
#ifdef GOOD
REQUIRE(scp.mEnvs.size() == 7);
REQUIRE(scp.hasBallotTimerUpcoming());
#else
REQUIRE(scp.mEnvs.size() == 4);
verifyPrepare(scp.mEnvs[3], v0SecretKey, qSetHash0, 0, A1, &A1,
0, 1, &B1);

REQUIRE(scp.mEnvs.size() == 3);
// still does not set h as b > computed_h
verifyPrepare(scp.mEnvs[2], v0SecretKey, qSetHash0, 0, A1, &A1,
0, 0, &B1);
REQUIRE(!scp.hasBallotTimerUpcoming());

scp.bumpTimerOffset();
// quorum commits A1
recvQuorumChecksEx2(
makePrepareGen(qSetHash, A2, &A1, 1, 1, &B1), false, false,
false, true);
REQUIRE(scp.mEnvs.size() == 5);
verifyConfirm(scp.mEnvs[4], v0SecretKey, qSetHash0, 0, 3, A3, 3,
3);
#endif
REQUIRE(scp.mEnvs.size() == 4);
verifyConfirm(scp.mEnvs[3], v0SecretKey, qSetHash0, 0, 2, A1, 1,
1);
REQUIRE(!scp.hasBallotTimerUpcoming());
}
}
}
Expand Down

5 comments on commit 0640fde

@latobarita
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saw approval from MonsieurNicolas
at MonsieurNicolas@0640fde

@latobarita
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merging MonsieurNicolas/stellar-core/SCPFixHighConfirmed = 0640fde into auto

@latobarita
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MonsieurNicolas/stellar-core/SCPFixHighConfirmed = 0640fde merged ok, testing candidate = 675382d

@latobarita
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all tests pass:
success: https://travis-ci.org/stellar/stellar-core/builds/421694946?utm_source=github_status&utm_medium=notification

@latobarita
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fast-forwarding master to auto = 675382d

Please sign in to comment.