Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add p2p-fullblocktest.py #6523

Merged
merged 1 commit into from Aug 24, 2015
Merged

Add p2p-fullblocktest.py #6523

merged 1 commit into from Aug 24, 2015

Conversation

@casey
Copy link
Contributor

casey commented Aug 5, 2015

This is a start at implementing FullBlockTestGenerator.java in python. Almost all the work is by @sdaftuar, I just did some final cleanup here and there. The main test is p2p-fullblocktest.py, and should be pretty easy to understand,

It currently implements only a few of the tests from FullBlockTestGenerator.java, but it's a start.

create_coinbase() now takes a height argument, so there are some changes to unrelated tests, as well as some new functionality added to the testing framework, to support the new tests.

@jgarzik
Copy link
Contributor

jgarzik commented Aug 6, 2015

+1

@laanwj laanwj added the Tests label Aug 6, 2015
@laanwj
Copy link
Member

laanwj commented Aug 6, 2015

Awesome.

@gavinandresen
Copy link
Contributor

gavinandresen commented Aug 6, 2015

Very nice!

But in the "probably not your fault" category, it crashes on my OSX machine calling the openssl BN_bin2bn function:

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x00000000296707cc

VM Regions Near 0x296707cc:
--> 
    __TEXT                 0000000107e1d000-0000000107e1f000 [    8K] r-x/rwx SM=COW  /usr/local/Cellar/python/2.7.10_2/Frameworks/Python.framework/Versions/2.7/Resources/Python.app/Contents/MacOS/Python

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libcrypto.0.9.8.dylib           0x00007fff93af4446 BN_bin2bn + 118
1   _ctypes.so                      0x00000001087d47c7 ffi_call_unix64 + 79
2   _ctypes.so                      0x00000001087d4fe6 ffi_call + 818
3   _ctypes.so                      0x00000001087d070b _ctypes_callproc + 867
4   _ctypes.so                      0x00000001087cab91 PyCFuncPtr_call + 1100
5   org.python.python               0x0000000107e31ad7 PyObject_Call + 99
6   org.python.python               0x0000000107eade7f PyEval_EvalFrameEx + 11417
7   org.python.python               0x0000000107eb16d1 fast_function + 262
8   org.python.python               0x0000000107eae553 PyEval_EvalFrameEx + 13165
9   org.python.python               0x0000000107eaafb4 PyEval_EvalCodeEx + 1387
10  org.python.python               0x0000000107e4fbf5 function_call + 352
11  org.python.python               0x0000000107e31ad7 PyObject_Call + 99
12  org.python.python               0x0000000107e3c962 instancemethod_call + 174
13  org.python.python               0x0000000107e31ad7 PyObject_Call + 99
14  org.python.python               0x0000000107e78f2d slot_tp_init + 64
15  org.python.python               0x0000000107e746e7 type_call + 182
16  org.python.python               0x0000000107e31ad7 PyObject_Call + 99
17  org.python.python               0x0000000107eade7f PyEval_EvalFrameEx + 11417
18  org.python.python               0x0000000107eaafb4 PyEval_EvalCodeEx + 1387
19  org.python.python               0x0000000107eaaa43 PyEval_EvalCode + 54
20  org.python.python               0x0000000107eca816 run_mod + 53
21  org.python.python               0x0000000107eca8b9 PyRun_FileExFlags + 133
22  org.python.python               0x0000000107eca3f9 PyRun_SimpleFileExFlags + 711
23  org.python.python               0x0000000107edbe09 Py_Main + 3057
24  libdyld.dylib                   0x00007fff950c25c9 start + 1

OS Version:            Mac OS X 10.10.4 (14E46)
Code Type:             X86-64 (Native)

Probably the same issue:
petertodd/python-bitcoinlib#30

@kanzure
kanzure reviewed Aug 6, 2015
View changes
qa/rpc-tests/test_framework/key.py Outdated
ssl.EC_KEY_new_by_curve_name.restype = ctypes.c_void_p
ssl.EC_KEY_new_by_curve_name.errcheck = _check_result

class CECKey:

This comment has been minimized.

Copy link
@kanzure

kanzure Aug 6, 2015

Contributor

nit: classes should at least subclass some base class such as object

This comment has been minimized.

Copy link
@casey

casey Aug 7, 2015

Author Contributor

fixed

@kanzure
kanzure reviewed Aug 6, 2015
View changes
qa/rpc-tests/test_framework/key.py Outdated
NID_secp256k1 = 714 # from openssl/obj_mac.h

# Thx to Sam Devlin for the ctypes magic 64-bit fix.
def _check_result (val, func, args):

This comment has been minimized.

Copy link
@kanzure

kanzure Aug 6, 2015

Contributor

Syntax nit: extra space after the function name, doesn't match style of other functions in the file. Also, docstrings would be nice for every function.

This comment has been minimized.

Copy link
@casey

casey Aug 7, 2015

