0xs34n / coin-cli

💸A minimal cryptocurrency CLI implementation in TypeScript & Immutable.js
https://coindemo.io
Apache License 2.0
188 stars 37 forks source link

Fix difficulty change error #4 #5

Closed ivanhuay closed 6 years ago

ivanhuay commented 6 years ago

what occurred to me to solve the problem is to use the index of each block to validate the difficulty.

To verify that the difficulty change works, add a test that performs the mining operation 25 times

running the test with the previous version of the code breaks and returns the error that is mentioned in the issue

0xs34n commented 6 years ago

Hey @ivanhuay, thanks again for the PR!

I think this.chain.size - 1 on the get difficulty() function should fix the issue. I ran it on your fork and it passed the test. Do you want to add it to ur PR? :)

ivanhuay commented 6 years ago

Using the function with this.chain.size -1 only delays the problem

  get difficulty(): number {
    return Math.round((this.chain.size - 1)/ 50) + 3;
  }

running the test on the master branch and mining 26 times gives the error again

../Dev/blockchain/coin-cli/src/Node.ts:35
                throw e;
                ^
Failed to add block: Invalid Chain Error: Invalid Block Error: Hash 000b16ea09d53796b722a6fba6605f423f4b53ac76a531bf91adc866233e7be4 does not meet difficulty 4

The problem is when a new block is added all the previous blocks are validated using the same difficulty thath process start here:

  set chain(chain: List<Block>) {
    try {
      this.shouldReplaceChain(chain);
      this._chain = chain;
    } catch (e) {
      throw e;
    }
  }

after running

addBlock(block: Block) {
    try {
      this.isValidNextBlock(block, this.latestBlock);
      this.chain = this.chain.push(block);//this line!
    } catch (e) {
      throw `Failed to add block: ${e}`;
    }
  }

shouldReplaceChain validate all blocks from chain using the same difficulty using isValidBlocks

a possible solution

in my pull request I use the function

getIndexDifficulty(index: number) {
    return Math.round(index / 50) + 3;
  }

that uses the index of each block to get the complexity

ivanhuay commented 6 years ago

@seanjameshan As a detail above, the problem is to validate all the blocks with the same difficulty

0xs34n commented 6 years ago

Hey @ivanhuay, thank you so much for the detailed response. I really appreciate your help on this issue and you are absolutely correct, my solution just delays the problem.

I feel like there is redundancy in getting the difficulty. How about we have one function called getDifficulty(index) that calculates the difficulty given the index?

Blockchain.js

  // remove get difficulty() 
  getDifficulty(index: number) {
    return Math.round(index / 50) + 3;
  }

inside isHashDifficult() function

return i >= this.getDifficulty(index);

inside isValidDifficulty() function

  isValidDifficulty(block: Block) {
    const hash = block.hash;
    if (!this.isHashDifficult(hash, block.index)) {
      throw `Hash ${hash} does not meet difficulty ${this.getDifficulty(block.index)}`; // add the block index when throwing the error
    }
  }

inside generateNextBlock() function

while (!this.isHashDifficult(hash, index)) { // add the index here

Ran it to mine 25 and 26 blocks and it passed! :)

Would love to get your feedback on this! Sorry for the late response - It's approaching the end of the school semester, so it's getting quite busy! But I'll definitely get back to you as soon as I can.

ivanhuay commented 6 years ago

I agree that solution looks good

I updated my pull request with those changes

0xs34n commented 6 years ago

Thank you @ivanhuay !!