Skip to content

refactor: improved initialization of members of LLMQContext and related changes #5150

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

Merged
merged 2 commits into from
Jul 30, 2023
Merged

refactor: improved initialization of members of LLMQContext and related changes #5150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e3f077
refactor: RAII initialization for members of llmq::context
knst Jun 5, 2023
81ed8ec
refactor: constructors of BlockAssembler using LLMQContext
knst Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 59 additions & 82 deletions src/llmq/context.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,108 +20,85 @@
#include <masternode/sync.h>

LLMQContext::LLMQContext(CEvoDB& evo_db, CTxMemPool& mempool, CConnman& connman, CSporkManager& sporkman,
const std::unique_ptr<PeerManager>& peerman, bool unit_tests, bool wipe) {
Create(evo_db, mempool, connman, sporkman, peerman, unit_tests, wipe);

/* Context aliases to globals used by the LLMQ system */
quorum_block_processor = llmq::quorumBlockProcessor.get();
qman = llmq::quorumManager.get();
clhandler = llmq::chainLocksHandler.get();
isman = llmq::quorumInstantSendManager.get();
}

LLMQContext::~LLMQContext() {
isman = nullptr;
clhandler = nullptr;
qman = nullptr;
quorum_block_processor = nullptr;

Destroy();
}

void LLMQContext::Create(CEvoDB& evo_db, CTxMemPool& mempool, CConnman& connman, CSporkManager& sporkman,
const std::unique_ptr<PeerManager>& peerman, bool unit_tests, bool wipe) {
bls_worker = std::make_shared<CBLSWorker>();

dkg_debugman = std::make_unique<llmq::CDKGDebugManager>();
llmq::quorumBlockProcessor = std::make_unique<llmq::CQuorumBlockProcessor>(evo_db, connman, peerman);
qdkgsman = std::make_unique<llmq::CDKGSessionManager>(connman, *bls_worker, *dkg_debugman, *llmq::quorumBlockProcessor, sporkman, peerman, unit_tests, wipe);
llmq::quorumManager = std::make_unique<llmq::CQuorumManager>(evo_db, connman, *bls_worker, *llmq::quorumBlockProcessor, *qdkgsman, ::masternodeSync, peerman);
sigman = std::make_unique<llmq::CSigningManager>(connman, *llmq::quorumManager, peerman, unit_tests, wipe);
shareman = std::make_unique<llmq::CSigSharesManager>(connman, *llmq::quorumManager, *sigman, peerman);
llmq::chainLocksHandler = std::make_unique<llmq::CChainLocksHandler>(mempool, connman, sporkman, *sigman, *shareman, *llmq::quorumManager, *::masternodeSync, peerman);
llmq::quorumInstantSendManager = std::make_unique<llmq::CInstantSendManager>(mempool, connman, sporkman, *llmq::quorumManager, *sigman, *shareman, *llmq::chainLocksHandler, *::masternodeSync, peerman, unit_tests, wipe);

const std::unique_ptr<PeerManager>& peerman, bool unit_tests, bool wipe) :
bls_worker{std::make_shared<CBLSWorker>()},
dkg_debugman{std::make_unique<llmq::CDKGDebugManager>()},
quorum_block_processor{[&]() -> llmq::CQuorumBlockProcessor* const {
assert(llmq::quorumBlockProcessor == nullptr);
llmq::quorumBlockProcessor = std::make_unique<llmq::CQuorumBlockProcessor>(evo_db, connman, peerman);
return llmq::quorumBlockProcessor.get();
}()},
qdkgsman{std::make_unique<llmq::CDKGSessionManager>(connman, *bls_worker, *dkg_debugman, *quorum_block_processor, sporkman, peerman, unit_tests, wipe)},
qman{[&]() -> llmq::CQuorumManager* const {
assert(llmq::quorumManager == nullptr);
llmq::quorumManager = std::make_unique<llmq::CQuorumManager>(evo_db, connman, *bls_worker, *quorum_block_processor, *qdkgsman, ::masternodeSync, peerman);
return llmq::quorumManager.get();
}()},
sigman{std::make_unique<llmq::CSigningManager>(connman, *llmq::quorumManager, peerman, unit_tests, wipe)},
shareman{std::make_unique<llmq::CSigSharesManager>(connman, *llmq::quorumManager, *sigman, peerman)},
clhandler{[&]() -> llmq::CChainLocksHandler* const {
assert(llmq::chainLocksHandler == nullptr);
llmq::chainLocksHandler = std::make_unique<llmq::CChainLocksHandler>(mempool, connman, sporkman, *sigman, *shareman, *llmq::quorumManager, *::masternodeSync, peerman);
return llmq::chainLocksHandler.get();
}()},
isman{[&]() -> llmq::CInstantSendManager* const {
assert(llmq::quorumInstantSendManager == nullptr);
llmq::quorumInstantSendManager = std::make_unique<llmq::CInstantSendManager>(mempool, connman, sporkman, *llmq::quorumManager, *sigman, *shareman, *llmq::chainLocksHandler, *::masternodeSync, peerman, unit_tests, wipe);
return llmq::quorumInstantSendManager.get();
}()}
{
// NOTE: we use this only to wipe the old db, do NOT use it for anything else
// TODO: remove it in some future version
auto llmqDbTmp = std::make_unique<CDBWrapper>(unit_tests ? "" : (GetDataDir() / "llmq"), 1 << 20, unit_tests, true);
Copy link
Collaborator

@kwvg kwvg Jun 9, 2023

Choose a reason for hiding this comment

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

@UdjinM6, can we drop this?

Copy link

@UdjinM6 UdjinM6 Jun 12, 2023

Choose a reason for hiding this comment

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

this was added ~2 years ago in v18.0 so, yeah, I guess it's safe to drop it in v20

}