Author Contributor

fixed

@fanquake
fanquake reviewed Aug 6, 2015
View changes
qa/rpc-tests/README.md Outdated
-----------------------------------------
- conflictedbalance.sh : More testing of malleable transaction handling


This comment has been minimized.

Copy link
@fanquake

fanquake Aug 6, 2015

Member

Why are you adding the note about conflictedbalance, it was removed in #6428

This comment has been minimized.

Copy link
@casey

casey Aug 7, 2015

Author Contributor

Whoops, bad merge, fixed.

@casey
Copy link
Contributor Author

casey commented Aug 7, 2015

@gavinandresen Unfortunately, I haven't made any progress on fixing this. I can reproduce it on OS X, as well as on my own Arch Linux dev box, but not on Debian.

On Arch I can get it to work by compiling python from a tarball (instead of using the version from arch's package manager), so I think it's a problem with python/ctypes, and not something weird in the code or libssl.

It does seem to be the same issue as petertodd/python-bitcoinlib#30

It might be a good idea to still merge this, assuming it works on travis and for people not on OS X, so we can start building up a good series of block acceptance tests.

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Aug 9, 2015

While I love the idea of ditching the existing comparison tool, rewriting it as-is seems like a very bad idea. The existing one is very hard to work with in large part because of the way the tests are generated - it's one big monolithic step-by-step. It really needs to separate out into something where individual tests are in individual functions/modules/whatever. Obviously running the tests in one run is probably the only way you can get it to run in reasonable time, but please don't duplicate the existing code.

On August 7, 2015 10:10:51 PM GMT+02:00, Casey Rodarmor notifications@github.com wrote:

@gavinandresen Unfortunately, I haven't made any progress on fixing
this. I can reproduce it on OS X, as well as on my own Arch Linux dev
box, but not on Debian.

On Arch I can get it to work by compiling python from a tarball
(instead of using the version from arch's package manager), so I think
it's a problem with python/ctypes, and not something weird in the code
or libssl.

It does seem to be the same issue as petertodd/python-bitcoinlib#30

It might be a good idea to still merge this, assuming it works on
travis and for people not on OS X, so we can start building up a good
series of block acceptance tests.


Reply to this email directly or view it on GitHub:
#6523 (comment)

@jgarzik
Copy link
Contributor

jgarzik commented Aug 10, 2015

A drop-in replacement in a more accessible language and format that behaved equivalently would seem to be a net win.

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Aug 10, 2015

Sure, but if you're gonna go to the effort to rewrite it, might as well rewrite it better instead of transliterating a very fragile monolithic chunk of code.

On August 10, 2015 5:10:39 AM GMT+02:00, Jeff Garzik notifications@github.com wrote:

A drop-in replacement in a more accessible language and format that
behaved equivalently would seem to be a net win.


Reply to this email directly or view it on GitHub:
#6523 (comment)

@jgarzik
Copy link
Contributor

jgarzik commented Aug 10, 2015

"might as well" is in the eye of the do-er... In the absence of someone coming along and doing an even better rewrite, an incremental net-win should not be blocked because it lacks perfection :)

@sipa
Copy link
Member

sipa commented Aug 10, 2015

Agree with @jgarzik. I think that just having a python version available will lower the barrier for extra tests using it.

@jonasschnelli
Copy link
Member

jonasschnelli commented Aug 10, 2015

+1
utACK.
Travis did run this test successfully (https://travis-ci.org/bitcoin/bitcoin/jobs/74625657#L3538).

@casey
Copy link
Contributor Author

casey commented Aug 10, 2015

I just pushed changes which should fix the crashes on Arch and OS X.

@gavinandresen
Copy link
Contributor

gavinandresen commented Aug 10, 2015

Tests successful on OSX, ACK from me.

@petertodd
Copy link
Contributor

petertodd commented Aug 18, 2015

@casey FWIW I just ported those fixes to python-bitcoinlib: petertodd/python-bitcoinlib#77

@laanwj
Copy link
Member

laanwj commented Aug 21, 2015

Needs rebase after #6509 (Fix race condition on test node shutdown)

@casey
Copy link
Contributor Author

casey commented Aug 21, 2015

Rebased onto master.

@fanquake
Copy link
Member

fanquake commented Aug 24, 2015

Retested to verify that the OSX crash has been fixed.

@laanwj laanwj merged commit 0ce7398 into bitcoin:master Aug 24, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Aug 24, 2015
0ce7398 Add p2p-fullblocktest.py (Casey Rodarmor)
@luke-jr
Copy link
Member

luke-jr commented Nov 15, 2015

In the future, please don't make changes to interfaces that quietly and subtley break things. In this case, create_coinbase's first argument changed meaning while quietly taking the same values and not throwing an error (or maintaining the previous functionality). As a result, I spent time trying to figure out why the CLTV RPC test failed on 0.11.2 when it had nothing to do with CLTV. :(

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Nov 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.