Page MenuHomePhabricator

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

Authored by Fabien on Sun, Nov 25, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Fabien updated this revision to Diff 6154.Wed, Nov 28, 18:41

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

schancel accepted this revision.Wed, Nov 28, 21:07
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.Thu, Nov 29, 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.Thu, Nov 29, 16:13
Fabien updated this revision to Diff 6167.Thu, Nov 29, 17:07

Make assertions more explicit regarding comments

Fabien updated this revision to Diff 6168.Thu, Nov 29, 17:10

Rebase

Fabien updated this revision to Diff 6170.Thu, Nov 29, 17:21

Rebase

Fabien updated this revision to Diff 6171.Thu, Nov 29, 17:23

Fix nit in comment

deadalnix requested changes to this revision.Thu, Nov 29, 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.Thu, Nov 29, 17:38
Fabien updated this revision to Diff 6173.Thu, Nov 29, 18:23

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

jasonbcox requested changes to this revision.Thu, Nov 29, 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.Thu, Nov 29, 23:25
jasonbcox added inline comments.Thu, Nov 29, 23:30
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.Fri, Nov 30, 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.

deadalnix added inline comments.Fri, Nov 30, 14:40
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.

Fabien updated this revision to Diff 6195.Fri, Nov 30, 15:18

Fix bad test case

deadalnix added inline comments.Fri, Nov 30, 15:56
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.

Fabien updated this revision to Diff 6198.Fri, Nov 30, 16:11

Improve assertions

deadalnix added inline comments.Fri, Nov 30, 16:13
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.

Fabien updated this revision to Diff 6199.Fri, Nov 30, 16:43

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

Fabien edited the summary of this revision. (Show Details)Fri, Nov 30, 16:44
Fabien updated this revision to Diff 6200.Fri, Nov 30, 16:47

Add missing assertion

jasonbcox added inline comments.Fri, Nov 30, 18:59
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.

Fabien updated this revision to Diff 6205.Fri, Nov 30, 20:37

Update assertion message

Fabien removed a child revision: Restricted Differential Revision.Fri, Nov 30, 22:16
deadalnix added inline comments.Sat, Dec 1, 23:11
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.

Fabien updated this revision to Diff 6229.Sun, Dec 2, 00:56

Update bad comment

deadalnix accepted this revision.Sun, Dec 2, 21:17
deadalnix added inline comments.Sun, Dec 2, 21:19
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.

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