[tests] Add messages.py #11648

jnewbery · 2017-11-09T20:52:35Z

Second part of #11518.

Moves the primitive Bitcoin datastructures and message classes into their own module, and tidies up the mininode.py module.

First commit is almost entirely move-only
Second commit is mostly move-only, but also does a little tidying.

fanquake · 2017-11-10T01:23:46Z

One travis failure in net.py:

2017-11-10 01:10:17.553000 TestFramework.node0 (DEBUG): RPC successfully started
2017-11-10 01:10:17.555000 TestFramework.node1 (DEBUG): RPC successfully started
2017-11-10 01:10:18.043000 TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/test_framework.py", line 120, in main
    self.run_test()
  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/net.py", line 27, in run_test
    self._test_getnettotals()
  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/net.py", line 54, in _test_getnettotals
    assert_equal(before['bytesrecv_per_msg']['pong'] + 32, after['bytesrecv_per_msg']['pong'])
  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/util.py", line 38, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(64 == 32)
2017-11-10 01:10:18.044000 TestFramework (INFO): Stopping nodes
2017-11-10 01:10:18.044000 TestFramework.node0 (DEBUG): Stopping node

meshcollider · 2017-11-10T06:50:22Z

@fanquake looks unrelated, restarted

laanwj · 2017-11-10T10:11:11Z

Thanks for trying to clean things up, but I think we need more categorization here; bitcoin P2P message types are clearly a different kind of primitives than consensus data structures such as CTransaction CTXin etc. Only the second group is called "primitives" in the C++ code.

ryanofsky · 2017-11-10T11:35:37Z

Maybe just call it something like serialize.py or messages.py.

mininode.py wildcard imports all names from primitives.py. This is to avoid having to change all test scripts that import from mininode.py.

Mostly move only. Adds a few extra comments.

jnewbery · 2017-11-10T17:00:46Z

I think we need more categorization here; bitcoin P2P message types are clearly a different kind of primitives than consensus data structures

I tried splitting this further into primitives.py (for the primitive consensus data structures) and messages.py (for the p2p message data structures), but the resulting layout didn't make much sense to me so I reverted it. The main motivation for this PR was to split out the mininode functionality from the underlying data structures. I've renamed the new module messages.py as suggested by @ryanofsky.

If you think it's worth splitting further into consensus primitives and p2p message data structures, I can look at doing that in a future PR. It would end up looking something like https://github.com/jnewbery/bitcoin/tree/pr11648.2.

laanwj · 2017-11-11T07:37:59Z

@jnewbery Thanks. Calling it messages.py is enough. I don't think they need to be split further unless they're used seperately, which they're not. It was just that having two concepts of "primitives" between the tests and code was confusing to me.

practicalswift · 2017-11-14T21:26:21Z

utACK 1135c79

maflcko

utACK 1135c79

Verified that it is indeed move-only and/or fixups due to rename.
I am neutral on the overall change. Some prefer a huge file where it is easy to jump back and forth, others prefer multiple files that bundle into smaller sets of related classes and functions.

I think there is no single true way of organizing the files, but if the refactoring helps some greater goal, I am happy with the changes.

maflcko · 2017-11-16T18:39:05Z

test/functional/test_framework/messages.py

+MAX_INV_SZ = 50000
+MAX_BLOCK_BASE_SIZE = 1000000
+
+COIN = 100000000 # 1 btc in satoshis


nit: This is a conversion helper constant not used by any of the primitives or messages.

It's used in:

CTxOut.__repr__

CTransaction.is_valid

It's arguable where this belongs, the ideal place might be some bitcoin-specific constants header, but for now it makes sense to have it here.

You are right, I missed those. Looks fine now.

laanwj · 2017-11-17T12:17:00Z

utACK 1135c79

1135c79 [tests] Tidy up mininode.py module (John Newbery) f9cd9b1 [tests] Move test_framework Bitcoin primitives into separate module (John Newbery) Pull request description: Second part of #11518. Moves the primitive Bitcoin datastructures and message classes into their own module, and tidies up the mininode.py module. - First commit is almost entirely move-only - Second commit is mostly move-only, but also does a little tidying. Tree-SHA512: 5d74802677f1ab788e43188653106a96fffd9ab1fe3aa6a4eb94e5807de5dd5c8ee212296f45e8d16c7e3d95cfc4891677e812b7944bd3ab604e04b3b88aa06e

