multi: Introduce AssumeValid.

[rstaudt2]

This introduces an AssumeValid parameter and configuration option. It is broken down into several commits to ease the review process.

---

Motivation: Checkpoints vs AssumeValid

Prior to this PR, Checkpoints were being used for two distinct purposes:

• Reject blocks that fork the main chain before the most recently known checkpoint
• Allow several validation checks to be skipped for blocks that are an ancestor of the most recently known checkpoint

This PR introduces a new parameter, AssumeValid, to control when validation checks can be skipped rather than continuing to use Checkpoints for this purpose.

The separation of Checkpoints and AssumeValid provides a few benefits:

• It allows for node operators to configure each capability independently
• It allows for testing the distinct sync options independently
• It allows for specifying an AssumeValid hash that is closer to tip
  • This is safe since when bypassing some validation checks with assume valid for a given block, the block is still validated to be part of the chain with the most proof of work and additionally must be an ancestor of the assumed valid block, meaning that changes in any transaction data would be detected as it would necessarily change the merkle root of the assumed valid block
  • This would not be safe with Checkpoints since rejecting forks before a certain point affects consensus

---

The --assumevalid config option

Since the AssumeValid hash can be closer to tip, this PR adds the ability to set it without a code change via an --assumevalid option. It can be specified as follows:

• (flag not specified): defaults to the hard-coded value in source
• --assumevalid=0: disable AssumeValid
• --assumevalid=[blockhash]: set AssumeValid to the specified block hash

Specifying an AssumeValid hash closer to tip allows for faster syncing. This is useful since the hard-coded AssumeValid block becomes deeper in the chain (and therefore more validation needs to be done post the AssumeValid block) the longer a particular release build is out.

As noted in the implementation, AssumeValid is clamped back as needed to ensure that it is at least 2 weeks worth of blocks behind the best header. This protects against malicious actors trying to play games by tricking users into using an assumed valid hash near the tip.

[davecgh]

I haven't reviewed the tests yet, but I spotted an issue I wanted to call out in the mean time.

[matthawkins90]

As I'm going through the readme and the code, I noticed that you seem to be using "AssumeValid Hash" and "AssumeValid Node" interchangeably.

For instance, in the comment above:

Specifying an AssumeValid hash closer to tip allows for faster syncing. This is useful since the hard-coded AssumeValid node becomes deeper in the chain (and therefore more validation needs to be done post the AssumeValid node) the longer a particular release build is out.

Did you do this intentionally, and I'm just misunderstanding?

Otherwise, I think it makes more sense to say that you specify "AssumeValid Hash" or an "AssumeValid Block", and if a node is broadcasting either of those things, then I suppose you can call it an "AssumeValid Node."

[davecgh]

It's because the term node is overloaded. It refers to both the peer (network node) as well as the internal nodes (blockNode) of the data structure that comprise the full block tree that a specific branch that ultimately becomes the blockchain is selected from.

So, the hash is what you specify as the config option. The node (aka block / blockNode) is what that hash resolves to (if any).

It would probably be clearer to those less familiar with the code to refer to them as blocks whenever they're being referencing in the same context as network nodes/peers.

EDIT: As for the specific text in question, I agree that it would probably read better as:

... This is useful since the hard-coded AssumeValid block becomes ...

[davecgh]

Confirming this hash:

$ dcrctl getblockhash 606000
00000000000000001251efb83ad1a5c71351b50fe9195f009cf0bf5a7cd99d52

[davecgh]

Confirming this hash as well:

$ dcrctl --testnet getblockhash 807905
00000000d64ceb1a686315ed56235e9a6838e3a22e9ec9bd92c2e04c09e0778b

[davecgh]

The test coverage looks great overall. Do you think it's worth adding one for the clamping on a side chain case identified during the review to prevent any possible regressions in that regard?

[rstaudt2]

Do you think it's worth adding one for the clamping on a side chain case identified during the review to prevent any possible regressions in that regard?

Yep, I was thinking the same. I added a test for this and validated that the test will fail if the fix is reverted.

[rstaudt2]

As I'm going through the readme and the code, I noticed that you seem to be using "AssumeValid Hash" and "AssumeValid Node" interchangeably.

Did you do this intentionally, and I'm just misunderstanding?

@matthawkins90 Thanks for pointing out that the terminology was a bit confusing. As @davecgh noted, the term "node" is overloaded, and when used in place of "block", it is referring to the internal struct blockNode that represents a block.

I've updated to use "block" rather than "node" for the cases that are not directly referring to a variable of the blockNode type.

[davecgh]

In addition to the code review, I also tested this as follows:

• Added prints for chosen flags and script execution policy to manually inspect the results
• Ran mainnet and testnet, and simnet with the default values
• Ran mainnet and testnet, and simnet with various custom values, including 0 to disable
• Forced clamping, both on the main chain and side chain via simnet
• Triggered reorgs of varying complexity via simnet

[matthawkins90]

Thanks for the explanation! Yup, sorry, I wasn't aware of the blockNode struct terminology. I see now that there's already a pretty heavily embedded syntax of variables and functions using node to refer to the blocks in the tree: blockNode, nodeStatus, node.hash, node.height. Makes sense now.