diff --git a/src/scp/BallotProtocol.cpp b/src/scp/BallotProtocol.cpp index c65d3327bf..cf4141e81b 100644 --- a/src/scp/BallotProtocol.cpp +++ b/src/scp/BallotProtocol.cpp @@ -378,7 +378,7 @@ BallotProtocol::updateCurrentValue(SCPBallot const& ballot) bool updated = false; if (!mCurrentBallot) { - bumpToBallot(ballot); + bumpToBallot(ballot, true); updated = true; } else @@ -393,7 +393,7 @@ BallotProtocol::updateCurrentValue(SCPBallot const& ballot) int comp = compareBallots(*mCurrentBallot, ballot); if (comp < 0) { - bumpToBallot(ballot); + bumpToBallot(ballot, true); updated = true; } else if (comp > 0) @@ -410,8 +410,6 @@ BallotProtocol::updateCurrentValue(SCPBallot const& ballot) "a smaller value"; // can't just bump to the value as we may already have // statements at counter+1 - // bumpToBallot(SCPBallot(mCurrentBallot->counter + 1, - // ballot.value)); return false; } } @@ -427,7 +425,7 @@ BallotProtocol::updateCurrentValue(SCPBallot const& ballot) } void -BallotProtocol::bumpToBallot(SCPBallot const& ballot) +BallotProtocol::bumpToBallot(SCPBallot const& ballot, bool check) { CLOG(DEBUG, "SCP") << "BallotProtocol::bumpToBallot" << " i: " << mSlot.getSlotIndex() @@ -436,11 +434,14 @@ BallotProtocol::bumpToBallot(SCPBallot const& ballot) // `bumpToBallot` should be never called once we committed. dbgAssert(mPhase != SCP_PHASE_EXTERNALIZE); - // We should move mCurrentBallot monotonically only - dbgAssert(!mCurrentBallot || compareBallots(ballot, *mCurrentBallot) >= 0); + if (check) + { + // We should move mCurrentBallot monotonically only + dbgAssert(!mCurrentBallot || + compareBallots(ballot, *mCurrentBallot) >= 0); + } - bool gotBumped = - !mCurrentBallot || (mCurrentBallot->counter != ballot.counter); + bool gotBumped = !mCurrentBallot || !(*mCurrentBallot == ballot); mCurrentBallot = make_unique(ballot); @@ -567,23 +568,31 @@ BallotProtocol::emitCurrentStateStatement() bool canEmit = (mCurrentBallot != nullptr); - if (mSlot.processEnvelope(envelope, true) == SCP::EnvelopeState::VALID) + // if we generate the same envelope, don't process it again + // this can occur when updating h in PREPARE phase + // as statements only keep track of h.n (but h.x could be different) + auto lastEnv = mLatestEnvelopes.find(mSlot.getSCP().getLocalNodeID()); + + if (lastEnv == mLatestEnvelopes.end() || !(lastEnv->second == envelope)) { - if (canEmit && - (!mLastEnvelope || - isNewerStatement(mLastEnvelope->statement, envelope.statement))) + if (mSlot.processEnvelope(envelope, true) == SCP::EnvelopeState::VALID) { - mLastEnvelope = make_unique(envelope); - // this will no-op if invoked from advanceSlot - // as advanceSlot consolidates all messages - sendLatestEnvelope(); + if (canEmit && + (!mLastEnvelope || isNewerStatement(mLastEnvelope->statement, + envelope.statement))) + { + mLastEnvelope = std::make_shared(envelope); + // this will no-op if invoked from advanceSlot + // as advanceSlot consolidates all messages sent + sendLatestEnvelope(); + } + } + else + { + // there is a bug in the application if it queued up + // a statement for itself that it considers invalid + throw std::runtime_error("moved to a bad state (ballot protocol)"); } - } - else - { - // there is a bug in the application if it queued up - // a statement for itself that it considers invalid - throw std::runtime_error("moved to a bad state (ballot protocol)"); } } @@ -636,6 +645,10 @@ BallotProtocol::getPrepareCandidates(SCPStatement const& hint) { hintBallots.insert(*prep.prepared); } + if (prep.preparedPrime) + { + hintBallots.insert(*prep.preparedPrime); + } } break; case SCP_ST_CONFIRM: @@ -681,6 +694,11 @@ BallotProtocol::getPrepareCandidates(SCPStatement const& hint) { candidates.insert(*prep.prepared); } + if (prep.preparedPrime && + areBallotsLessAndCompatible(*prep.preparedPrime, topVote)) + { + candidates.insert(*prep.preparedPrime); + } } break; case SCP_ST_CONFIRM: @@ -715,7 +733,7 @@ BallotProtocol::updateCurrentIfNeeded() { if (!mCurrentBallot || compareBallots(*mCurrentBallot, *mHighBallot) < 0) { - bumpToBallot(*mHighBallot); + bumpToBallot(*mHighBallot, true); } } @@ -746,11 +764,23 @@ BallotProtocol::attemptPreparedAccept(SCPStatement const& hint) } // if we already prepared this ballot, don't bother checking again - if (mPrepared && compareBallots(ballot, *mPrepared) <= 0) + + // if ballot <= p' ballot is neither a candidate for p nor p' + if (mPreparedPrime && compareBallots(ballot, *mPreparedPrime) <= 0) { continue; } + if (mPrepared) + { + // if ballot is already covered by p, skip + if (areBallotsLessAndCompatible(ballot, *mPrepared)) + { + continue; + } + // otherwise, there is a chance it increases p' + } + bool accepted = federatedAccept( // checks if any node is voting for this ballot [&ballot, this](SCPStatement const& st) @@ -1200,35 +1230,32 @@ BallotProtocol::setAcceptCommit(SCPBallot const& c, SCPBallot const& h) bool didWork = false; - if (mCurrentBallot && compareBallots(h, *mCurrentBallot) < 0 && - !areBallotsCompatible(h, *mCurrentBallot)) + if (!mHighBallot || !mCommit || compareBallots(*mHighBallot, h) != 0 || + compareBallots(*mCommit, c) != 0) { - // we would end up with h < b and !(h ~ b) which is an invalid state - CLOG(DEBUG, "SCP") << "Ignoring new values: would violate h ~ b"; + mCommit = make_unique(c); + mHighBallot = make_unique(h); + + didWork = true; } - else + + if (mPhase == SCP_PHASE_PREPARE) { - if (!mHighBallot || !mCommit || compareBallots(*mHighBallot, h) != 0 || - compareBallots(*mCommit, c) != 0) + mPhase = SCP_PHASE_CONFIRM; + if (mCurrentBallot && !areBallotsLessAndCompatible(h, *mCurrentBallot)) { - mCommit = make_unique(c); - mHighBallot = make_unique(h); - didWork = true; + bumpToBallot(h, false); } - if (mPhase == SCP_PHASE_PREPARE) - { - mPhase = SCP_PHASE_CONFIRM; - didWork = true; - } + didWork = true; + } - if (didWork) - { - updateCurrentIfNeeded(); + if (didWork) + { + updateCurrentIfNeeded(); - mSlot.getSCPDriver().acceptedCommit(mSlot.getSlotIndex(), h); - emitCurrentStateStatement(); - } + mSlot.getSCPDriver().acceptedCommit(mSlot.getSlotIndex(), h); + emitCurrentStateStatement(); } return didWork; @@ -1434,7 +1461,10 @@ BallotProtocol::hasPreparedBallot(SCPBallot const& ballot, case SCP_ST_PREPARE: { auto const& p = st.pledges.prepare(); - res = p.prepared && areBallotsLessAndCompatible(ballot, *p.prepared); + res = + (p.prepared && areBallotsLessAndCompatible(ballot, *p.prepared)) || + (p.preparedPrime && + areBallotsLessAndCompatible(ballot, *p.preparedPrime)); } break; case SCP_ST_CONFIRM: @@ -1510,7 +1540,8 @@ BallotProtocol::setPrepared(SCPBallot const& ballot) if (mPrepared) { - if (compareBallots(*mPrepared, ballot) < 0) + int comp = compareBallots(*mPrepared, ballot); + if (comp < 0) { if (!areBallotsCompatible(*mPrepared, ballot)) { @@ -1519,6 +1550,15 @@ BallotProtocol::setPrepared(SCPBallot const& ballot) mPrepared = make_unique(ballot); didWork = true; } + else if (comp > 0) + { + // check if we should update only p' + if (!mPreparedPrime || compareBallots(*mPreparedPrime, ballot) < 0) + { + mPreparedPrime = make_unique(ballot); + didWork = true; + } + } } else { @@ -1609,7 +1649,8 @@ BallotProtocol::setStateFromEnvelope(SCPEnvelope const& e) recordEnvelope(e); - mLastEnvelope = make_unique(e); + mLastEnvelope = std::make_shared(e); + mLastEnvelopeEmit = mLastEnvelope; auto const& pl = e.statement.pledges; @@ -1619,7 +1660,7 @@ BallotProtocol::setStateFromEnvelope(SCPEnvelope const& e) { auto const& prep = pl.prepare(); auto const& b = prep.ballot; - bumpToBallot(b); + bumpToBallot(b, true); if (prep.prepared) { mPrepared = make_unique(*prep.prepared); @@ -1643,7 +1684,7 @@ BallotProtocol::setStateFromEnvelope(SCPEnvelope const& e) { auto const& c = pl.confirm(); auto const& v = c.ballot.value; - bumpToBallot(c.ballot); + bumpToBallot(c.ballot, true); mPrepared = make_unique(c.nPrepared, v); mHighBallot = make_unique(c.nH, v); mCommit = make_unique(c.nCommit, v); @@ -1654,7 +1695,7 @@ BallotProtocol::setStateFromEnvelope(SCPEnvelope const& e) { auto const& ext = pl.externalize(); auto const& v = ext.commit.value; - bumpToBallot(SCPBallot(UINT32_MAX, v)); + bumpToBallot(SCPBallot(UINT32_MAX, v), true); mPrepared = make_unique(UINT32_MAX, v); mHighBallot = make_unique(ext.nH, v); mCommit = make_unique(ext.commit); @@ -1818,7 +1859,11 @@ BallotProtocol::sendLatestEnvelope() // emit current envelope if needed if (mCurrentMessageLevel == 0 && mLastEnvelope && mSlot.isFullyValidated()) { - mSlot.getSCPDriver().emitEnvelope(*mLastEnvelope); + if (!mLastEnvelopeEmit || mLastEnvelope != mLastEnvelopeEmit) + { + mLastEnvelopeEmit = mLastEnvelope; + mSlot.getSCPDriver().emitEnvelope(*mLastEnvelopeEmit); + } } } diff --git a/src/scp/BallotProtocol.h b/src/scp/BallotProtocol.h index bf8937040f..d07f256ee2 100644 --- a/src/scp/BallotProtocol.h +++ b/src/scp/BallotProtocol.h @@ -51,8 +51,11 @@ class BallotProtocol int mCurrentMessageLevel; // number of messages triggered in one run - std::unique_ptr - mLastEnvelope; // last envelope emitted by this node + std::shared_ptr + mLastEnvelope; // last envelope generated by this node + + std::shared_ptr + mLastEnvelopeEmit; // last envelope emitted by this node public: BallotProtocol(Slot& slot); @@ -99,7 +102,7 @@ class BallotProtocol SCPEnvelope* getLastMessageSend() const { - return mLastEnvelope.get(); + return mLastEnvelopeEmit.get(); } void setStateFromEnvelope(SCPEnvelope const& e); @@ -227,7 +230,8 @@ class BallotProtocol // helper function that updates the current ballot // this is the lowest level method to update the current ballot and as // such doesn't do any validation - void bumpToBallot(SCPBallot const& ballot); + // check: verifies that ballot is greater than old one + void bumpToBallot(SCPBallot const& ballot, bool check); // switch the local node to the given ballot's value // with the assumption that the ballot is more recent than the one diff --git a/src/scp/SCPTests.cpp b/src/scp/SCPTests.cpp index e4e4355c6c..05a16564f0 100644 --- a/src/scp/SCPTests.cpp +++ b/src/scp/SCPTests.cpp @@ -911,6 +911,14 @@ TEST_CASE("ballot protocol core5", "[scp][ballotprotocol]") verifyConfirm(scp.mEnvs[6], v0SecretKey, qSetHash0, 0, 3, A3, 2, 2); } + SECTION("v-blocking prepared A3+B3") + { + recvVBlocking(makePrepareGen(qSetHash, A3, &B3, + 2, 2, &A3)); + REQUIRE(scp.mEnvs.size() == 7); + verifyConfirm(scp.mEnvs[6], v0SecretKey, + qSetHash0, 0, 3, A3, 2, 2); + } SECTION("v-blocking confirm A3") { recvVBlocking( @@ -1027,12 +1035,66 @@ TEST_CASE("ballot protocol core5", "[scp][ballotprotocol]") } } } - SECTION("get conflicting prepared B2") + SECTION("get conflicting prepared B") { - recvVBlocking(makePrepareGen(qSetHash, B2, &B2)); + SECTION("same counter") + { + recvVBlocking(makePrepareGen(qSetHash, B2, &B2)); + REQUIRE(scp.mEnvs.size() == 6); + verifyPrepare(scp.mEnvs[5], v0SecretKey, qSetHash0, + 0, A2, &B2, 0, 2, &A2); + } + SECTION("higher counter") + { + recvVBlocking( + makePrepareGen(qSetHash, B3, &B2, 2, 2)); + REQUIRE(scp.mEnvs.size() == 6); + verifyPrepare(scp.mEnvs[5], v0SecretKey, qSetHash0, + 0, A3, &B2, 0, 2, &A2); + + recvQuorumChecks(makePrepareGen(qSetHash, B3, &B2, 2, 2), false); + REQUIRE(scp.mEnvs.size() == 6); + } + } + } + SECTION("Confirm prepared mixed") + { + // a few nodes prepared B2 + recvVBlocking(makePrepareGen(qSetHash, B2, &B2, 0, 0, &A2)); + REQUIRE(scp.mEnvs.size() == 5); + verifyPrepare(scp.mEnvs[4], v0SecretKey, qSetHash0, 0, A2, + &B2, 0, 0, &A2); + + SECTION("mixed A2") + { + // causes h=A2 + // but c = 0, as p >!~ h + scp.receiveEnvelope( + makePrepare(v3SecretKey, qSetHash, 0, A2, &A2)); + REQUIRE(scp.mEnvs.size() == 6); verifyPrepare(scp.mEnvs[5], v0SecretKey, qSetHash0, 0, A2, &B2, 0, 2, &A2); + + scp.receiveEnvelope( + makePrepare(v4SecretKey, qSetHash, 0, A2, &A2)); + + REQUIRE(scp.mEnvs.size() == 6); + } + SECTION("mixed B2") + { + // causes h=B2, c=B2 + scp.receiveEnvelope( + makePrepare(v3SecretKey, qSetHash, 0, B2, &B2)); + + REQUIRE(scp.mEnvs.size() == 6); + verifyPrepare(scp.mEnvs[5], v0SecretKey, qSetHash0, 0, + B2, &B2, 2, 2, &A2); + + scp.receiveEnvelope( + makePrepare(v4SecretKey, qSetHash, 0, B2, &B2)); + + REQUIRE(scp.mEnvs.size() == 6); } } } @@ -1212,6 +1274,14 @@ TEST_CASE("ballot protocol core5", "[scp][ballotprotocol]") verifyConfirm(scp.mEnvs[6], v0SecretKey, qSetHash0, 0, 3, A3, 2, 2); } + SECTION("v-blocking prepared A3+B3") + { + recvVBlocking(makePrepareGen(qSetHash, A3, &A3, + 2, 2, &B3)); + REQUIRE(scp.mEnvs.size() == 7); + verifyConfirm(scp.mEnvs[6], v0SecretKey, + qSetHash0, 0, 3, A3, 2, 2); + } SECTION("v-blocking confirm A3") { recvVBlocking( @@ -1298,10 +1368,11 @@ TEST_CASE("ballot protocol core5", "[scp][ballotprotocol]") } SECTION("CONFIRM B2") { - recvVBlockingChecks( - makeConfirmGen(qSetHash, 2, B2, 2, 2), - false); - REQUIRE(scp.mEnvs.size() == 5); + recvVBlocking( + makeConfirmGen(qSetHash, 2, B2, 2, 2)); + REQUIRE(scp.mEnvs.size() == 6); + verifyConfirm(scp.mEnvs[5], v0SecretKey, + qSetHash0, 0, 2, B2, 2, 2); } } SECTION("EXTERNALIZE") @@ -1329,12 +1400,78 @@ TEST_CASE("ballot protocol core5", "[scp][ballotprotocol]") } } } - SECTION("get conflicting prepared B2") + SECTION("get conflicting prepared B") + { + SECTION("same counter") + { + // messages are ignored as B2 < A2 + recvQuorumChecks(makePrepareGen(qSetHash, B2, &B2), + false); + REQUIRE(scp.mEnvs.size() == 5); + } + SECTION("higher counter") + { + recvVBlocking( + makePrepareGen(qSetHash, B3, &B2, 2, 2)); + REQUIRE(scp.mEnvs.size() == 6); + // A2 > B2 -> p = A2, p'=B2 + verifyPrepare(scp.mEnvs[5], v0SecretKey, qSetHash0, + 0, A3, &A2, 2, 2, &B2); + + // node is stuck as it's trying to + // commit A2=<2,y> but rest of its quorum is + // trying to commit B2 + recvQuorumChecks( + makePrepareGen(qSetHash, B3, &B2, 2, 2), false); + REQUIRE(scp.mEnvs.size() == 6); + + // accept c=B2 ; also bumps counter to 3 + recvVBlocking( + makeConfirmGen(qSetHash, 2, B3, 2, 2)); + REQUIRE(scp.mEnvs.size() == 7); + verifyConfirm(scp.mEnvs[6], v0SecretKey, qSetHash0, + 0, 2, B3, 2, 2); + } + } + } + SECTION("Confirm prepared mixed") + { + // a few nodes prepared B2 + recvVBlocking(makePrepareGen(qSetHash, A2, &A2, 0, 0, &B2)); + REQUIRE(scp.mEnvs.size() == 5); + verifyPrepare(scp.mEnvs[4], v0SecretKey, qSetHash0, 0, A2, + &A2, 0, 0, &B2); + + SECTION("mixed A2") { - // can't switch to B2 - recvQuorumChecks(makePrepareGen(qSetHash, B2, &B2), - false); - REQUIRE(scp.mEnvs.size() == 5); + // causes h=A2, c=A2 + scp.receiveEnvelope( + makePrepare(v3SecretKey, qSetHash, 0, A2, &A2)); + + REQUIRE(scp.mEnvs.size() == 6); + verifyPrepare(scp.mEnvs[5], v0SecretKey, qSetHash0, 0, + A2, &A2, 2, 2, &B2); + + scp.receiveEnvelope( + makePrepare(v4SecretKey, qSetHash, 0, A2, &A2)); + + REQUIRE(scp.mEnvs.size() == 6); + } + SECTION("mixed B2") + { + // causes h=B2 + // but c = 0, as p >!~ h + 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); + + scp.receiveEnvelope( + makePrepare(v4SecretKey, qSetHash, 0, B2, &B2)); + + REQUIRE(scp.mEnvs.size() == 6); } } } @@ -1487,6 +1624,52 @@ TEST_CASE("ballot protocol core5", "[scp][ballotprotocol]") } } } + SECTION("Confirm prepared mixed") + { + // a few nodes prepared A2 + // causes p=A2 + recvVBlockingChecks(makePrepareGen(qSetHash, A2, &A2), + false); + REQUIRE(scp.mEnvs.size() == 0); + + // a few nodes prepared B2 + // causes p=B2, p'=A2 + recvVBlockingChecks( + makePrepareGen(qSetHash, A2, &B2, 0, 0, &A2), false); + REQUIRE(scp.mEnvs.size() == 0); + + SECTION("mixed A2") + { + // causes h=A2 + // but c = 0, as p >!~ h + scp.receiveEnvelope( + makePrepare(v3SecretKey, qSetHash, 0, A2, &A2)); + + REQUIRE(scp.mEnvs.size() == 1); + verifyPrepare(scp.mEnvs[0], v0SecretKey, qSetHash0, 0, + A2, &B2, 0, 2, &A2); + + scp.receiveEnvelope( + makePrepare(v4SecretKey, qSetHash, 0, A2, &A2)); + + REQUIRE(scp.mEnvs.size() == 1); + } + SECTION("mixed B2") + { + // causes h=B2, c=B2 + scp.receiveEnvelope( + makePrepare(v3SecretKey, qSetHash, 0, B2, &B2)); + + REQUIRE(scp.mEnvs.size() == 1); + verifyPrepare(scp.mEnvs[0], v0SecretKey, qSetHash0, 0, + B2, &B2, 2, 2, &A2); + + scp.receiveEnvelope( + makePrepare(v4SecretKey, qSetHash, 0, B2, &B2)); + + REQUIRE(scp.mEnvs.size() == 1); + } + } } SECTION("switch prepared B1") {