promag · 2017-11-17T15:13:56Z

test/functional/test_framework/messages.py

+    return sha256(sha256(s))
+
+def ser_compact_size(l):
+    r = b""


Unnecessary?

Generally, it is easier to verify move-only-ness when no code changes are made. We've had pull requests in the past that did "all the refactoring" in one go and introduced bugs.

IMO both ways are acceptable, as long as the commits that move code are separate from those that make changes.

In any case, moving code around doesn't obligate someone to also fix all the problems it has, in style or other things. Can be done later.

Yes, happy to see this fixed up when you touch that function for other reasons. Not sure if this change warrants a pull request on itself.

…arate module I manually recreated this commit, since we have A LOT of conflicts in mininode. However since it is primarily just a move, it was pretty easy to recreate Signed-off-by: Pasta <pasta@dashboost.org>

* [tests] Tidy up mininode.py module Mostly move only. Adds a few extra comments. * bitcoin#11648 [tests] Move test_framework Bitcoin primitives into separate module I manually recreated this commit, since we have A LOT of conflicts in mininode. However since it is primarily just a move, it was pretty easy to recreate Signed-off-by: Pasta <pasta@dashboost.org> * add import to messages.py Signed-off-by: Pasta <pasta@dashboost.org> * move import from mininode.py to messages.py Signed-off-by: Pasta <pasta@dashboost.org> * fix test failure Signed-off-by: Pasta <pasta@dashboost.org> * remove empty line at top of messages.py Signed-off-by: pasta <pasta@dashboost.org> * alphabetize MESSAGEMAP seperated by if it is dash specific or not Signed-off-by: pasta <pasta@dashboost.org> * remove accidentally added feefilter message Signed-off-by: pasta <pasta@dashboost.org> * Add missing getmnlistd/mnlistdiff messages to MESSAGEMAP Co-authored-by: John Newbery <john@johnnewbery.com> Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

jnewbery mentioned this pull request Nov 9, 2017

[tests] [demonstration] Simplify/clarify the NodeConn/NodeConnCB mininode classes #11518

Closed

6 tasks

fanquake added the Tests label Nov 9, 2017

jnewbery force-pushed the add_primitives_py branch from 738f1da to 46fd472 Compare November 10, 2017 16:47

jnewbery added 2 commits November 10, 2017 11:56

[tests] Move test_framework Bitcoin primitives into separate module

f9cd9b1

mininode.py wildcard imports all names from primitives.py. This is to avoid having to change all test scripts that import from mininode.py.

[tests] Tidy up mininode.py module

1135c79

Mostly move only. Adds a few extra comments.

jnewbery force-pushed the add_primitives_py branch from 46fd472 to 1135c79 Compare November 10, 2017 16:58

jnewbery changed the title ~~[tests] Add primitives.py~~ [tests] Add messages.py Nov 10, 2017

jnewbery mentioned this pull request Nov 14, 2017

tests: Remove unused mininode functions {ser,deser}_int_vector(...). Remove unused imports. #11683

Merged

laanwj requested a review from maflcko November 16, 2017 12:56

maflcko added the Refactoring label Nov 16, 2017

maflcko reviewed Nov 16, 2017

View reviewed changes

laanwj merged commit 1135c79 into bitcoin:master Nov 17, 2017

promag reviewed Nov 17, 2017

View reviewed changes

jnewbery deleted the add_primitives_py branch November 17, 2017 16:15

UdjinM6 mentioned this pull request Oct 3, 2019

Backport orphan TX tests and dependent PRs dashpay/dash#3127

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[tests] Add messages.py #11648

[tests] Add messages.py #11648

Uh oh!

Conversation

jnewbery commented Nov 9, 2017

Uh oh!

fanquake commented Nov 10, 2017

Uh oh!

meshcollider commented Nov 10, 2017

Uh oh!

laanwj commented Nov 10, 2017

Uh oh!

ryanofsky commented Nov 10, 2017

Uh oh!

jnewbery commented Nov 10, 2017

Uh oh!

laanwj commented Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Nov 14, 2017

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 16, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj commented Nov 17, 2017

Uh oh!

promag Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

laanwj commented Nov 11, 2017 •

edited

Loading