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.

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

Fixes for staking code and thread shutdown #505

Closed
wants to merge 10 commits into from
Closed

Fixes for staking code and thread shutdown #505

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
acd2b3b
wait for mining threads to stop
dimxy Sep 23, 2021
24f7d50
convert staking array to vector (to correctly destroy members) and ma…
dimxy Sep 23, 2021
87ac53d
staking logging improved
dimxy Sep 23, 2021
f582ad9
maxkp replaced with array.capacity
dimxy Sep 25, 2021
ba1b22a
'struct' eliminated
dimxy Sep 25, 2021
5098d67
unneeded memset removed
dimxy Sep 25, 2021
dcbf657
Update src/miner.cpp
dimxy Jul 5, 2022
f8c17ce
add interruption_point in komodo_waituntilelegible
dimxy Jul 12, 2022
12261b9
change sleeps to interruptible in miner
dimxy Jul 14, 2022
106c9d3
Merge branch 'dev' into dimxy-fix-staking-array
dimxy Jul 14, 2022
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
88 changes: 45 additions & 43 deletions src/komodo_bitcoind.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2450,35 +2450,39 @@ int64_t komodo_coinsupply(int64_t *zfundsp,int64_t *sproutfundsp,int32_t height)
return(supply);
}

struct komodo_staking *komodo_addutxo(struct komodo_staking *array,int32_t *numkp,int32_t *maxkp,uint32_t txtime,uint64_t nValue,uint256 txid,int32_t vout,char *address,uint8_t *hashbuf,CScript pk)
void komodo_addutxo(std::vector<komodo_staking> &array,int32_t *maxkp,uint32_t txtime,uint64_t nValue,uint256 txid,int32_t vout,char *address,uint8_t *hashbuf,CScript pk)
{
uint256 hash; uint32_t segid32; struct komodo_staking *kp;
uint256 hash; uint32_t segid32; struct komodo_staking kp;
segid32 = komodo_stakehash(&hash,address,hashbuf,txid,vout);
if ( *numkp >= *maxkp )
if ( array.size() >= *maxkp )
{
*maxkp += 1000;
Copy link

Choose a reason for hiding this comment

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

I can't see the reason for maxkp. I'm thinking it is to control allocations. If the desired functionality is to grow in chunks of 1000 elements, why not use array.size()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay thank you, maxkp can be eliminated and array.size() and capacity() be used

array = (struct komodo_staking *)realloc(array,sizeof(*array) * (*maxkp));
//fprintf(stderr,"realloc max.%d array.%p\n",*maxkp,array);
}
kp = &array[(*numkp)++];
//fprintf(stderr,"kp.%p num.%d\n",kp,*numkp);
memset(kp,0,sizeof(*kp));
strcpy(kp->address,address);
kp->txid = txid;
kp->vout = vout;
kp->hashval = UintToArith256(hash);
kp->txtime = txtime;
kp->segid32 = segid32;
kp->nValue = nValue;
kp->scriptPubKey = pk;
return(array);
//array = (struct komodo_staking *)realloc(array,sizeof(*array) * (*maxkp));
Copy link

Choose a reason for hiding this comment

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

Comments like this make the code difficult to follow. If you remove these lines before committing you lose nothing, as features within git make historical research easy.

array.reserve(*maxkp);
//fprintf(stderr,"realloc max.%d array.size().%d array.capacity().%d\n", *maxkp,array.size(), array.capacity());
}
memset(&kp,0,sizeof(kp));
strcpy(kp.address,address);
kp.txid = txid;
kp.vout = vout;
kp.hashval = UintToArith256(hash);
kp.txtime = txtime;
kp.segid32 = segid32;
kp.nValue = nValue;
kp.scriptPubKey = pk;
array.push_back(kp);
//fprintf(stderr,"kp.%p array.size().%d\n",kp,array.size());
}

