refactor/feat: Refactor and add safety belts in llmq utils#5378
refactor/feat: Refactor and add safety belts in llmq utils#5378UdjinM6 merged 2 commits intodashpay:developfrom
Conversation
…to be more precise which block index we actually expect
UdjinM6
changed the title
|
|
||
| uint256 GetHashModifier(const Consensus::LLMQParams& llmqParams, const CBlockIndex* pCycleQuorumBaseBlockIndex) | ||
| { | ||
| ASSERT_IF_DEBUG(pCycleQuorumBaseBlockIndex->nHeight % llmqParams.dkgInterval == 0); |
There was a problem hiding this comment.
I kinda wonder if we should create an "ASSUME" macro or something similar which if debug is enabled with act as an assert, but if not debug would print to log if the assumption was false? Idk, probably not the most useful
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
light-utACK for squash merge... I do think it'd be better to have some way to tell which condition failed
it fails like that (I broke |
| void PreComputeQuorumMembers(const CBlockIndex* pQuorumBaseBlockIndex, bool reset_cache = false); | ||
| uint256 GetHashModifier(const Consensus::LLMQParams& llmqParams, const CBlockIndex* pQuorumBaseBlockIndex); | ||
| void PreComputeQuorumMembers(const CBlockIndex* pindex, bool reset_cache = false); | ||
| uint256 GetHashModifier(const Consensus::LLMQParams& llmqParams, const CBlockIndex* pCycleQuorumBaseBlockIndex); |
There was a problem hiding this comment.
My only "objection" is here. GetHashModifier is called for both non-rotated and rotated quorums, hence "cycle" is more adapted to rotation.
There was a problem hiding this comment.
for non-rotation quorums pCycleQuorumBaseBlockIndex == pQuorumBaseBlockIndex and since using pQuorumBaseBlockIndex for rotation ones is wrong using pCycleQuorumBaseBlockIndex for both is a good compromise imo
3 participants
Issue being fixed or feature implemented
We use
pQuorumBaseBlockIndexname when we shouldn't and we don't check that quorum types and block indexes provided as input params in llmq utils satisfy our requirements. This is kind of ok-ish as long as we use these functions appropriately but it's better to make things clearer and to have actual checks imo.noticed this while reviewing #5366
What was done?
Rename
pQuorumBaseBlockIndextopCycleQuorumBaseBlockIndex/pindexin a few places. Check that quorum types and block indexes have expected values.How Has This Been Tested?
run tests locally
Breaking Changes
n/a
Checklist: