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 CScriptNum decode python implementation in functional suite #14816

Merged
merged 1 commit into from Dec 4, 2018

Conversation

Projects
None yet
8 participants
@instagibbs
Copy link
Member

commented Nov 27, 2018

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

@fanquake fanquake added the Tests label Nov 27, 2018

@promag
Copy link
Member

left a comment

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.

test/functional/test_framework/script.py Outdated
@@ -371,11 +371,12 @@ def __init__(self, d=0):

@staticmethod
def encode(obj):
val = obj.value

This comment has been minimized.

Copy link
@promag

promag Nov 27, 2018

Member

Could discard these unrelated changes.

This comment has been minimized.

Copy link
@instagibbs

instagibbs Nov 27, 2018

Author Member

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

This comment has been minimized.

Copy link
@promag

promag Nov 28, 2018

Member

I understand but IMO it's better without.

This comment has been minimized.

Copy link
@instagibbs

instagibbs Nov 28, 2018

Author Member

already removed

This comment has been minimized.

Copy link
@promag

promag Nov 28, 2018

Member

Yeah I know :)

test/functional/test_framework/script.py Outdated
num_mask = (2**(len(value)*8) - 1) >> 1
if len(value) == 0:
return 0
else :

This comment has been minimized.

Copy link
@promag

promag Nov 27, 2018

Member

nit, remove extra space.

This comment has been minimized.

Copy link
@instagibbs

instagibbs Nov 27, 2018

Author Member

done

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2018

@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

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

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

@instagibbs instagibbs force-pushed the instagibbs:functional_cscriptnum_decode branch 3 times, most recently Nov 27, 2018

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2018

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

@promag
Copy link
Member

left a comment

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

utACK 9f9bd43.

test/functional/test_framework/script.py Outdated
# We assume valid push_size and minimal encoding
value = vch[1:]
# Mask for all but the highest result bit
num_mask = (2**(len(value)*8) - 1) >> 1

This comment has been minimized.

Copy link
@promag

promag Nov 28, 2018

Member

nit, could move this to L401.

test/functional/test_framework/script.py Outdated
num_mask = (2**(len(value)*8) - 1) >> 1
if len(value) == 0:
return 0
else:

This comment has been minimized.

Copy link
@promag

promag Nov 28, 2018

Member

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

test/functional/test_framework/script.py Outdated
return 0
else:
result = 0
for i in range(len(value)):

This comment has been minimized.

Copy link
@practicalswift

practicalswift Nov 28, 2018

Member

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

value = vch[1:]
# Mask for all but the highest result bit
num_mask = (2**(len(value)*8) - 1) >> 1
if len(value) == 0:

This comment has been minimized.

Copy link
@practicalswift

practicalswift Nov 28, 2018

Member

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

This comment has been minimized.

Copy link
@instagibbs

instagibbs Nov 28, 2018

Author Member

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

This comment has been minimized.

Copy link
@practicalswift

practicalswift Nov 28, 2018

Member

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 instagibbs force-pushed the instagibbs:functional_cscriptnum_decode branch Nov 28, 2018

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2018

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

test/functional/mining_basic.py Outdated
# sequence numbers must not be max for nLockTime to have effect
coinbase_tx.vin[0].nSequence = 2 ** 32 - 2
coinbase_tx.rehash()

# round-tip the encoded bip34 block height commitment

This comment has been minimized.

Copy link
@Empact

Empact Nov 28, 2018

Member

typo: tip/trip

This comment has been minimized.

Copy link
@instagibbs

instagibbs Nov 29, 2018

Author Member

fixed

@instagibbs instagibbs force-pushed the instagibbs:functional_cscriptnum_decode branch to 2012d4d Nov 29, 2018

@scravy

scravy approved these changes Nov 29, 2018

Copy link
Contributor

left a comment

utACK

@promag

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

utACK 2012d4d, nice to have more usages.

@maguayo

maguayo approved these changes Dec 1, 2018

Copy link

left a comment

utACK

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

utACK 2012d4d

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Dec 4, 2018

Merge #14816: Add CScriptNum decode python implementation in function…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.