void LLMQContext::Destroy() {
LLMQContext::~LLMQContext() {
// LLMQContext doesn't own these objects, but still need to care of them for consistancy:
llmq::quorumInstantSendManager.reset();
llmq::chainLocksHandler.reset();
shareman.reset();
sigman.reset();
llmq::quorumManager.reset();
qdkgsman.reset();
llmq::quorumBlockProcessor.reset();
dkg_debugman.reset();
bls_worker.reset();
{
LOCK(llmq::cs_llmq_vbc);
llmq::llmq_versionbitscache.Clear();
}
}

void LLMQContext::Interrupt() {
if (shareman != nullptr) {
shareman->InterruptWorkerThread();
}
if (llmq::quorumInstantSendManager != nullptr) {
llmq::quorumInstantSendManager->InterruptWorkerThread();
}
shareman->InterruptWorkerThread();

assert(isman == llmq::quorumInstantSendManager.get());
llmq::quorumInstantSendManager->InterruptWorkerThread();
}

void LLMQContext::Start() {
if (bls_worker != nullptr) {
bls_worker->Start();
}
if (qdkgsman != nullptr) {
qdkgsman->StartThreads();
}
if (llmq::quorumManager != nullptr) {
llmq::quorumManager->Start();
}
if (shareman != nullptr) {
shareman->RegisterAsRecoveredSigsListener();
shareman->StartWorkerThread();
}
if (llmq::chainLocksHandler != nullptr) {
llmq::chainLocksHandler->Start();
}
if (llmq::quorumInstantSendManager != nullptr) {
llmq::quorumInstantSendManager->Start();
}
assert(quorum_block_processor == llmq::quorumBlockProcessor.get());
assert(qman == llmq::quorumManager.get());
assert(clhandler == llmq::chainLocksHandler.get());
assert(isman == llmq::quorumInstantSendManager.get());
Comment on lines +75 to +78
Copy link
Member

@PastaPastaPasta PastaPastaPasta Jun 22, 2023

Choose a reason for hiding this comment

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

these could still be null which could cause crashes below.. Are we sure these are always non-null? It seems yes as they are assigned in the constructor.

Copy link
Collaborator Author

@knst knst Jun 22, 2023

Choose a reason for hiding this comment

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

yes, if this exists than means all members are correctly initialized and not nullptr.


bls_worker->Start();
qdkgsman->StartThreads();
qman->Start();
shareman->RegisterAsRecoveredSigsListener();
shareman->StartWorkerThread();

llmq::chainLocksHandler->Start();
llmq::quorumInstantSendManager->Start();
}

void LLMQContext::Stop() {
if (llmq::quorumInstantSendManager != nullptr) {
llmq::quorumInstantSendManager->Stop();
}
if (llmq::chainLocksHandler != nullptr) {
llmq::chainLocksHandler->Stop();
}
if (shareman != nullptr) {
shareman->StopWorkerThread();
shareman->UnregisterAsRecoveredSigsListener();
}
if (llmq::quorumManager != nullptr) {
llmq::quorumManager->Stop();
}
if (qdkgsman != nullptr) {
qdkgsman->StopThreads();
}
if (bls_worker != nullptr) {
bls_worker->Stop();
}
assert(quorum_block_processor == llmq::quorumBlockProcessor.get());
assert(qman == llmq::quorumManager.get());
assert(clhandler == llmq::chainLocksHandler.get());
assert(isman == llmq::quorumInstantSendManager.get());

llmq::quorumInstantSendManager->Stop();
llmq::chainLocksHandler->Stop();

shareman->StopWorkerThread();
shareman->UnregisterAsRecoveredSigsListener();
qman->Stop();
qdkgsman->StopThreads();
bls_worker->Stop();
}
34 changes: 21 additions & 13 deletions src/llmq/context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,27 +27,35 @@ class CInstantSendManager;
}

