This adds coverage on three test cases, where blocks are rejected and do not leak memory in the block index for all cases:
- Forks from a new genesis block.
- Forks from before a checkpoint that has been reached.
- Forks at a checkpoint.
Details
Details
- Reviewers
deadalnix Fabien - Group Reviewers
Restricted Project - Commits
- rSTAGING13c0b0619074: Add a test to ensure memory isn't consumed for blocks pre-checkpoint
rABC13c0b0619074: Add a test to ensure memory isn't consumed for blocks pre-checkpoint
make check ninja check
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| src/test/checkpoints_tests.cpp | ||
|---|---|---|
| 71 ↗ | (On Diff #7806) | Realized I missed a check on state. I'll fix that tomorrow. |
Comment Actions
Massively improved test coverage (see updated summary)
| src/config.h | ||
|---|---|---|
| 113 ↗ | (On Diff #7797) | Added this back. I think it makes sense that any class derived from DummyConfig (only used for testing) should have access to the chainParams and be able to modify them. |
| src/test/checkpoints_tests.cpp | ||
|---|---|---|
| 12 ↗ | (On Diff #7831) | This oddity works but is bad practice. This very likely needs to be fixed before landing, but I wanted to share this proof-of-concept of the test both for initial review and before I go to sleep today (right now!). |
| src/test/checkpoints_tests.cpp | ||
|---|---|---|
| 207 ↗ | (On Diff #7831) | Also check the reject reason to ensure both cases are covered, as the reject code is identical for B and AB |
| src/test/checkpoints_tests.cpp | ||
|---|---|---|
| 101 ↗ | (On Diff #7929) | Fabien asked this already ^^ DummyConfig defines max block size as 0 by default. Without setting it, the tests fail due to a blocksize check in CheckBlock() which is called by ContextualCheckBlockHeader. |
| src/test/checkpoints_tests.cpp | ||
|---|---|---|
| 96 ↗ | (On Diff #7841) | What does the DummyConfig returns for block size i you don't override ? If it is nonsense, overriding it all over the place is hardly the right way to go. |
Comment Actions
Remove unnecessary setting of blocksize
| src/test/checkpoints_tests.cpp | ||
|---|---|---|
| 101 ↗ | (On Diff #7933) | Since I'm copying from mainparams, this is no longer necessary... |
| src/test/checkpoints_tests.cpp | ||
|---|---|---|
| 46 ↗ | (On Diff #8000) | You want to make a copy of the checkpoints ? |
| 50 ↗ | (On Diff #8000) | should conflict *but* not be accepted ? Why but ? |
| 63 ↗ | (On Diff #8000) | What are the properties of this header ? Why do we care ? Are we supposed to parse hexadecimal by hand to figure this out ? |
| 81 ↗ | (On Diff #8000) | ref ? |
| 98 ↗ | (On Diff #8000) | Note that this isn't per se incorrect, but the whole config is initialized with invalid chainParams, and then the correct ones are created and the old ones removed in favor of the new ones. This doesn't make a lot of sense. Why initialize the object wrong and then fix it rather than initialize it right ? |
| 131 ↗ | (On Diff #8000) | Is this the actual genesis block ? If so, there is a function to generate it. |
| 191 ↗ | (On Diff #8000) | You assume these heard have some properties. I know information in comments is never incorrect, but just in case, it would b good to check for it. |
| 217 ↗ | (On Diff #8000) | You just made a refactoring relative to the use of that map, so you should do it here as well, or you are sabotaging your own work. |
| src/test/checkpoints_tests.cpp | ||
|---|---|---|
| 50 ↗ | (On Diff #8000) | The 'but' does seem odd. Fixed. |
| 98 ↗ | (On Diff #8000) | I definitely thought about this when I implemented it. Looking with fresh eyes, it's clear that initializing it the correct way is the right thing to do. I think I was just lazy the first time around. |
| 131 ↗ | (On Diff #8000) | Good call. |
| 191 ↗ | (On Diff #8000) | Good idea. Done. |
| 217 ↗ | (On Diff #8000) | Good catch. Done. |
| src/chainparams.h | ||
|---|---|---|
| 109 ↗ | (On Diff #8124) | I don't think you need a new API. https://reviews.bitcoinabc.org/source/bitcoin-abc/browse/master/src/chainparams.h$58 |
| src/config.h | ||
| 114 ↗ | (On Diff #8124) | It doesn't seems that this needs to be proctected |
| src/chainparams.h | ||
|---|---|---|
| 109 ↗ | (On Diff #8124) | Good callout for the mainnet genesis block, but the first test generates its own genesis block with a unique nTime, so the API is still needed. |
Comment Actions
For posterity: Harbormaster seems to be missing something and not detecting the tests being run, despite them passing. I'm going to land this and keep an eye on master just to be sure.