Put height into mined commitments and use it instead of the special handling of quorumVvecHash #2501
Conversation
As described in dashpay/dips#31 (see discussion).
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| bool CFinalCommitment::VerifyNull(int nHeight) const | ||
| bool CFinalCommitment::VerifyNull() const |
There was a problem hiding this comment.
Not relevant for this PR but I'd like a comment on this method normally, overall comments are fairly limited in this new code
There was a problem hiding this comment.
I'll create a PR in the next days to include more documentation in general, hope that helps understanding all the new code.
| return state.DoS(100, false, REJECT_INVALID, "bad-qc-height"); | ||
| } | ||
|
|
||
| if (!mapBlockIndex.count(qcTx.commitment.quorumHash)) { |
There was a problem hiding this comment.
I prefer it to be explicit, I think it is easier to read and it conveys the intent better
| if (!mapBlockIndex.count(qcTx.commitment.quorumHash)) { | |
| if (mapBlockIndex.count(qcTx.commitment.quorumHash) != 0) { |
| if (!Params().GetConsensus().llmqs.count((Consensus::LLMQType)qcTx.commitment.llmqType)) { | ||
| return state.DoS(100, false, REJECT_INVALID, "bad-qc-type"); | ||
| } | ||
| const auto& params = Params().GetConsensus().llmqs.at((Consensus::LLMQType)qcTx.commitment.llmqType); |
There was a problem hiding this comment.
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 ¯\(ツ)/¯
There was a problem hiding this comment.
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)
| @@ -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); | |||
There was a problem hiding this comment.
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 ¯\(ツ)/¯)
There was a problem hiding this comment.
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.
| @@ -79,6 +79,7 @@ class CFinalCommitment | |||
| return false; | |||
| } | |||
| if (quorumPublicKey.IsValid() || | |||
| !quorumVvecHash.IsNull() || | |||
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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).
…andling of quorumVvecHash (dashpay#2501) * Allow to skip sig verification for CFinalCommitment::Verify * Add CFinalCommitmentTxPayload and CheckLLMQCommitment and use it As described in dashpay/dips#31 (see discussion). * Properly ban nodes for invalid commitments
See discussion in dashpay/dips#31