struct LLMQContext {
LLMQContext() = delete;
LLMQContext(const LLMQContext&) = delete;
LLMQContext(CEvoDB& evo_db, CTxMemPool& mempool, CConnman& connman, CSporkManager& sporkman,
const std::unique_ptr<PeerManager>& peerman, bool unit_tests, bool wipe);
~LLMQContext();

void Create(CEvoDB& evo_db, CTxMemPool& mempool, CConnman& connman, CSporkManager& sporkman,
const std::unique_ptr<PeerManager>& peerman, bool unit_tests, bool wipe);
void Destroy();
void Interrupt();
void Start();
void Stop();

std::shared_ptr<CBLSWorker> bls_worker;
std::unique_ptr<llmq::CDKGDebugManager> dkg_debugman;
std::unique_ptr<llmq::CDKGSessionManager> qdkgsman;
std::unique_ptr<llmq::CSigSharesManager> shareman;
std::unique_ptr<llmq::CSigningManager> sigman;

llmq::CQuorumBlockProcessor* quorum_block_processor{nullptr};
llmq::CQuorumManager* qman{nullptr};
llmq::CChainLocksHandler* clhandler{nullptr};
llmq::CInstantSendManager* isman{nullptr};
/** Guaranteed if LLMQContext is initialized then all members are valid too
*
* Please note, that members here should not be re-ordered, because initialization
* some of them requires other member initialized.
* For example, constructor `quorumManager` requires `bls_worker`.
*
* Some objects are still global variables and their de-globalization is not trivial
* at this point. LLMQContext keeps just a pointer to them and doesn't own these objects,
* but it still guarantees that objects are created and valid
*/
const std::shared_ptr<CBLSWorker> bls_worker;
const std::unique_ptr<llmq::CDKGDebugManager> dkg_debugman;
llmq::CQuorumBlockProcessor* const quorum_block_processor;
const std::unique_ptr<llmq::CDKGSessionManager> qdkgsman;
llmq::CQuorumManager* const qman;
const std::unique_ptr<llmq::CSigningManager> sigman;
const std::unique_ptr<llmq::CSigSharesManager> shareman;
llmq::CChainLocksHandler* const clhandler;
llmq::CInstantSendManager* const isman;
};

#endif // BITCOIN_LLMQ_CONTEXT_H
15 changes: 7 additions & 8 deletions src/miner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include <governance/governance.h>
#include <llmq/blockprocessor.h>
#include <llmq/chainlocks.h>
#include <llmq/context.h>
#include <llmq/instantsend.h>
#include <llmq/utils.h>
#include <masternode/payments.h>
Expand Down Expand Up @@ -57,16 +58,15 @@ BlockAssembler::Options::Options() {
}

