Page MenuHomePhabricator

Fix test framework buffering issue which disturbed abc-p2p-fullblocktest.py
AbandonedPublic

Authored by freetrader on Jun 28 2017, 07:30.

Details

Reviewers
deadalnix
sickpig
kyuupichan
awemany
Group Reviewers
Restricted Project
Summary

T53 manifested as a memory exhaustion problem on Travis, but the underlying
problem was that the abc-p2p-fullblocktest was producing enormous outputs
due to having entered an anomalous condition based on bad p2p communication
between the node and the test framework (messages that were broken up
could not be properly re-assembled - this interfered seriously as
blocks and transactions increased in size and were read in several parts).

This commit reworks the way that the test framework reads, buffers and
processes p2p messages from the node, eliminating this re-assembly problem
that plagued abc-p2p-fullblocktest (and perhaps some spurious problems on
other tests too).

A new helper class, P2PBuffer, is introduced in p2pbuffer.py .
This class does the buffering based on separators ("network magic") and
emits what should be integral single messages through its
get_next_message interface.

The test framework derives a MininodeP2PBuffer from this class.

Test Plan

qa/pull-tester/rpc-tests -extended
qa/rpc-tests/abc-p2p-fullblocktest.py # repetitive tests
python3 qa/rpc-tests/test_framework/p2pbuffer.py # run Python unit tests (doctests) for p2pbuffer

Diff Detail

Repository
rABC Bitcoin ABC
Branch
framework_buffering_fix1_travistest
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 431
Build 431: arc lint + arc unit

Event Timeline

Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJun 28 2017, 07:30
qa/rpc-tests/test_framework/mininode.py
1645

I'll do an update to capitalize errant comments.

freetrader edited the test plan for this revision. (Show Details)

Here what I propose. You change got-data in mininode to do the following:

  1. If the message doesn't have the right magic at its start, error
  2. If the buffer is shorter than the expected length of the message, wait longer (for handle_read to be called again, which in turn will call get_data
  3. If the buffer is >= than the expected length, parse the slice we parse a message and assign the extra bytes back into the buffer.
  4. Call got_message with the message we just decoded.
  5. If the buffer is not empty, do it again.

Here you go, problem solved.

qa/rpc-tests/test_framework/mininode.py
44

This class doesn't looks like it is very useful.

1679

This seems to be the core of the issue. I'm a bit puzzled why we need a diff changing hundreds of lines of code to change this.

1745

This is a good refactoring, but unrelated to that change.

1788

Use local variable.

deadalnix requested changes to this revision.Jun 28 2017, 08:13
This revision now requires changes to proceed.Jun 28 2017, 08:13

I think D284 supersedes this.

Agreed, D284 code change is simpler and appears to be just as stable.

Therefore abandoning this one.