mining: Improve block template generator test coverage

[santi-h]

All Submissions:

• Submission follows the Code Contribution Guidelines
• There are not any other open Pull Requests for the same update/change
• Commit messages are formatted according to Model Git Commit Messages
• All changes are compliant with the latest version of Go and the one prior to it
• The code being submitted is commented according to the Code Documentation and Commenting section of the Code Contribution Guidelines
• Any new logging statements use an appropriate subsystem and logging level
• Code has been formatted with go fmt
• Running go test does not fail any tests or report any vet issues
• Running golint does not report any new issues that did not already exist

New Feature Submissions:

• Code is accompanied by tests which exercise both the positive and negative (error paths) conditions (if applicable)

Bug Fixes:

• Code is accompanied by new tests which trigger the bug being fixed to prevent regressions

Changes to Core Features:

• An explanation of what the changes do and why they should be included is provided
• Code is accompanied by updates to tests and/or new tests for the core changes (if applicable)

[rstaudt2]

Looks like you are making some good progress on working through the mining code and adding more test coverage. Nice! A few comments on the overall design:

• The harness is intended to create everything that is needed to start from a pretty blank slate. Is there any advantage to creating the chain separately vs. just creating the harness and then changing the chain state as needed for the tests afterward like the existing tests do? This would simplify things quite a bit.
• I see you added a separate test case for the tip generation error. When you get further into it, and more coverage is added, it's probably going to be very cumbersome to have a separate test case for every small error case. I started doing something a bit more generic a while ago (see this temp commit) that allows each test scenario to specify a munge function which allows that scenario to modify things on the harness or underlying chain. It may be worth seeing if something like that makes it easier, you'll probably get a better and better idea of the best way to do it the more test cases you add.

Also, a few notes on some other code contribution guidelines. You may already be aware and be planning on cleaning them up later, but FYI:

• First line of the commit needs to be <= 50 chars and should end with a .
• Other lines in the commit need to be <= 72 chars
• Code lines should not exceed 80 chars
  • If you are using vscode, you can use rulers to show where the line length is at:

      "editor.rulers": [
        72,
        80
      ],
  

• All comments should end with a .

[santi-h]

Is there any advantage to creating the chain separately vs. just creating the harness and then changing the chain state as needed for the tests afterward like the existing tests do?

@rstaudt2 I didn't want to make changes to the chain state after the harness is created because I could very easily leave the harness in an inconsistent state. newMiningHarness uses the values from the chain to create other objects. For example it uses chain.isTreasuryAgendaActive and chain.bestState.Height in order to AddFakeUTXO. If I change chain.isTreasuryAgendaActive or chain.bestState.Height after the harness was created then it would leave it in an inconsistent state unless I re-run AddFakeUTXO manually.

But I'm probably being a bit paranoid and it's not a big deal if the harness is in an inconsistent state. Plus it does make writing tests easier. For example all these lines will just become a one liner:

chain.bestState.Height = chainParams.StakeValidationHeight - 2

I see you added a separate test case for the tip generation error. When you get further into it, and more coverage is added, it's probably going to be very cumbersome to have a separate test case for every small error case.

Ya I was thinking about that too. My plan is to start doing something dumb like this since I don't really know the code that well yet, and as I write more and more tests for NewBlockTemplate identify patterns to make writing them easier and refactor as I go. I will probably do something like what you did there with the anonymous structs. This was kind of a premature review request to make sure I'm not doing something completely crazy.

Also, a few notes on some other code contribution guidelines. You may already be aware and be planning on cleaning them up later

Yeah that exactly what I was planning on doing. I appreciate you pointing those out though since it will save me time!

[rstaudt2]

@rstaudt2 I didn't want to make changes to the chain state after the harness is created because I could very easily leave the harness in an inconsistent state. newMiningHarness uses the values from the chain to create other objects. For example it uses chain.isTreasuryAgendaActive and chain.bestState.Height in order to AddFakeUTXO. If I change chain.isTreasuryAgendaActive or chain.bestState.Height after the harness was created then it would leave it in an inconsistent state unless I re-run AddFakeUTXO manually.

It shouldn't be an issue to change values after the harness is created. What the harness is doing is just starting the chain at a point where you have some outputs to spend. After that point, you can modify the underlying chain however you want to simulate things like more blocks/transactions/utxos being added or error scenarios.

You are correct that it is easy to leave the chain in an unrealistic/inconsistent state, though. For example, in reality you should be adding real blocks with corresponding transactions, utxos, and updating all values of the best state as well as several other things. An alternative to this is to use the chaingen harness which creates a real underlying chain. You can see an example test I added recently for the consensus changes I was making here. The downside to using that is it is harder to mock certain scenarios, which is why I started with a fake chain for this test suite, but I wouldn't be opposed to using that either.