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.

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

refactor(llmq): separate llmq/utils into llmq/utils and llmq/complex_… #4798

Closed

Conversation

PastaPastaPasta
Copy link
Member

…utils

llmq/utils should remain very slim including only trivial headers, while llmq/complex_utils for the time being may continue to include more complex includes such as

#include <llmq/quorums.h>
#include <llmq/commitment.h>
#include <llmq/snapshot.h>
#include <evo/deterministicmns.h>
#include <masternode/meta.h>

This helps us break some circular dependancies and makes llmq/utils a nice simple neighborhood to live. This also includes substancial include refactoring as well as moving a function into its own util files

note that the circular depends linter had a lot of exclusions added, but I think that's an artifact of the fact that c(omplex) is earlier in the alphabet than u(tils)

@UdjinM6
Copy link

UdjinM6 commented Apr 25, 2022

linter complains

@github-actions
Copy link

This pull request has conflicts, please rebase.


std::string BitsToHexStr(const std::vector<bool>& vBits);

#endif // BITCOIN_UTIL_GETUNIQUEPATH_H
Copy link

Choose a reason for hiding this comment

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

Suggested change
#endif // BITCOIN_UTIL_GETUNIQUEPATH_H
#endif // BITCOIN_UTIL_GETUNIQUEPATH_H

