proof of work based on bitcoin.

[b-yap]

continue from: #62

address comments:
#62 (comment)

I don't think the UpgradeVersion is necessary here.

#62 (comment)

Whenever you take a link from github, make sure you use a permalink, which contains the commit hash. You can do this by clicking on the ... on the left of the line number. Why? Because the master branch can always change.

#62 (comment)

This whole file should probably go into ChainConfig.
TARGET_TIMESPAN_SECS should be in ChainConfig, and should be returned as rust Duration.
Uint256 version should be derived from that, and not hardcoded.
TARGET_SPACING should certainly be in ChainConfig.
DIFFICULTY_ADJUSTMENT_INTERVAL should be a function.
TIMESPAN_ADJUSTMENT_FACTOR: This should be called MAX_DIFFICULTY_ADJUSTMENT_PER_INTERVAL, which is basically the max increase/decrease in work that can happen. This protects from having huge swings in difficulty. Also should go into ChainConfig
Finally, the upper and lower things also should be functions.
Preferably, find a way to group them all under PoW in ChainConfig instead of having them scattered. Maybe just prefix? Maybe substruct? I don't know. Please propose something.

#62 (comment)

The block index contains the height

#62 (comment)

call it "interval" instead of 2016 blocks

#62 (comment)

So why do we have a separate function for testnet if we do this check which is true anyway for testnet? Please combine both functions into one function and make them depend on ChainConfig.

Another TODO: To be created in another PR.
#62 (comment)

Sam: This isn't an operation I'm particularly happy about and I think we should discuss the reason why this is necessary. Since we're starting from scratch, let's try to make both H256 and Uint256 use the same order. No need to reverse things every time. This can get very confusing and error-prone very quickly.

Ben: As we briefly discussed earlier I think forking fixed_hash is the best way to approach this, my 30s peak makes it look like we should be dandy dependency wise so it's just a case of fixing the macros and tests and we should be good to go.

[TheQuantumPhysicist]

This doesn't seem to be correct. The block that you use to recalculate the new nBits is the first block in the 2016-block interval. Then, if that is longer than two weeks, the difficulty goes down. If it's less than two weeks, the difficulty goes up.

[b-yap]

I will rename the function to get_starting_block_time.

The function is using the prev_block_index to call the get_ancestor method, based on the adjustment interval.

[iljakuklic]

I know this would deviate from Bitcoin but the difficulty is determined by the time difference of the last two blocks that are at a height that is a multiple of 2016, so this field is not needed. Even if the field is included, we need to check it has the correct value if it's relied upon for the difficulty check. Have you thought about getting rid of it?

[b-yap]

The bits field is being used in the ff. ways:

1. When no_retargeting field in the config is set to true. That only happens for Regtest.

   mintlayer-core/common/src/chain/pow.rs

   Line 59 in 1e390f9

   ChainType::Regtest => true,

2. if it's not due for retargeting yet, and allow_min_dfficulty_blocks is set to false, which is the Mainnet and Signet:

   mintlayer-core/common/src/chain/pow.rs

   Line 65 in 1e390f9

   ChainType::Mainnet | ChainType::Signet => false,

Let's say the current block height is 8067, and it's not due for retarget (since retarget has happened at 8064).
I am assuming that the difficulty is retrieved from the previous block, at height 8066. No calculation needed.
If we remove the bits field, we'll have to keep calculating the time diff of the blocks of height 8064 and 6048.

As for checking whether its value is correct, Did you mean the check_proof_of_work()? Or do you mean checking whether the previous block's bits field is a correct value?

[iljakuklic]

As for checking whether its value is correct, Did you mean the check_proof_of_work()? Or do you mean checking whether the previous block's bits field is a correct value?

What I mean is that the value of the bits field cannot be trusted to contain the difficulty anyway. I see the get_work_required method here but I can't see it being used anywhere. I.e. at any point, the following must pass:

assert_eq!(calculate_difficulty_from_the_scratch(...).try_into(), Just(this_block.bits))

Blocks that do not satisfy that condition must be rejected, since the check_proof_of_work function relies on the bits field to contain accurate difficulty.

[b-yap]

The get_work_required()(previously check_for_work_required()) will be used in the block production.
This was the earliest implementation : ed8db7d#diff-9a00856cffad13af6904b1cb9d4b5a4fd9e0591f10a35a67ddd805141890eeb6R91

But I removed it in this PR, because it's meant for the block production PR (which is currently closed, until Anton's or this is pushed).

Did you mean because the bits is a u32 representation, while the difficulty is 256, that's why the bits field can't be trusted? Or because the previous block can't be trusted, since the bits field can easily be just any number and not from a calculated value during the generation of the 2016th multiple block?

I haven't checked in bitcoin where that is, but we can actually add that kind of validation.

I somehow do agree with you; removing the bits field is not so bad, since we can always calculate for the difficulty. Your get_block_id_by_height function will be useful (Though I think I have to access it through the BlockIndex).

For now I've been following how Bitcoin does it. But also I don't have problems diverging from Bitcoin.
Although I don't know what Sam @TheQuantumPhysicist thinks, we can wait for his opinion.