BlockAssembler::BlockAssembler(const CSporkManager& sporkManager, CGovernanceManager& governanceManager,
const llmq::CQuorumBlockProcessor& quorumBlockProcessor, llmq::CChainLocksHandler& clhandler,
llmq::CInstantSendManager& isman, CEvoDB& evoDb, CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params, const Options& options) :
LLMQContext& llmq_ctx, CEvoDB& evoDb, CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params, const Options& options) :
chainparams(params),
m_mempool(mempool),
m_chainstate(chainstate),
spork_manager(sporkManager),
governance_manager(governanceManager),
quorum_block_processor(quorumBlockProcessor),
m_clhandler(clhandler),
m_isman(isman),
quorum_block_processor(*llmq_ctx.quorum_block_processor),
m_clhandler(*llmq_ctx.clhandler),
m_isman(*llmq_ctx.isman),
m_evoDb(evoDb)
{
blockMinFeeRate = options.blockMinFeeRate;
Expand All @@ -92,9 +92,8 @@ static BlockAssembler::Options DefaultOptions()
}

BlockAssembler::BlockAssembler(const CSporkManager& sporkManager, CGovernanceManager& governanceManager,
const llmq::CQuorumBlockProcessor& quorumBlockProcessor, llmq::CChainLocksHandler& clhandler,
llmq::CInstantSendManager& isman, CEvoDB& evoDb, CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params)
: BlockAssembler(sporkManager, governanceManager, quorumBlockProcessor, clhandler, isman, evoDb, chainstate, mempool, params, DefaultOptions()) {}
LLMQContext& llmq_ctx, CEvoDB& evoDb, CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params)
: BlockAssembler(sporkManager, governanceManager, llmq_ctx, evoDb, chainstate, mempool, params, DefaultOptions()) {}

void BlockAssembler::resetBlock()
{
Expand Down
7 changes: 3 additions & 4 deletions src/miner.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class CCreditPoolDiff;
class CGovernanceManager;
class CScript;
class CSporkManager;
struct LLMQContext;

namespace Consensus { struct Params; };
namespace llmq {
Expand Down Expand Up @@ -172,11 +173,9 @@ class BlockAssembler
};

explicit BlockAssembler(const CSporkManager& sporkManager, CGovernanceManager& governanceManager,
const llmq::CQuorumBlockProcessor& quorumBlockProcessor, llmq::CChainLocksHandler& clhandler,
llmq::CInstantSendManager& isman, CEvoDB& evoDb, CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params);
LLMQContext& llmq_ctx, CEvoDB& evoDb, CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params);
explicit BlockAssembler(const CSporkManager& sporkManager, CGovernanceManager& governanceManager,
const llmq::CQuorumBlockProcessor& quorumBlockProcessor, llmq::CChainLocksHandler& clhandler,
llmq::CInstantSendManager& isman, CEvoDB& evoDb, CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params, const Options& options);
LLMQContext& llmq_ctx, CEvoDB& evoDb, CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params, const Options& options);

