Page MenuHomePhabricator

Add a test to ensure memory isn't consumed for blocks pre-checkpoint
ClosedPublic

Authored by jasonbcox on Mar 22 2019, 19:49.

Details

Summary

This adds coverage on three test cases, where blocks are rejected and do not leak memory in the block index for all cases:

  1. Forks from a new genesis block.
  2. Forks from before a checkpoint that has been reached.
  3. Forks at a checkpoint.
Test Plan
make check
ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
pr11028-test-3
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5505
Build 9072: Bitcoin ABC Buildbot (legacy)
Build 9071: arc lint + arc unit

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.

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!).

Fabien requested changes to this revision.Mar 26 2019, 09:55
Fabien added inline comments.
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

This revision now requires changes to proceed.Mar 26 2019, 09:55
Fabien requested changes to this revision.Mar 28 2019, 09:14

Please request review again when #include "chainparams.cpp" hsa been removed, clearing my queue.

src/test/checkpoints_tests.cpp
15 ↗(On Diff #7841)

This seems unnecessary

96 ↗(On Diff #7841)

dito

This revision now requires changes to proceed.Mar 28 2019, 09:14
src/test/checkpoints_tests.cpp
15 ↗(On Diff #7841)

The chainparams classes are defined in the cpp file rather than the header. These need to be moved otherwise.

96 ↗(On Diff #7841)

Without this, the tests fail due to a blocksize check in CheckBlock() which is called by ContextualCheckBlockHeader.

deadalnix requested changes to this revision.Apr 3 2019, 20:49
deadalnix added inline comments.
src/test/checkpoints_tests.cpp
82 ↗(On Diff #7929)

Do you really want to make a copy here?

98 ↗(On Diff #7929)

MakeUnique

101 ↗(On Diff #7929)

Why is there an overide to return the default value?

This revision now requires changes to proceed.Apr 3 2019, 20:49
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.

Fixes for recent feedback

jasonbcox changed the visibility from "Custom Policy" to "Restricted Project (Project)".Apr 4 2019, 17:35
jasonbcox changed the edit policy from "Restricted Project (Project)" to "Restricted Project (Project)".
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.

deadalnix requested changes to this revision.Apr 6 2019, 12:58
deadalnix added inline comments.
src/test/checkpoints_tests.cpp
101 ↗(On Diff #7933)

There is still no explanation provided for this, even though as you point out, both @Fabien and myself stumbled on it.

This revision now requires changes to proceed.Apr 6 2019, 12:58

Added a comment to explain config blocksize being set

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...

deadalnix requested changes to this revision.Apr 14 2019, 19:50
deadalnix added inline comments.
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.

This revision now requires changes to proceed.Apr 14 2019, 19:50
jasonbcox added inline comments.
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.

jasonbcox marked 5 inline comments as done.

Rebase and address feedback

deadalnix requested changes to this revision.Apr 20 2019, 14:55
deadalnix added inline comments.
src/chainparams.h
109
src/config.h
114

It doesn't seems that this needs to be proctected

This revision now requires changes to proceed.Apr 20 2019, 14:55

Use existing API for mainnet genesis block.
Revert unnecessary protected change.

src/chainparams.h
109

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.

deadalnix requested changes to this revision.May 4 2019, 17:49

If you don't need a new API, then don't change the API.

src/chainparams.cpp
61 ↗(On Diff #8210)

Revert.

src/chainparams.h
110 ↗(On Diff #8210)

Remove.

This revision now requires changes to proceed.May 4 2019, 17:49
This revision is now accepted and ready to land.May 14 2019, 21:28

Rebase + added missing includes due to refactors over the last couple weeks

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.

jasonbcox changed the visibility from "Restricted Project (Project)" to "Public (No Login Required)".
This revision was landed with ongoing or failed builds.May 24 2019, 16:35
This revision was automatically updated to reflect the committed changes.