int32_t komodo_staked(CMutableTransaction &txNew,uint32_t nBits,uint32_t *blocktimep,uint32_t *txtimep,uint256 *utxotxidp,int32_t *utxovoutp,uint64_t *utxovaluep,uint8_t *utxosig, uint256 merkleroot)
{
static struct komodo_staking *array; static int32_t numkp,maxkp; static uint32_t lasttime;
// use thread_local to prevent crash in case of accidental thread overlapping
thread_local std::vector<komodo_staking> array;
thread_local int32_t maxkp;
thread_local uint32_t lasttime;

int32_t PoSperc = 0, newStakerActive;
std::set<CBitcoinAddress> setAddress; struct komodo_staking *kp; int32_t winners,segid,minage,nHeight,counter=0,i,m,siglen=0,nMinDepth = 1,nMaxDepth = 99999999; std::vector<COutput> vecOutputs; uint32_t block_from_future_rejecttime,besttime,eligible,earliest = 0; CScript best_scriptPubKey; arith_uint256 mindiff,ratio,bnTarget,tmpTarget; CBlockIndex *tipindex,*pindex; CTxDestination address; bool fNegative,fOverflow; uint8_t hashbuf[256]; CTransaction tx; uint256 hashBlock;
std::set<CBitcoinAddress> setAddress; int32_t winners,segid,minage,nHeight,counter=0,i,m,siglen=0,nMinDepth = 1,nMaxDepth = 99999999; std::vector<COutput> vecOutputs; uint32_t block_from_future_rejecttime,besttime,eligible,earliest = 0; CScript best_scriptPubKey; arith_uint256 mindiff,ratio,bnTarget,tmpTarget; CBlockIndex *tipindex,*pindex; CTxDestination address; bool fNegative,fOverflow; uint8_t hashbuf[256]; CTransaction tx; uint256 hashBlock;
uint64_t cbPerc = *utxovaluep, tocoinbase = 0;
if (!EnsureWalletIsAvailable(0))
return 0;
Expand All @@ -2500,7 +2504,7 @@ int32_t komodo_staked(CMutableTransaction &txNew,uint32_t nBits,uint32_t *blockt
// this was for VerusHash PoS64
//tmpTarget = komodo_PoWtarget(&PoSperc,bnTarget,nHeight,ASSETCHAINS_STAKED);
bool resetstaker = false;
if ( array != 0 )
if ( array.size() != 0 )
{
LOCK(cs_main);
CBlockIndex* pblockindex = chainActive[tipindex->GetHeight()];
Expand All @@ -2512,15 +2516,14 @@ int32_t komodo_staked(CMutableTransaction &txNew,uint32_t nBits,uint32_t *blockt
}
}

if ( resetstaker || array == 0 || time(NULL) > lasttime+600 )
if ( resetstaker || array.size() == 0 || time(NULL) > lasttime+600 )
{
LOCK2(cs_main, pwalletMain->cs_wallet);
pwalletMain->AvailableCoins(vecOutputs, false, NULL, true);
if ( array != 0 )
if ( array.size() != 0 )
{
free(array);
array = 0;
maxkp = numkp = 0;
array.clear();
maxkp = 0;
lasttime = 0;
}
BOOST_FOREACH(const COutput& out, vecOutputs)
Expand All @@ -2546,16 +2549,16 @@ int32_t komodo_staked(CMutableTransaction &txNew,uint32_t nBits,uint32_t *blockt
continue;
if ( myGetTransaction(out.tx->GetHash(),tx,hashBlock) != 0 && (pindex= komodo_getblockindex(hashBlock)) != 0 )
{
array = komodo_addutxo(array,&numkp,&maxkp,(uint32_t)pindex->nTime,(uint64_t)nValue,out.tx->GetHash(),out.i,(char *)CBitcoinAddress(address).ToString().c_str(),hashbuf,(CScript)pk);
//fprintf(stderr,"addutxo numkp.%d vs max.%d\n",numkp,maxkp);
komodo_addutxo(array,&maxkp,(uint32_t)pindex->nTime,(uint64_t)nValue,out.tx->GetHash(),out.i,(char *)CBitcoinAddress(address).ToString().c_str(),hashbuf,(CScript)pk);
//fprintf(stderr,"addutxo array.size().%d vs max.%d\n",array.size(),maxkp);
}
}
}
lasttime = (uint32_t)time(NULL);
//fprintf(stderr,"finished kp data of utxo for staking %u ht.%d numkp.%d maxkp.%d\n",(uint32_t)time(NULL),nHeight,numkp,maxkp);
//fprintf(stderr,"finished kp data of utxo for staking %u ht.%d array.size().%d maxkp.%d\n",(uint32_t)time(NULL),nHeight,array.size(),maxkp);
}
block_from_future_rejecttime = (uint32_t)GetTime() + ASSETCHAINS_STAKED_BLOCK_FUTURE_MAX;
for (i=winners=0; i<numkp; i++)
for (i=winners=0; i<array.size(); i++)
{
if ( fRequestShutdown || !GetBoolArg("-gen",false) )
return(0);
Expand All @@ -2564,23 +2567,23 @@ int32_t komodo_staked(CMutableTransaction &txNew,uint32_t nBits,uint32_t *blockt
fprintf(stderr,"[%s:%d] chain tip changed during staking loop t.%u counter.%d\n",ASSETCHAINS_SYMBOL,nHeight,(uint32_t)time(NULL),i);
return(0);
}
kp = &array[i];
eligible = komodo_stake(0,bnTarget,nHeight,kp->txid,kp->vout,0,(uint32_t)tipindex->nTime+ASSETCHAINS_STAKED_BLOCK_FUTURE_HALF,kp->address,PoSperc);
struct komodo_staking &kp = array[i];
Copy link

Choose a reason for hiding this comment

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

structs have been upgraded to first-class citizens now. No need to use that keyword here (unless you're compiling for C).

eligible = komodo_stake(0,bnTarget,nHeight,kp.txid,kp.vout,0,(uint32_t)tipindex->nTime+ASSETCHAINS_STAKED_BLOCK_FUTURE_HALF,kp.address,PoSperc);
if ( eligible > 0 )
{
besttime = 0;
if ( eligible == komodo_stake(1,bnTarget,nHeight,kp->txid,kp->vout,eligible,(uint32_t)tipindex->nTime+ASSETCHAINS_STAKED_BLOCK_FUTURE_HALF,kp->address,PoSperc) )
if ( eligible == komodo_stake(1,bnTarget,nHeight,kp.txid,kp.vout,eligible,(uint32_t)tipindex->nTime+ASSETCHAINS_STAKED_BLOCK_FUTURE_HALF,kp.address,PoSperc) )
{
// have elegible utxo to stake with.
if ( earliest == 0 || eligible < earliest || (eligible == earliest && (*utxovaluep == 0 || kp->nValue < *utxovaluep)) )
if ( earliest == 0 || eligible < earliest || (eligible == earliest && (*utxovaluep == 0 || kp.nValue < *utxovaluep)) )
{
// is better than the previous best, so use it instead.
earliest = eligible;
best_scriptPubKey = kp->scriptPubKey;
*utxovaluep = (uint64_t)kp->nValue;
decode_hex((uint8_t *)utxotxidp,32,(char *)kp->txid.GetHex().c_str());
*utxovoutp = kp->vout;
*txtimep = kp->txtime;
best_scriptPubKey = kp.scriptPubKey;
*utxovaluep = (uint64_t)kp.nValue;
decode_hex((uint8_t *)utxotxidp,32,(char *)kp.txid.GetHex().c_str());
*utxovoutp = kp.vout;
*txtimep = kp.txtime;
}
/*if ( eligible < block_from_future_rejecttime )
{
Expand All @@ -2591,11 +2594,10 @@ int32_t komodo_staked(CMutableTransaction &txNew,uint32_t nBits,uint32_t *blockt
}
}
}
if ( numkp < 500 && array != 0 )
if ( array.size() < 500 && array.size() != 0 )
{
free(array);
array = 0;
maxkp = numkp = 0;
array.clear();
maxkp = 0;
lasttime = 0;
}
if ( earliest != 0 )
Expand Down
2 changes: 1 addition & 1 deletion src/komodo_bitcoind.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,6 @@ struct komodo_staking
CScript scriptPubKey;
};

struct komodo_staking *komodo_addutxo(struct komodo_staking *array,int32_t *numkp,int32_t *maxkp,uint32_t txtime,uint64_t nValue,uint256 txid,int32_t vout,char *address,uint8_t *hashbuf,CScript pk);
void komodo_addutxo(std::vector<komodo_staking> &array,int32_t *maxkp,uint32_t txtime,uint64_t nValue,uint256 txid,int32_t vout,char *address,uint8_t *hashbuf,CScript pk);

int32_t komodo_staked(CMutableTransaction &txNew,uint32_t nBits,uint32_t *blocktimep,uint32_t *txtimep,uint256 *utxotxidp,int32_t *utxovoutp,uint64_t *utxovaluep,uint8_t *utxosig, uint256 merkleroot);
2 changes: 2 additions & 0 deletions src/miner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2188,6 +2188,8 @@ void static BitcoinMiner()
if (minerThreads != NULL)
{
minerThreads->interrupt_all();
std::cout << "Waiting for mining threads to stop..." << std::endl;
dimxy marked this conversation as resolved.
Show resolved Hide resolved
minerThreads->join_all(); // prevent thread overlapping
Copy link

Choose a reason for hiding this comment

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

Given the lack of interruption points, using both interrupt_all() and join_all() can (and "randomly" will) bring everything to a grinding halt when between interruptibles and unjoinable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point about lack of interruption points.
Currently we do not have join what leads to inaccurate thread shutdown.
And adding join_all works well in marmara and fixed a crash when a user called 'setgenerate false' and 'setgenerate true' quickly in a sequence.
But I missed an extra interruption_point (from the marmara code) in komodo_waituntilelegible which does a long loop which would make thread shutdowns faster, adding it

Copy link

Choose a reason for hiding this comment

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

and fixed a crash when a user called 'setgenerate false' and 'setgenerate true' quickly in a sequence.

This is actually where adding the join_all() creates the issue. Most specifically, it creates an issue with NN mining and CreateNewBlock (which MCL doesn't use).

Copy link
Collaborator Author

@dimxy dimxy Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Could you explain what issue you mean? I believe joins may create an issue when a thread can never be joined and hangs because it may have a long loop without interruption points or sleep or wait calls. If we have such loops we need to fix this to provide thread graceful shutdown to work.
(I am going to run my dev NN node on this branch for testing)

Copy link

@TheComputerGenie TheComputerGenie Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

BitcoinMiner only contains a single interruption_point()
That point comes after the while loop at:

komodo/src/miner.cpp

Line 1969 in dcbf657

while ( GetTime() < B.nTime-2 )

which can hold up to 17.5 minutes, as set by:

komodo/src/miner.cpp

Line 860 in dcbf657

pblock->nTime += (r % (33 - gpucount)*(33 - gpucount));

Ironically, the likelihood of that longest pause is increased by the new "stall reduction" code increasing the possibility for gpucount to reach 0 (assuming the rand hits 1056, which is possible).

Being async, the thought would be that this part would stick on its own and there would be no care; however, in reality, there are about a dozen circumstances where it locks everything (one of those circumstances being that threads are ignorant of each other and on advanced hardware come back with multiple solves when many miner threads are used).

When you look at what "should be" vs "what is", it shouldn't be a problem because NNs "should be" only running one thread; however, this is becoming decreasingly true with more and more NNs seeking to hit smaller and more predictable gaps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a good catch, this loop.
However I believe when a user calls setgenerate false and then setgenerate true without join there will be two running threads for some time and this is basically not good at all (in a staking chain this could even create a crash as there is a static komodo_staking *array var that could be corrupted in this case). So I think threads should be stopped gracefully by joining.
This loop you mentioned has sleep() inside and we can replace it on boost::this_thread::sleep_for function (which allows to interrupt the thread) and we should check other remaining loops in miner.cpp to add interruption points in a similar way

Copy link

@TheComputerGenie TheComputerGenie Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

and we can replace it on boost::this_thread::sleep_for function (which allows to interrupt the thread) and we should check other remaining loops in miner.cpp to add interruption points in a similar way

However you want to do it, just want to make sure that you/everyone is aware that doing it as-is will lock up NNs; so, whatever way it's done to protect stakers that could crash needs to be done in such a way as to protect both.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure about your doubts though.
Deleting a thread object without join is an obvious bug IMO and should be fixed.
Fixing it may add some delay on daemon stopping or setgenerate false but this is not a lock-up if we do this properly.
Checking all loops in miner.cpp...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

btw this code does not work at all:

komodo/src/miner.cpp

Line 251 in 91ea37b

boost::this_thread::disable_interruption();
as 'disable_interruption' is a type and to activate it we need to create a local var.
Maybe we should fix this too as it was intended

Copy link

@TheComputerGenie TheComputerGenie Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Haven't fully tested it yet, but (along with the others)

komodo/src/miner.cpp

Line 1989 in 12261b9

boost::this_thread::sleep_for(boost::chrono::seconds(1)); // allow to interrupt

does look like it'll solve the issue of my concern. ty

delete minerThreads;
minerThreads = NULL;
}
Expand Down