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

Disabling wallet should disable staking as well #752

Open
mimirmim opened this issue Feb 26, 2020 · 1 comment
Open

Disabling wallet should disable staking as well #752

mimirmim opened this issue Feb 26, 2020 · 1 comment
Labels
Component: Core App Related to the application itself. Component: Wallet Relating to keystore, tx creation, and balance tracking Tag: PoS Related to Proof of Stake Tag: PoW Related to Proof of Work consensus

Comments

@mimirmim
Copy link
Contributor

mimirmim commented Feb 26, 2020

Problem

The problem is that in the BlockAssembler::CreateNewBlock, there is a bunch of messy code that just returns null to prevent you from staking if you have a wallet disabled. The issue is that this is just all redundant and this code and thread will keep running returning a nullptr.

miner.cpp @ BlockAssembler::CreateNewBlock

#ifdef ENABLE_WALLET
    //Need wallet if this is for proof of stake,
    std::shared_ptr<CWallet> pwalletMain = nullptr;
#endif
    if (fProofOfStake) {
#ifdef ENABLE_WALLET
        if (!gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET)) {
            pwalletMain = GetMainWallet();
        }
        if (!pwalletMain) {
#endif
            error("Failing to get the Main Wallet for CreateNewBlock with Proof of Stake\n");
            return nullptr;
#ifdef ENABLE_WALLET
        }
#endif

Expected

What I had expected is that disabling the wallet should prevent staking without needing to set an option. I'm unsure of the performance hit, if any, of this pointless code execution on the staking thread when it should never get this far.

Behaviour

The actual behaviour is that instead the code gets as far as spinning up a new Staking thread that does nothing but return a nullptr

Proposed Fix

A simple fix would be just to prevent staking when you disable the wallet even IF staking is enabled in the config.

@CaveSpectre11
Copy link
Collaborator

Two things here. First is we need to do is clean up the -disablewallet checks. There's upwards of 60 checks on the flag through the code; and that's going to roll through to improvements all around.

BitcoinMiner() calls CreateNewBlock() in it's working loop; which is held up by either the existence of the wallet (fProofOfStake && enablewallet) [where enablewallet is defined by the -disablewallet flag]; or fGenerateBitcoins.

The minting thread is created inside nested disablewallet checks:

#ifdef ENABLE_WALLET
    if(!gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET)){
        g_wallet_init_interface.Start(scheduler);

        //Start staking thread last
        if (!gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET) && gArgs.GetBoolArg("-staking", true) && !gArgs.GetBoolArg("-exchangesandservicesmode", false))
            threadGroupStaking.create_thread(&ThreadStakeMiner);

BitcoinMiner() needs to be broken up into two threads: one for mining, one for minting. For now, since fGenerateBitcoins is defined globally, there will be a mining thread running (started with fProofofStake=false), and mining in parallel with the minting thread (started with fProofOfStake=true).

The mining thread wouldn't need to bother with checking enablewallet, since it won't be started if -disablewallet was set.

The other places that turn on fGenerateBitcoins also needs to be looked into, since they come from rpc commands; it needs to make sure it cleanly starts the thread when executed (like generatetoaddress) and actually functions with -disablewallet running (like getblocktemplate).

@CaveSpectre11 CaveSpectre11 added Component: Core App Related to the application itself. Component: Wallet Relating to keystore, tx creation, and balance tracking Tag: PoS Related to Proof of Stake Tag: PoW Related to Proof of Work consensus labels Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Core App Related to the application itself. Component: Wallet Relating to keystore, tx creation, and balance tracking Tag: PoS Related to Proof of Stake Tag: PoW Related to Proof of Work consensus
Projects
None yet
Development

No branches or pull requests

2 participants