CloakProject / codename-phoenix

New CloakCoin wallet base, Bitcoin fork 0.21.1
MIT License
5 stars 3 forks source link

CheckStakeKernelHash fails in CheckProofOfStake. #7

Closed r3rcloak closed 5 years ago

r3rcloak commented 5 years ago

CheckStakeKernelHash fails in CheckProofOfStake: hashProofOfStake > bnCoinDayWeight * bnTargetPerCoinDay

Values should be checked and confirmed This is occuring for block #8776 (hash 125e964127c2277dc8a5d8b298ce844972ae445db612b6e98ecbb4b64db89fa2).

anorakthagreat commented 5 years ago

image

This ends up zero - should be equivalent of txindex.pos.nTxPos - txindex.pos.nBlockPos in the old codebase.

I believe this should be used to get the nTxPos: https://github.com/CloakProject/codename-phoenix/blob/02ad6da1a356b2428c218b275a8c7ded9b848f0b/src/index/txindex.cpp#L74

P.S. don't have the new environment set up yet, otherwise I'd test.

r3rcloak commented 5 years ago

Thanks anorak. I've pretty much got it pinned down now, just a case of resolving the last of the stake modifier issues :) No real point in setting anything up until sync works imo.

r3rcloak commented 5 years ago

Thanks again for the heads up on this anorak! This has lead me down something of a rabbit hole as the means of pulling this data has now been abstracted away in the new codebase. Adding it is not the primary issue, rather that in order to retrieve the tx index we need to keep a transaction index in memory at all times. I've created a skeleton issue for this, which I'll flesh out later with more details (memory footprint headaches).

anorakthagreat commented 5 years ago

Not sure we have to keep the entire tx index in mem, that method grabs the particular txpos from disk, based on one/curren txid, so we only need that, no?

Also, just for comparison, seems like NAV doesn't calc the offset:

Navcoin:

CheckStakeKernelHash(pindexPrev, nBits, *pblockindex, txPrev, txin.prevout, tx.nTime, hashProofOfStake, targetProofOfStake, fDebug)

Cloak:

bool CheckStakeKernelHash(nBits, blockFrom, nTxPrevOffset, txPrev, prevout, nTimeTx, hashProofOfStake, fPrintProofOfStake) {

anorakthagreat commented 5 years ago

Look at CheckStakeKernelHashV2 in their implementation (main.cpp), seems they had the same issue.

They changed their hash calculation:

https://github.com/NAVCoin/navcoin-core/blob/master/src/main.cpp#L8768

vs. our current:

https://github.com/CloakProject/CloakCoin/blob/b813d5f21387143b572e7f605f302ec56377de66/src/kernel.cpp#L321

anorakthagreat commented 5 years ago

Originally implemented as a new kernel protocol in BlackCoin: https://github.com/pvasin2/blackcoin-old/commit/3658dc5efe2a159ab93b9df0eb10e6a018523956#diff-fa018f4d28b5116a35b36b72fc9f4e51

r3rcloak commented 5 years ago

Thanks! We need to maintain the old hash algorithm for PoS for a while, but I've checked out the newer version and it's less hassle-prone for sure, we just can't switch atm.

WRT keeping the tx index in mem, it's also an issue for Enigma as that needs to query previous inputs supplied by other participants. I'm sure there'll be a solution though, query the disk ad-hoc may leave things open to disk thrashing attacks. More thought needed down the line.

anorakthagreat commented 5 years ago

Thanks! We need to maintain the old hash algorithm for PoS for a while, but I've checked out the newer version and it's less hassle-prone for sure, we just can't switch atm.

WRT keeping the tx index in mem, it's also an issue for Enigma as that needs to query previous inputs supplied by other participants. I'm sure there'll be a solution though, query the disk ad-hoc may leave things open to disk thrashing attacks. More thought needed down the line.

True, but we don't need to dump the old algo, look at BlackCoin, they kept both hash functions (V1 and V2). Re txindex - if we need to keep track of stuff in there, it's either gotta be stored in mem or on disk: mem is a bit faster and only makes a difference when syncing a bunch of blocks, syncing 1 block/min at runtime is not an issue; storage is cheaper resource-wise, reading from SSD or fast SDcards is fast enough for sync. Don't believe disk thrashing would be an issue, there's no writes involved here..

r3rcloak commented 5 years ago

resolved in commit: https://github.com/CloakProject/codename-phoenix/commit/743b95b06396a3bc9c995c1842eaf781f3a27463