Add test coverage for messages requesting invalid blocks
ClosedPublic
Actions

Authored by jasonbcox on Nov 21 2019, 17:18.

Details

Reviewers

deadalnix
Fabien

Group Reviewers

Restricted Project

Commits

rSTAGING67c715cd4553: Add test coverage for messages requesting invalid blocks
rABC67c715cd4553: Add test coverage for messages requesting invalid blocks

Summary

While digging around the net code and tests, I realized these cases are not covered.
There are related tests that cover similar code paths in p2p_fingerprint and others, but they
do not test this behavior explicitly.

Test Plan

test_runner.py abc-get-invalid-block

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jasonbcox created this revision.Nov 21 2019, 17:18

Herald added a reviewer: deadalnix. · View Herald TranscriptNov 21 2019, 17:18

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8217: Diff 14299.Nov 21 2019, 17:32

Fabien requested changes to this revision.Nov 22 2019, 08:07

Fabien added a subscriber: Fabien.

Fabien added inline comments.

test/functional/abc-get-invalid-block.py
30 ↗	(On Diff #14299)	You can use `MSG_BLOCK` from `messages` and define `MSG_CMPCT_BLOCK` to avoid magic values
66 ↗	(On Diff #14299)	Add `assert_equal(blocks[-1], node.getbestblockhash())` in between.

This revision now requires changes to proceed.Nov 22 2019, 08:07

Address feedback

Build Bitcoin-ABC / Diffs / Diff Testing started.

jasonbcox added inline comments.Nov 22 2019, 23:55

test/functional/abc-get-invalid-block.py
30 ↗	(On Diff #14299)	I think you mean GETBLOCKS as MSG_BLOCK is to send a block not request one. Also, there's no compact block equivalent message in net_processing that I see. I also don't see any appropriate wrapper for GETDATA that abstracts away these magic numbers. Plenty of other tests use this atm, so I don't think this is a huge issue.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8241: Diff 14341.Nov 23 2019, 00:03

jasonbcox added a child revision: Restricted Differential Revision.Nov 23 2019, 00:03

The description give no real information as to what this is actually doing or testing.

test/functional/abc-get-invalid-block.py
93 ↗	(On Diff #14341)	Time based tests are going to be both slow and flaky. This is not the approach you want to take.

Fabien requested changes to this revision.Nov 24 2019, 10:06

Fabien added inline comments.

test/functional/abc-get-invalid-block.py
30 ↗	(On Diff #14299)	It's not about the message but about the inventory type in getdata: https://en.bitcoin.it/wiki/Protocol_documentation#Inventory_Vectors. ATM only `MSG_TX` and `MSG_BLOCK` are defined in `messages.py` and their only callsite is in `mininode.py`. I agree the name is confusing when you don't have the context. If plenty of other tests use these magic values, then updating these other tests seems a better solution to me, despite out of scope for this diff.

This revision now requires changes to proceed.Nov 24 2019, 10:06

jasonbcox added inline comments.Nov 25 2019, 21:34

test/functional/abc-get-invalid-block.py
30 ↗	(On Diff #14299)	Oh gotcha.

Use assert_debug_log instead of time.sleep
Refactor to prevent needing to jump around the code just to read and understand everything.
Add MSG_CMPCTBLOCK to messages.py
Improve perf from ~25 seconds to ~6 seconds