Page MenuHomePhabricator

Add test coverage for messages requesting invalid blocks
ClosedPublic

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

Details

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
Branch
dos-test
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 8217
Build 14460: Default Diff Build & Tests
Build 14459: arc lint + arc unit

Event Timeline

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

You can use MSG_BLOCK from messages and define MSG_CMPCT_BLOCK to avoid magic values

66

Add assert_equal(blocks[-1], node.getbestblockhash()) in between.

This revision now requires changes to proceed.Nov 22 2019, 08:07
test/functional/abc-get-invalid-block.py
30

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.

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

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
test/functional/abc-get-invalid-block.py
30

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
This revision is now accepted and ready to land.Nov 26 2019, 10:19