/** Construct a new block template with coinbase to scriptPubKeyIn */
std::unique_ptr<CBlockTemplate> CreateNewBlock(const CScript& scriptPubKeyIn);
Expand Down
2 changes: 1 addition & 1 deletion src/rpc/evo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1471,7 +1471,7 @@ static UniValue protx_diff(const JSONRPCRequest& request, const ChainstateManage

CSimplifiedMNListDiff mnListDiff;
std::string strError;
LLMQContext& llmq_ctx = EnsureAnyLLMQContext(request.context);
const LLMQContext& llmq_ctx = EnsureAnyLLMQContext(request.context);

if (!BuildSimplifiedMNListDiff(baseBlockHash, blockHash, mnListDiff, *llmq_ctx.quorum_block_processor, strError, extended)) {
throw std::runtime_error(strError);
Expand Down
13 changes: 6 additions & 7 deletions src/rpc/mining.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,7 @@ static bool GenerateBlock(ChainstateManager& chainman, CBlock& block, uint64_t&
}

static UniValue generateBlocks(ChainstateManager& chainman, const CTxMemPool& mempool, CEvoDB& evodb,
llmq::CQuorumBlockProcessor& quorum_block_processor, llmq::CChainLocksHandler& clhandler,
llmq::CInstantSendManager& isman, const CScript& coinbase_script, int nGenerate, uint64_t nMaxTries)
LLMQContext& llmq_ctx, const CScript& coinbase_script, int nGenerate, uint64_t nMaxTries)
{
int nHeightEnd = 0;
int nHeight = 0;
Expand All @@ -165,7 +164,7 @@ static UniValue generateBlocks(ChainstateManager& chainman, const CTxMemPool& me
UniValue blockHashes(UniValue::VARR);
while (nHeight < nHeightEnd && !ShutdownRequested())
{
std::unique_ptr<CBlockTemplate> pblocktemplate(BlockAssembler(*sporkManager, *governance, quorum_block_processor, clhandler, isman, evodb, chainman.ActiveChainstate(), mempool, Params()).CreateNewBlock(coinbase_script));
std::unique_ptr<CBlockTemplate> pblocktemplate(BlockAssembler(*sporkManager, *governance, llmq_ctx, evodb, chainman.ActiveChainstate(), mempool, Params()).CreateNewBlock(coinbase_script));
if (!pblocktemplate.get())
throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block");
CBlock *pblock = &pblocktemplate->block;
Expand Down Expand Up @@ -252,7 +251,7 @@ static UniValue generatetodescriptor(const JSONRPCRequest& request)
ChainstateManager& chainman = EnsureChainman(node);
LLMQContext& llmq_ctx = EnsureLLMQContext(node);

return generateBlocks(chainman, mempool, *node.evodb, *llmq_ctx.quorum_block_processor, *llmq_ctx.clhandler, *llmq_ctx.isman, coinbase_script, num_blocks, max_tries);
return generateBlocks(chainman, mempool, *node.evodb, llmq_ctx, coinbase_script, num_blocks, max_tries);
}

static UniValue generatetoaddress(const JSONRPCRequest& request)
Expand Down Expand Up @@ -291,7 +290,7 @@ static UniValue generatetoaddress(const JSONRPCRequest& request)

CScript coinbase_script = GetScriptForDestination(destination);

return generateBlocks(chainman, mempool, *node.evodb, *llmq_ctx.quorum_block_processor, *llmq_ctx.clhandler, *llmq_ctx.isman, coinbase_script, num_blocks, max_tries);
return generateBlocks(chainman, mempool, *node.evodb, llmq_ctx, coinbase_script, num_blocks, max_tries);
}

static UniValue generateblock(const JSONRPCRequest& request)
Expand Down Expand Up @@ -371,7 +370,7 @@ static UniValue generateblock(const JSONRPCRequest& request)
LOCK(cs_main);

CTxMemPool empty_mempool;
std::unique_ptr<CBlockTemplate> blocktemplate(BlockAssembler(*sporkManager, *governance, *llmq_ctx.quorum_block_processor, *llmq_ctx.clhandler, *llmq_ctx.isman, *node.evodb, active_chainstate, empty_mempool, chainparams).CreateNewBlock(coinbase_script));
std::unique_ptr<CBlockTemplate> blocktemplate(BlockAssembler(*sporkManager, *governance, llmq_ctx, *node.evodb, active_chainstate, empty_mempool, chainparams).CreateNewBlock(coinbase_script));
if (!blocktemplate) {
throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block");
}
Expand Down Expand Up @@ -787,7 +786,7 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
// Create new block
CScript scriptDummy = CScript() << OP_TRUE;
LLMQContext& llmq_ctx = EnsureAnyLLMQContext(request.context);
pblocktemplate = BlockAssembler(*sporkManager, *governance, *llmq_ctx.quorum_block_processor, *llmq_ctx.clhandler, *llmq_ctx.isman, *node.evodb, active_chainstate, mempool, Params()).CreateNewBlock(scriptDummy);
pblocktemplate = BlockAssembler(*sporkManager, *governance, llmq_ctx, *node.evodb, active_chainstate, mempool, Params()).CreateNewBlock(scriptDummy);
if (!pblocktemplate)
throw JSONRPCError(RPC_OUT_OF_MEMORY, "Out of memory");

Expand Down
Loading