@@ -109,7 +124,7 @@ for CIRC in $(cd src && ../contrib/devtools/circular-dependencies.py {*,*/*,*/*/
fi
done
if [[ ${IS_EXPECTED_CIRC} == 0 ]]; then
echo "A new circular dependency in the form of \"${CIRC}\" appears to have been introduced."
echo "\"${CIRC}\""
Copy link

Choose a reason for hiding this comment

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

Suggested change
echo "\"${CIRC}\""
echo "A new circular dependency in the form of \"${CIRC}\" appears to have been introduced."

#include <string>
#include <vector>

std::string BitsToHexStr(const std::vector<bool>& vBits)
Copy link

Choose a reason for hiding this comment

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

why not in util/strencodings.*?

Comment on lines +164 to +181
//void CFinalCommitment::ToJson(UniValue& obj) const
//{
// obj.setObject();
// obj.pushKV("version", (int)nVersion);
// obj.pushKV("llmqType", (int)llmqType);
// obj.pushKV("quorumHash", quorumHash.ToString());
// obj.pushKV("quorumIndex", quorumIndex);
// obj.pushKV("signersCount", CountSigners());
// obj.pushKV("signers", CLLMQUtils::ToHexStr(signers));
// obj.pushKV("validMembersCount", CountValidMembers());
// obj.pushKV("validMembers", CLLMQUtils::ToHexStr(validMembers));
// obj.pushKV("quorumPublicKey", quorumPublicKey.ToString());
// obj.pushKV("quorumVvecHash", quorumVvecHash.ToString());
// obj.pushKV("quorumSig", quorumSig.ToString());
// obj.pushKV("membersSig", membersSig.ToString());
//}
//
Copy link

Choose a reason for hiding this comment

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

?

Comment on lines +238 to +249
//void CFinalCommitmentTxPayload::ToJson(UniValue& obj) const
//{
// obj.setObject();
// obj.pushKV("version", int(nVersion));
// obj.pushKV("height", int(nHeight));
//
// UniValue qcObj;
// commitment.ToJson(qcObj);
// obj.pushKV("commitment", qcObj);
//}

Copy link

Choose a reason for hiding this comment

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

?

Comment on lines +135 to +136
// void ToJson(UniValue& obj) const;

Copy link

Choose a reason for hiding this comment

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

?

@github-actions
Copy link

This pull request has conflicts, please rebase.

Copy link
Collaborator

@ogabrielides ogabrielides left a comment

Choose a reason for hiding this comment

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

@PastaPastaPasta Please apply those changes (I have no access to push)

diff --git a/src/llmq/commitment.cpp b/src/llmq/commitment.cpp
index 6b436dd2f9..8e2ca96490 100644
--- a/src/llmq/commitment.cpp
+++ b/src/llmq/commitment.cpp
@@ -161,23 +161,6 @@ bool CFinalCommitment::VerifySizes(const Consensus::LLMQParams& params) const
     return true;
 }
 
-//void CFinalCommitment::ToJson(UniValue& obj) const
-//{
-//    obj.setObject();
-//    obj.pushKV("version", (int)nVersion);
-//    obj.pushKV("llmqType", (int)llmqType);
-//    obj.pushKV("quorumHash", quorumHash.ToString());
-//    obj.pushKV("quorumIndex", quorumIndex);
-//    obj.pushKV("signersCount", CountSigners());
-//    obj.pushKV("signers", CLLMQUtils::ToHexStr(signers));
-//    obj.pushKV("validMembersCount", CountValidMembers());
-//    obj.pushKV("validMembers", CLLMQUtils::ToHexStr(validMembers));
-//    obj.pushKV("quorumPublicKey", quorumPublicKey.ToString());
-//    obj.pushKV("quorumVvecHash", quorumVvecHash.ToString());
-//    obj.pushKV("quorumSig", quorumSig.ToString());
-//    obj.pushKV("membersSig", membersSig.ToString());
-//}
-//
 bool CheckLLMQCommitment(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state)
 {
     CFinalCommitmentTxPayload qcTx;
@@ -235,15 +218,5 @@ bool CheckLLMQCommitment(const CTransaction& tx, const CBlockIndex* pindexPrev,
     return true;
 }
 
-//void CFinalCommitmentTxPayload::ToJson(UniValue& obj) const
-//{
-//    obj.setObject();
-//    obj.pushKV("version", int(nVersion));
-//    obj.pushKV("height", int(nHeight));
-//
-//    UniValue qcObj;
-//    commitment.ToJson(qcObj);
-//    obj.pushKV("commitment", qcObj);
-//}
 
 } // namespace llmq
diff --git a/src/llmq/commitment.h b/src/llmq/commitment.h
index 9bbb03c8fd..24795bd1e0 100644
--- a/src/llmq/commitment.h
+++ b/src/llmq/commitment.h
@@ -132,7 +132,6 @@ public:
     CFinalCommitment commitment;
 
 public:
-//    void ToJson(UniValue& obj) const;
 
     SERIALIZE_METHODS(CFinalCommitmentTxPayload, obj)
     {
diff --git a/src/llmq/instantsend.cpp b/src/llmq/instantsend.cpp
index c6827f4c20..b05d99395a 100644
--- a/src/llmq/instantsend.cpp
+++ b/src/llmq/instantsend.cpp
@@ -8,6 +8,7 @@
 #include <llmq/quorums.h>
 #include <llmq/utils.h>
 #include <llmq/commitment.h>
+#include <llmq/complex_utils.h>
 
 #include <bls/bls_batchverifier.h>
 #include <chainparams.h>
@@ -508,7 +509,7 @@ void CInstantSendManager::ProcessTx(const CTransaction& tx, bool fRetroactive, c
     // block after we retroactively locked all transactions.
     if (!IsInstantSendMempoolSigningEnabled() && !fRetroactive) return;
 
-    if (!TrySignInputLocks(tx, fRetroactive, CLLMQUtils::GetInstantSendLLMQType(WITH_LOCK(cs_main, return ::ChainActive().Tip())), params)) {
+    if (!TrySignInputLocks(tx, fRetroactive, CLLMQComplexUtils::GetInstantSendLLMQType(WITH_LOCK(cs_main, return ::ChainActive().Tip())), params)) {
         return;
     }
 
@@ -682,7 +683,7 @@ void CInstantSendManager::HandleNewInputLockRecoveredSig(const CRecoveredSig& re
 
 void CInstantSendManager::TrySignInstantSendLock(const CTransaction& tx)
 {
-    const auto llmqType = CLLMQUtils::GetInstantSendLLMQType(WITH_LOCK(cs_main, return ::ChainActive().Tip()));
+    const auto llmqType = CLLMQComplexUtils::GetInstantSendLLMQType(WITH_LOCK(cs_main, return ::ChainActive().Tip()));
 
     for (auto& in : tx.vin) {
         auto id = ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, in.prevout));
@@ -851,7 +852,7 @@ bool CInstantSendManager::PreVerifyInstantSendLock(const llmq::CInstantSendLock&
 bool CInstantSendManager::ProcessPendingInstantSendLocks()
 {
     const CBlockIndex* pBlockIndexTip = WITH_LOCK(cs_main, return ::ChainActive().Tip());
-    if (pBlockIndexTip && CLLMQUtils::GetInstantSendLLMQType(pBlockIndexTip) == Params().GetConsensus().llmqTypeDIP0024InstantSend) {
+    if (pBlockIndexTip && CLLMQComplexUtils::GetInstantSendLLMQType(pBlockIndexTip) == Params().GetConsensus().llmqTypeDIP0024InstantSend) {
         // Don't short circuit. Try to process deterministic and not deterministic islocks
         return ProcessPendingInstantSendLocks(true) & ProcessPendingInstantSendLocks(false);
     } else {
diff --git a/src/llmq/snapshot.cpp b/src/llmq/snapshot.cpp
index 7ab267b429..864bacbc8e 100644
--- a/src/llmq/snapshot.cpp
+++ b/src/llmq/snapshot.cpp
@@ -11,6 +11,7 @@
 #include <llmq/blockprocessor.h>
 #include <llmq/commitment.h>
 #include <llmq/utils.h>
+#include <llmq/complex_utils.h>
 
 #include <base58.h>
 #include <chainparams.h>
@@ -162,7 +163,7 @@ bool BuildQuorumRotationInfo(const CGetQuorumRotationInfo& request, CQuorumRotat
     }
 
     //Quorum rotation is enabled only for InstantSend atm.
-    Consensus::LLMQType llmqType = CLLMQUtils::GetInstantSendLLMQType(blockIndex);
+    Consensus::LLMQType llmqType = CLLMQComplexUtils::GetInstantSendLLMQType(blockIndex);
 
     // Since the returned quorums are in reversed order, the most recent one is at index 0
     const Consensus::LLMQParams& llmqParams = GetLLMQParams(llmqType);
diff --git a/src/rpc/rpcquorums.cpp b/src/rpc/rpcquorums.cpp
index ebe9d0b8a1..e2c1b90a9b 100644
--- a/src/rpc/rpcquorums.cpp
+++ b/src/rpc/rpcquorums.cpp
@@ -870,7 +870,7 @@ static UniValue verifyislock(const JSONRPCRequest& request)
             pBlockIndex = ::ChainActive()[signHeight];
         }
     }
-    auto llmqType = llmq::CLLMQUtils::GetInstantSendLLMQType(pBlockIndex);
+    auto llmqType = llmq::CLLMQComplexUtils::GetInstantSendLLMQType(pBlockIndex);
     // First check against the current active set, if it fails check against the last active set
     int signOffset{llmq::GetLLMQParams(llmqType).dkgInterval};
     return llmq::quorumSigningManager->VerifyRecoveredSig(llmqType, signHeight, id, txid, sig, 0) ||
-- 
2.32.0 (Apple Git-132)

…utils

llmq/utils should remain very slim including only trivial headers, while llmq/complex_utils for the time being may continue to include more complex includes such as. This helps us break some circular dependancies and makes llmq/utils a nice simple neighborhood to live. This also includes substancial include refactoring as well as moving a function into its own util files
```
```
@github-actions
Copy link

This pull request has conflicts, please rebase.

PastaPastaPasta pushed a commit that referenced this pull request Jan 18, 2024
)

## Issue being fixed or feature implemented
`llmq/utils` has simple util code that used all over code base and also
have too heavy code for calculation quorums such as:
`GetAllQuorumMembers`, `EnsureQuorumConnections` and other.

These helpers for calculation quorums are used only by
evo/deterministicmns, evo/simplifiedmns and llmq/* modules, but
llmq/utils is included in many other modules for various trivial
helpers.



## What was done?
Prior work:
 - #5753
 - #5486
 See also #4798

This PR remove all non-quorum calculation code from llmq/utils.
Eventually it happens that easier to take everything out rather than
move Quorum Calculation to new place atm:
- new module llmq/options have a code related to various params, command
line options, spork-related etc
- llmq/utils is not included in various files which do not use any
llmq/utils code
 - helper `BuildCommitmentHash` goes to llmq/commitment
 - helper `BuildSignHash` goes to llmq/signing
- helper `GetLLMQParam` inlined since it's trivial (it has not been
trivial when introduced ages ago)
- removed dependency of `IsQuorumEnabled` on CQuorumManager which means
`quorumManager` deglobalization is done for 90%


## How Has This Been Tested?
 - Run unit functional tests
- updated circular dependencies
`test/lint/lint-circular-dependencies.sh`
- check that llmq/utils is not included without needs to calculate
Quorums Members
```
$ grep -r include src/ 2> /dev/null | grep -v .Po: | grep -vE 'llmq/utils.(h|cpp)': | grep llmq/utils  
src/evo/mnauth.cpp:#include <llmq/utils.h>
src/evo/deterministicmns.cpp:#include <llmq/utils.h>
src/llmq/quorums.cpp:#include <llmq/utils.h>
src/llmq/blockprocessor.cpp:#include <llmq/utils.h>
src/llmq/commitment.cpp:#include <llmq/utils.h>
src/llmq/debug.cpp:#include <llmq/utils.h>
src/llmq/dkgsessionhandler.cpp:#include <llmq/utils.h>
src/llmq/dkgsession.cpp:#include <llmq/utils.h>
src/llmq/dkgsessionmgr.cpp:#include <llmq/utils.h>
src/rpc/quorums.cpp:#include <llmq/utils.h>
```


## Breaking Changes
N/A

## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
@knst
Copy link
Collaborator

knst commented Jan 18, 2024

superseeded by #5753 and #5790
llmq/utils.h has now only quorum calculations; all "light" things are moved out such as llmq/options.h

@knst knst closed this Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants