Page MenuHomePhabricator

Fix abc-finalize-block.py header propagation issue
ClosedPublic

Authored by Fabien on Nov 25 2018, 17:01.

Details

Summary

The wait_for_invalid_block success is dependent of the header propagation.
It assumes that the last block generated by alt_node will become the tip
of node, which is not the case if previous invalid headers are received.

Also add ascii art in comments

Test Plan
./test/functional/test_runner.py abc-finalize-block

Diff Detail

Repository
rABC Bitcoin ABC
Branch
fix_abc-finalize-block.py
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4193
Build 6452: Bitcoin ABC Buildbot (legacy)
Build 6451: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Fix nits in comments, and add a few assertions on alt_node

schancel added inline comments.
test/functional/abc-finalize-block.py
12 ↗(On Diff #6154)

I feel like we should standardize these somewhere?

deadalnix requested changes to this revision.Nov 29 2018, 16:13
deadalnix added inline comments.
test/functional/abc-finalize-block.py
113 ↗(On Diff #6154)

Is 200 really the block that is finalized ?

127 ↗(On Diff #6154)

It's usually not the best idea to do computation like this in tests. You are better off just checking this has the value you expect.

169 ↗(On Diff #6154)

Are you absolutely sure that 200 is the block that is finalized ?

This revision now requires changes to proceed.Nov 29 2018, 16:13

Make assertions more explicit regarding comments

deadalnix requested changes to this revision.Nov 29 2018, 17:38
deadalnix added inline comments.
test/functional/abc-finalize-block.py
118 ↗(On Diff #6171)

assert_equal(node.getbestblockhash(), tip)

120 ↗(On Diff #6171)

The finalized block is supposed to be 209

This revision now requires changes to proceed.Nov 29 2018, 17:38

Update the test to reflect the expected behavior.
The test will fail ATM.

jasonbcox requested changes to this revision.Nov 29 2018, 23:25
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
test/functional/abc-finalize-block.py
225 ↗(On Diff #6173)

is -> should be
The error message should always read in a way that reveals what the problem is. The message currently reads that the finalized block IS the alt_node's tip, which is clearly not the case.

This revision now requires changes to proceed.Nov 29 2018, 23:25
test/functional/abc-finalize-block.py
66 ↗(On Diff #6173)

It seems strange to me that the ordering of finalizing a particular block changes whether block 210 is invalid or valid-headers. I would expect behavior similar to the marked comment (see below) where finding a block forked from before a finalized block would be marked as invalid, same as marking a block as finalized invalidates prior forks.

Since this diff is building upon existing behavior, this should be fixed in a future diff.

208 ↗(On Diff #6173)

marking this line (see related comment)

Fabien planned changes to this revision.Nov 30 2018, 14:13
Fabien marked an inline comment as done.
Fabien added inline comments.
test/functional/abc-finalize-block.py
225 ↗(On Diff #6173)

I don't get your point, this message will only appear if the assertion is false, so the message looks correct.
Maybe you meant "Finalize block should not be alt_node's tip" ?

Anyway this whole test case makes no sense, I will update.

test/functional/abc-finalize-block.py
66 ↗(On Diff #6173)

There is really no point tracking down every single orphan that ever existed since the beginning of time. It only matter that we do so for chain that actually have more work.

test/functional/abc-finalize-block.py
125 ↗(On Diff #6173)

assert_equal(node.getbestblockhash(), tip)

Checking that the top block is at a certain height do not test anything useful and certainly do not test what is described in the comment above.

test/functional/abc-finalize-block.py
126 ↗(On Diff #6195)

This is not testing anything, really, it just makes the test more fragile. You want to check the best block is tip, regardless of tip's height.

128 ↗(On Diff #6195)

You want to test this is tip's parent, not at height 0209 in general.

133 ↗(On Diff #6195)

Same apply here. getblockhash can help.

Split the diff, this part only contains what the summary describes

test/functional/abc-finalize-block.py
225 ↗(On Diff #6173)

Oh yes. You're correct. Sorry for the confusion. "Finalize block should not be alt_node's tip" is more correct that the current message because the message is making an assumption of what the value could be. These kinds of messages age poorly as modifications to the test and underlying code may change the behavior.

Update assertion message

Fabien removed a child revision: Restricted Differential Revision.Nov 30 2018, 22:16
test/functional/abc-finalize-block.py
52 ↗(On Diff #6205)

The block is never accepted. in fact it is not even downloaded, as we can see by the state "valid-headers". The block is ignored as it is on a minority chain.

test/functional/abc-finalize-block.py
143 ↗(On Diff #6229)

You shouldn't need to use this. A wait for is preferable as it would ensure the magic is working by itself without the test having to shovel blocks back and forth.

This revision is now accepted and ready to land.Dec 2 2018, 23:13
This revision was automatically updated to reflect the committed changes.