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.

Sign up for GitHub

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

Jump to bottom

Put height into mined commitments and use it instead of the special handling of quorumVvecHash #2501

Merged
merged 3 commits into from
Nov 27, 2018
Merged

Put height into mined commitments and use it instead of the special handling of quorumVvecHash #2501

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6bc2fe5
Allow to skip sig verification for CFinalCommitment::Verify
codablock Nov 26, 2018
bfca0e0
Add CFinalCommitmentTxPayload and CheckLLMQCommitment and use it
codablock Nov 26, 2018
9f21ee1
Properly ban nodes for invalid commitments
codablock Nov 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/evo/deterministicmns.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,12 +674,12 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const C
LogPrintf("CDeterministicMNManager::%s -- MN %s revoked operator key at height %d: %s\n",
__func__, proTx.proTxHash.ToString(), nHeight, proTx.ToString());
} else if (tx.nType == TRANSACTION_QUORUM_COMMITMENT) {
llmq::CFinalCommitment qc;
llmq::CFinalCommitmentTxPayload qc;
if (!GetTxPayload(tx, qc)) {
assert(false); // this should have been handled already
}
if (!qc.IsNull()) {
HandleQuorumCommitment(qc, newList);
if (!qc.commitment.IsNull()) {
HandleQuorumCommitment(qc.commitment, newList);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/evo/specialtx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ bool CheckSpecialTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVali
case TRANSACTION_COINBASE:
return CheckCbTx(tx, pindexPrev, state);
case TRANSACTION_QUORUM_COMMITMENT:
return true; // can't really check much here. checks are done in ProcessBlock
return llmq::CheckLLMQCommitment(tx, pindexPrev, state);
}

return state.DoS(10, false, REJECT_INVALID, "bad-tx-type-check");
Expand Down
24 changes: 11 additions & 13 deletions src/llmq/quorums_blockprocessor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ void CQuorumBlockProcessor::ProcessMessage(CNode* pfrom, const std::string& strC

auto members = CLLMQUtils::GetAllQuorumMembers(type, qc.quorumHash);

if (!qc.Verify(members)) {
if (!qc.Verify(members, true)) {
LOCK(cs_main);
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this lock only need to happen for the Misbehaving? If that is the case, there isn't really a reason to have it above the print. (not really relevant tho ¯\(ツ)/¯)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, you're right. It's only for Misbehaving. I'd like to avoid fixing that for now as I don't want to loose the ACK for this PR if it's not really required...need to finish RC5.

LogPrintf("CQuorumBlockProcessor::%s -- commitment for quorum %s:%d is not valid, peer=%d\n", __func__,
qc.quorumHash.ToString(), qc.llmqType, pfrom->id);
Expand Down Expand Up @@ -167,7 +167,7 @@ bool CQuorumBlockProcessor::ProcessCommitment(const CBlockIndex* pindexPrev, con
}

if (qc.IsNull()) {
if (!qc.VerifyNull(pindexPrev->nHeight + 1)) {
if (!qc.VerifyNull()) {
return state.DoS(100, false, REJECT_INVALID, "bad-qc-invalid-null");
}
return true;
Expand All @@ -185,7 +185,7 @@ bool CQuorumBlockProcessor::ProcessCommitment(const CBlockIndex* pindexPrev, con

auto members = CLLMQUtils::GetAllQuorumMembers(params.type, quorumHash);

if (!qc.Verify(members)) {
if (!qc.Verify(members, true)) {
return state.DoS(100, false, REJECT_INVALID, "bad-qc-invalid");
}

Expand Down Expand Up @@ -236,21 +236,18 @@ bool CQuorumBlockProcessor::GetCommitmentsFromBlock(const CBlock& block, const C

for (const auto& tx : block.vtx) {
if (tx->nType == TRANSACTION_QUORUM_COMMITMENT) {
CFinalCommitment qc;
CFinalCommitmentTxPayload qc;
if (!GetTxPayload(*tx, qc)) {
// should not happen as it was verified before processing the block
return state.DoS(100, false, REJECT_INVALID, "bad-tx-payload");
}

if (!consensus.llmqs.count((Consensus::LLMQType)qc.llmqType)) {
return state.DoS(100, false, REJECT_INVALID, "bad-qc-type");
}

// only allow one commitment per type and per block
if (ret.count((Consensus::LLMQType)qc.llmqType)) {
if (ret.count((Consensus::LLMQType)qc.commitment.llmqType)) {
return state.DoS(100, false, REJECT_INVALID, "bad-qc-dup");
}

ret.emplace((Consensus::LLMQType)qc.llmqType, std::move(qc));
ret.emplace((Consensus::LLMQType)qc.commitment.llmqType, std::move(qc.commitment));
}
}

Expand Down Expand Up @@ -406,7 +403,6 @@ bool CQuorumBlockProcessor::GetMinableCommitment(Consensus::LLMQType llmqType, c
if (it == minableCommitmentsByQuorum.end()) {
// null commitment required
ret = CFinalCommitment(Params().GetConsensus().llmqs.at(llmqType), quorumHash);
ret.quorumVvecHash = ::SerializeHash(std::make_pair(quorumHash, nHeight));
return true;
}

Expand All @@ -419,11 +415,13 @@ bool CQuorumBlockProcessor::GetMinableCommitmentTx(Consensus::LLMQType llmqType,
{
AssertLockHeld(cs_main);

CFinalCommitment qc;
if (!GetMinableCommitment(llmqType, pindexPrev, qc)) {
CFinalCommitmentTxPayload qc;
if (!GetMinableCommitment(llmqType, pindexPrev, qc.commitment)) {
return false;
}

qc.nHeight = pindexPrev->nHeight + 1;

CMutableTransaction tx;
tx.nVersion = 3;
tx.nType = TRANSACTION_QUORUM_COMMITMENT;
Expand Down
90 changes: 69 additions & 21 deletions src/llmq/quorums_commitment.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
#include "quorums_utils.h"

#include "chainparams.h"
#include "validation.h"

#include "evo/specialtx.h"

#include <univalue.h>

Expand All @@ -24,7 +27,7 @@ CFinalCommitment::CFinalCommitment(const Consensus::LLMQParams& params, const ui
LogPrintStr(strprintf("CFinalCommitment::%s -- %s", __func__, tinyformat::format(__VA_ARGS__))); \
} while(0)

bool CFinalCommitment::Verify(const std::vector<CDeterministicMNCPtr>& members) const
bool CFinalCommitment::Verify(const std::vector<CDeterministicMNCPtr>& members, bool checkSigs) const
{
if (nVersion == 0 || nVersion > CURRENT_VERSION) {
return false;
Expand Down Expand Up @@ -76,30 +79,33 @@ bool CFinalCommitment::Verify(const std::vector<CDeterministicMNCPtr>& members)
}
}

uint256 commitmentHash = CLLMQUtils::BuildCommitmentHash((uint8_t)params.type, quorumHash, validMembers, quorumPublicKey, quorumVvecHash);
// sigs are only checked when the block is processed
UdjinM6 marked this conversation as resolved.
Show resolved Hide resolved
if (checkSigs) {
uint256 commitmentHash = CLLMQUtils::BuildCommitmentHash((uint8_t)params.type, quorumHash, validMembers, quorumPublicKey, quorumVvecHash);

std::vector<CBLSPublicKey> memberPubKeys;
for (size_t i = 0; i < members.size(); i++) {
if (!signers[i]) {
continue;
std::vector<CBLSPublicKey> memberPubKeys;
for (size_t i = 0; i < members.size(); i++) {
if (!signers[i]) {
continue;
}
memberPubKeys.emplace_back(members[i]->pdmnState->pubKeyOperator);
}
memberPubKeys.emplace_back(members[i]->pdmnState->pubKeyOperator);
}

if (!membersSig.VerifySecureAggregated(memberPubKeys, commitmentHash)) {
LogPrintfFinalCommitment("invalid aggregated members signature\n");
return false;
}
if (!membersSig.VerifySecureAggregated(memberPubKeys, commitmentHash)) {
LogPrintfFinalCommitment("invalid aggregated members signature\n");
return false;
}

if (!quorumSig.VerifyInsecure(quorumPublicKey, commitmentHash)) {
LogPrintfFinalCommitment("invalid quorum signature\n");
return false;
if (!quorumSig.VerifyInsecure(quorumPublicKey, commitmentHash)) {
LogPrintfFinalCommitment("invalid quorum signature\n");
return false;
}
}

return true;
}

bool CFinalCommitment::VerifyNull(int nHeight) const
bool CFinalCommitment::VerifyNull() const
Copy link
Member

Choose a reason for hiding this comment

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

Not relevant for this PR but I'd like a comment on this method normally, overall comments are fairly limited in this new code

Copy link
Author

Choose a reason for hiding this comment

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

I'll create a PR in the next days to include more documentation in general, hope that helps understanding all the new code.

{
if (!Params().GetConsensus().llmqs.count((Consensus::LLMQType)llmqType)) {
LogPrintfFinalCommitment("invalid llmqType=%d\n", llmqType);
Expand All @@ -111,11 +117,6 @@ bool CFinalCommitment::VerifyNull(int nHeight) const
return false;
}

uint256 expectedQuorumVvecHash = ::SerializeHash(std::make_pair(quorumHash, nHeight));
if (quorumVvecHash != expectedQuorumVvecHash) {
return false;
}

return true;
}

Expand Down Expand Up @@ -143,4 +144,51 @@ void CFinalCommitment::ToJson(UniValue& obj) const
obj.push_back(Pair("quorumPublicKey", quorumPublicKey.ToString()));
}

bool CheckLLMQCommitment(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state)
{
CFinalCommitmentTxPayload qcTx;
if (!GetTxPayload(tx, qcTx)) {
return state.DoS(100, false, REJECT_INVALID, "bad-qc-payload");
}

if (qcTx.nVersion == 0 || qcTx.nVersion > CFinalCommitmentTxPayload::CURRENT_VERSION) {
return state.DoS(100, false, REJECT_INVALID, "bad-qc-version");
}

if (qcTx.nHeight != pindexPrev->nHeight + 1) {
return state.DoS(100, false, REJECT_INVALID, "bad-qc-height");
}

if (!mapBlockIndex.count(qcTx.commitment.quorumHash)) {
Copy link
Member

Choose a reason for hiding this comment

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

I prefer it to be explicit, I think it is easier to read and it conveys the intent better

Suggested change
if (!mapBlockIndex.count(qcTx.commitment.quorumHash)) {
if (mapBlockIndex.count(qcTx.commitment.quorumHash) != 0) {

return state.DoS(100, false, REJECT_INVALID, "bad-qc-quorum-hash");
}

const CBlockIndex* pindexQuorum = mapBlockIndex[qcTx.commitment.quorumHash];

if (pindexQuorum != pindexPrev->GetAncestor(pindexQuorum->nHeight)) {
// not part of active chain
return state.DoS(100, false, REJECT_INVALID, "bad-qc-quorum-hash");
}

if (!Params().GetConsensus().llmqs.count((Consensus::LLMQType)qcTx.commitment.llmqType)) {
LogPrintfFinalCommitment("invalid llmqType=%d\n", qcTx.commitment.llmqType);
return false;
UdjinM6 marked this conversation as resolved.
Show resolved Hide resolved
}
const auto& params = Params().GetConsensus().llmqs.at((Consensus::LLMQType)qcTx.commitment.llmqType);
Copy link
Member

Choose a reason for hiding this comment

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

might make sense to have Params().GetConsensus().llmqs stored in a variable instead of fetching it twice, but compiler would probably do that optimization already ¯\(ツ)/¯

Copy link
Author

Choose a reason for hiding this comment

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

Well, the first one is checking for existence (which is an important validity check). The second one actually fetches it (by using at to to indicate that existence is a requirement). I'm usually in favor of using .find(), but only if performance is important at that point (e.g. possibly large maps)


if (qcTx.commitment.IsNull()) {
if (!qcTx.commitment.VerifyNull()) {
return state.DoS(100, false, REJECT_INVALID, "bad-qc-invalid-null");
}
return true;
}

auto members = CLLMQUtils::GetAllQuorumMembers(params.type, qcTx.commitment.quorumHash);
if (!qcTx.commitment.Verify(members, false)) {
return false;
UdjinM6 marked this conversation as resolved.
Show resolved Hide resolved
}

return true;
}

}
29 changes: 27 additions & 2 deletions src/llmq/quorums_commitment.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ class CFinalCommitment
return (int)std::count(validMembers.begin(), validMembers.end(), true);
}

bool Verify(const std::vector<CDeterministicMNCPtr>& members) const;
bool VerifyNull(int nHeight) const;
bool Verify(const std::vector<CDeterministicMNCPtr>& members, bool checkSigs) const;
bool VerifyNull() const;
bool VerifySizes(const Consensus::LLMQParams& params) const;

void ToJson(UniValue& obj) const;
Expand Down Expand Up @@ -79,6 +79,7 @@ class CFinalCommitment
return false;
}
if (quorumPublicKey.IsValid() ||
!quorumVvecHash.IsNull() ||
Copy link
Member

Choose a reason for hiding this comment

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

this seems bad... I don't really know enough about it though, so I'll just point it out and wait for you to say it's correct :)

Copy link
Author

Choose a reason for hiding this comment

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

It's correct, but you made me question it for a second ;)
This method returns false if any of the fields is non-null. For the BLS stuff, that means if they are valid. For uint256, it means that it's not null (all-zero).

membersSig.IsValid() ||
quorumSig.IsValid()) {
return false;
Expand All @@ -87,6 +88,30 @@ class CFinalCommitment
}
};

class CFinalCommitmentTxPayload
{
public:
static const uint16_t CURRENT_VERSION = 1;

public:
uint16_t nVersion{CURRENT_VERSION};
uint32_t nHeight{(uint32_t)-1};
CFinalCommitment commitment;

public:
ADD_SERIALIZE_METHODS

template<typename Stream, typename Operation>
inline void SerializationOp(Stream& s, Operation ser_action)
{
READWRITE(nVersion);
READWRITE(nHeight);
READWRITE(commitment);
}
};

bool CheckLLMQCommitment(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state);

}

#endif //DASH_QUORUMS_COMMITMENT_H
2 changes: 1 addition & 1 deletion src/llmq/quorums_dummydkg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ void CDummyDKG::CreateFinalCommitment(Consensus::LLMQType llmqType, const CBlock
}
fqc.membersSig = CBLSSignature::AggregateSecure(memberSigs, memberPubKeys, commitmentHash);

if (!fqc.Verify(members)) {
if (!fqc.Verify(members, true)) {
LogPrintf("CDummyDKG::%s -- self created final commitment is invalid for quorum %s:%d, validMembers=%d, signers=%d\n", __func__,
fqc.quorumHash.ToString(), fqc.llmqType, fqc.CountValidMembers(), fqc.CountSigners());
continue;
Expand Down