Add CScriptNum decode python implementation in functional suite by instagibbs · Pull Request #14816 · bitcoin/bitcoin

instagibbs · 2018-11-27T15:22:53Z

I needed this for reasons and thought it'd be good to upsteam it.

promag

Restarted appveyor.

I needed this for reasons and thought it'd be good to upsteam it.

Could add one reason? otherwise this is dead code.

promag · 2018-11-27T16:17:23Z

test/functional/test_framework/script.py

Could discard these unrelated changes.

it reduces the verbosity of it, I can remove but seemed better to me.

I understand but IMO it's better without.

already removed

Yeah I know :)

promag · 2018-11-27T16:18:22Z

test/functional/test_framework/script.py

nit, remove extra space.

instagibbs · 2018-11-27T16:25:59Z

@promag it can(should?) be used in tests in the future. Allows extraction of blockheight from the coinbase transaction, for example.

I can think a bit more where to stick this into a test, which I noted in the OP.

promag · 2018-11-27T16:28:37Z

@instagibbs I just think you could add one usage here.

instagibbs · 2018-11-27T16:45:31Z

Addressed issues, and added a single usage in a test.

promag

Thanks for adding the test, I think it makes this PR more complete. I'm leaving two nits for your consideration.

utACK 9f9bd43.

promag · 2018-11-28T09:57:02Z

test/functional/test_framework/script.py

nit, could move this to L401.

promag · 2018-11-28T09:57:42Z

test/functional/test_framework/script.py

nit, could remove else branch, and above just return 0 if len(value) == 0.

practicalswift · 2018-11-28T12:39:05Z

test/functional/test_framework/script.py

If @promag:s suggestion is not implemented: use enumerate which is more idiomatic :-)

practicalswift · 2018-11-28T12:39:41Z

test/functional/test_framework/script.py

If @promag:s suggestions is not implemented: Use if not value: which is more idiomatic

fwiw I wouldn't know hot to read that suggestion properly, sorry

Yes leave it as is - it was just a nit :-)

I just meant to say that if sequence: and if not sequence: are very common Python idioms for checking non-emptiness and emptiness of sequences. Perhaps this is not the ideal case for my suggestion since it is not obvious from the variable name that value is a sequence :-)

instagibbs · 2018-11-28T16:17:56Z

comments addressed, added multi-byte and negative round-trip testing as well

Empact · 2018-11-27T20:23:16Z

test/functional/mining_basic.py

typo: tip/trip

scravy

utACK

promag · 2018-11-30T23:21:00Z

utACK 2012d4d, nice to have more usages.

maguayo

utACK

laanwj · 2018-12-04T10:27:36Z

utACK 2012d4d

…al suite 2012d4d Add CScriptNum decode python implementation in functional suite (Gregory Sanders) Pull request description: I needed this for reasons and thought it'd be good to upsteam it. Tree-SHA512: 6ea89fa2a5f5a7759ba722f2b4ed5cd6423ebfff4e83ac8b8b5c935e6aa479684e626c5f41fa020816d2a9079a99af5564e30808594d5c13e3b51ec9b474926d

Migrates the CScriptNum decode tests into a unit test, and moved some changes made in bitcoin#14816. Made possible by the integration of test_framework unit testing in bitcoin#18576. Further extends the original test with larger ints, similar to the scriptnum_tests.cpp file. Adds test to blocktools.py testing fn create_coinbase() with CScriptNum decode.

…n script.py 7daffc6 [test] CScriptNum Decode Check as Unit Tests (Gillian Chu) Pull request description: The CScriptNum test (#14816) is a roundtrip test of the test framework. Thus, it would be better suited as a unit test. This is now possible with the introduction of the unit test module for the functional tests. See #18576. This PR: 1. Refactors the CScriptNum tests into 2 unit tests, one in script.py and one in blocktools.py. 2. Extends the script.py CScriptNum test to trial larger numbers. ACKs for top commit: laanwj: ACK 7daffc6 Tree-SHA512: 17a04a4bfff1b1817bfc167824c679455d9e06e6e0164c00a7e44f8aa5041c5f5080adcc1452fd80ba1a6d8409f976c982bc481d686c434edf97a5893a32a436

… test in script.py 7daffc6 [test] CScriptNum Decode Check as Unit Tests (Gillian Chu) Pull request description: The CScriptNum test (bitcoin#14816) is a roundtrip test of the test framework. Thus, it would be better suited as a unit test. This is now possible with the introduction of the unit test module for the functional tests. See bitcoin#18576. This PR: 1. Refactors the CScriptNum tests into 2 unit tests, one in script.py and one in blocktools.py. 2. Extends the script.py CScriptNum test to trial larger numbers. ACKs for top commit: laanwj: ACK 7daffc6 Tree-SHA512: 17a04a4bfff1b1817bfc167824c679455d9e06e6e0164c00a7e44f8aa5041c5f5080adcc1452fd80ba1a6d8409f976c982bc481d686c434edf97a5893a32a436

…al suite Summary: 2012d4df2 Add CScriptNum decode python implementation in functional suite (Gregory Sanders) Pull request description: I needed this for reasons and thought it'd be good to upsteam it. Tree-SHA512: 6ea89fa2a5f5a7759ba722f2b4ed5cd6423ebfff4e83ac8b8b5c935e6aa479684e626c5f41fa020816d2a9079a99af5564e30808594d5c13e3b51ec9b474926d Backport of Core [[bitcoin/bitcoin#14816 | PR14816]] Test Plan: `test_runner.py mining_basic` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D6713

Summary: > Migrates the CScriptNum decode tests into a unit test, and moved some > changes made in [[bitcoin/bitcoin#14816 | core#14816]]. Made possible by the integration of > test_framework unit testing in [[bitcoin/bitcoin#18576 | core#18576]]. Further extends the original > test with larger ints, similar to the scriptnum_tests.cpp file. Adds > test to blocktools.py testing fn create_coinbase() with CScriptNum > decode. Backport note: The functional tests are usually executed individually via a `subprocess`. This means the `get_srcdir()` function in cdefs.py is able to find the proper source directory, because the parent directory of the python process is the project root that contains `src/`. But when we run the new test_framework unit test, the process currently running is `test_runner.py` itself which is located in the build directory. So cdefs.py finds the wrong `src/` directory, the one located in the build directory, which does not contain `consensus.h`. I fixed this by setting the environment variable `SRCDIR` in `test_runner.py` before calling the unittest `unittest.TestLoader().loadTestsFromName` machinery which causes `cdefs.py` to be imported. This is a backport of [[bitcoin/bitcoin#19082 | core#19082]] Test Plan: `ninja check-functional` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9276

…unctional suite 2012d4d Add CScriptNum decode python implementation in functional suite (Gregory Sanders) Pull request description: I needed this for reasons and thought it'd be good to upsteam it. Tree-SHA512: 6ea89fa2a5f5a7759ba722f2b4ed5cd6423ebfff4e83ac8b8b5c935e6aa479684e626c5f41fa020816d2a9079a99af5564e30808594d5c13e3b51ec9b474926d

fanquake added the Tests label Nov 27, 2018

promag reviewed Nov 27, 2018

View reviewed changes

instagibbs force-pushed the functional_cscriptnum_decode branch 3 times, most recently from 920ed73 to 9f9bd43 Compare November 27, 2018 16:44

promag reviewed Nov 28, 2018

View reviewed changes

practicalswift reviewed Nov 28, 2018

View reviewed changes

instagibbs force-pushed the functional_cscriptnum_decode branch from 9f9bd43 to dd27c1c Compare November 28, 2018 16:16

Empact reviewed Nov 28, 2018

View reviewed changes

Add CScriptNum decode python implementation in functional suite

2012d4d

instagibbs force-pushed the functional_cscriptnum_decode branch from dd27c1c to 2012d4d Compare November 29, 2018 13:31

scravy approved these changes Nov 29, 2018

View reviewed changes

maguayo approved these changes Dec 1, 2018

View reviewed changes

laanwj merged commit 2012d4d into bitcoin:master Dec 4, 2018

instagibbs mentioned this pull request May 13, 2020

tests: implement base58_decode #18965

Merged

maflcko mentioned this pull request May 13, 2020

test: Make scriptnum test a unit test maflcko/bitcoin-core#24

Closed

maflcko mentioned this pull request May 27, 2020

test: Moved the CScriptNum asserts into the unit test in script.py #19082

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Conversation

instagibbs commented Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Nov 27, 2018

Uh oh!

instagibbs commented Nov 27, 2018

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Nov 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scravy left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Nov 30, 2018

Uh oh!

maguayo left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Dec 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

instagibbs commented Nov 27, 2018 •

edited

Loading

promag Nov 27, 2018 •

edited

Loading

instagibbs commented Nov 27, 2018 •

edited

Loading

promag left a comment •

edited

Loading

practicalswift Nov 28, 2018 •

edited

Loading

practicalswift Nov 28, 2018 •

edited

Loading

practicalswift Nov 28, 2018 •

edited

Loading