Add CScriptNum decode python implementation in functional suite #14816

pull instagibbs wants to merge 1 commits into bitcoin:master from instagibbs:functional_cscriptnum_decode changing 2 files +26 −2
  1. instagibbs commented at 3:22 pm on November 27, 2018: member
    I needed this for reasons and thought it’d be good to upsteam it.
  2. fanquake added the label Tests on Nov 27, 2018
  3. in test/functional/test_framework/script.py:374 in 104a6bdd31 outdated 
    370@@ -371,11 +371,12 @@ def __init__(self, d=0):
371 
372     @staticmethod
373     def encode(obj):
374+        val = obj.value
    promag commented at 4:17 pm on November 27, 2018:
    Could discard these unrelated changes.
    instagibbs commented at 4:25 pm on November 27, 2018:
    it reduces the verbosity of it, I can remove but seemed better to me.
    promag commented at 4:34 pm on November 28, 2018:
    I understand but IMO it’s better without.
    instagibbs commented at 4:46 pm on November 28, 2018:
    already removed
    promag commented at 4:54 pm on November 28, 2018:
    Yeah I know :)
  4. in test/functional/test_framework/script.py:396 in 104a6bdd31 outdated 
    392+        value = vch[1:]
393+        # Mask for all but the highest result bit
394+        num_mask = (2**(len(value)*8) - 1) >> 1
395+        if len(value) == 0:
396+            return 0
397+        else :
    promag commented at 4:18 pm on November 27, 2018:
    nit, remove extra space.
    instagibbs commented at 4:43 pm on November 27, 2018:
    done
  5. promag commented at 4:21 pm on November 27, 2018: member

    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.

  6. instagibbs commented at 4:25 pm on November 27, 2018: member

    @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.

  7. promag commented at 4:28 pm on November 27, 2018: member
    @instagibbs I just think you could add one usage here.
  8. instagibbs force-pushed on Nov 27, 2018
  9. instagibbs force-pushed on Nov 27, 2018
  10. instagibbs force-pushed on Nov 27, 2018
  11. instagibbs commented at 4:45 pm on November 27, 2018: member
    Addressed issues, and added a single usage in a test.
  12. in test/functional/mining_basic.py:74 in 9f9bd43026 outdated 
    70+        coinbase_tx = create_coinbase(height=next_height)
71         # sequence numbers must not be max for nLockTime to have effect
72         coinbase_tx.vin[0].nSequence = 2 ** 32 - 2
73         coinbase_tx.rehash()
74 
75+        # round-tip the encoded bip34 block height commitment
    Empact commented at 8:23 pm on November 27, 2018:
    typo: tip/trip
    instagibbs commented at 1:32 pm on November 29, 2018:
    fixed
  13. in test/functional/test_framework/script.py:393 in 9f9bd43026 outdated 
    384@@ -385,6 +385,23 @@ def encode(obj):
385             r[-1] |= 0x80
386         return bytes([len(r)]) + r
387 
388+    @staticmethod
389+    def decode(vch):
390+        # We assume valid push_size and minimal encoding
391+        value = vch[1:]
392+        # Mask for all but the highest result bit
393+        num_mask = (2**(len(value)*8) - 1) >> 1
    promag commented at 9:57 am on November 28, 2018:
    nit, could move this to L401.
  14. in test/functional/test_framework/script.py:396 in 9f9bd43026 outdated 
    391+        value = vch[1:]
392+        # Mask for all but the highest result bit
393+        num_mask = (2**(len(value)*8) - 1) >> 1
394+        if len(value) == 0:
395+            return 0
396+        else:
    promag commented at 9:57 am on November 28, 2018:
    nit, could remove else branch, and above just return 0 if len(value) == 0.
  15. promag commented at 9:59 am on November 28, 2018: member

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

    utACK 9f9bd43.

  16. in test/functional/test_framework/script.py:398 in 9f9bd43026 outdated 
    393+        num_mask = (2**(len(value)*8) - 1) >> 1
394+        if len(value) == 0:
395+            return 0
396+        else:
397+            result = 0
398+            for i in range(len(value)):
    practicalswift commented at 12:39 pm on November 28, 2018:
    If @promag:s suggestion is not implemented: use enumerate which is more idiomatic :-)
  17. in test/functional/test_framework/script.py:393 in 9f9bd43026 outdated 
    389+    def decode(vch):
390+        # We assume valid push_size and minimal encoding
391+        value = vch[1:]
392+        # Mask for all but the highest result bit
393+        num_mask = (2**(len(value)*8) - 1) >> 1
394+        if len(value) == 0:
    practicalswift commented at 12:39 pm on November 28, 2018:
    If @promag:s suggestions is not implemented: Use if not value: which is more idiomatic
    instagibbs commented at 4:17 pm on November 28, 2018:
    fwiw I wouldn’t know hot to read that suggestion properly, sorry
    practicalswift commented at 7:26 pm on November 28, 2018:

    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 :-)

  18. instagibbs force-pushed on Nov 28, 2018
  19. instagibbs commented at 4:17 pm on November 28, 2018: member
    comments addressed, added multi-byte and negative round-trip testing as well
  20. Add CScriptNum decode python implementation in functional suite 2012d4df23
  21. instagibbs force-pushed on Nov 29, 2018
  22. scravy approved
  23. scravy commented at 1:35 pm on November 29, 2018: contributor
    utACK
  24. promag commented at 11:21 pm on November 30, 2018: member
    utACK 2012d4d, nice to have more usages.
  25. maguayo approved
  26. maguayo commented at 5:35 pm on December 1, 2018: none
    utACK
  27. laanwj commented at 10:27 am on December 4, 2018: member
    utACK 2012d4df23b7b065ccc1dbe596073d98e428fd3d
  28. laanwj merged this on Dec 4, 2018
  29. laanwj closed this on Dec 4, 2018
  30. laanwj referenced this in commit 0257062e50 on Dec 4, 2018
  31. gillichu referenced this in commit 8a04208077 on May 27, 2020
  32. gillichu referenced this in commit 9d5dd8752a on May 27, 2020
  33. gillichu referenced this in commit 48d08c0899 on May 28, 2020
  34. gillichu referenced this in commit 74b863e51c on May 28, 2020
  35. gillichu referenced this in commit c368d241da on May 28, 2020
  36. gillichu referenced this in commit 401487d24e on May 28, 2020
  37. gillichu referenced this in commit ef4cce175b on May 31, 2020
  38. gillichu referenced this in commit 58c4a30b0d on May 31, 2020
  39. gillichu referenced this in commit 63cba4b7fe on Jun 1, 2020
  40. gillichu referenced this in commit 91c08205bc on Jun 2, 2020
  41. gillichu referenced this in commit 60927840ff on Jun 2, 2020
  42. gillichu referenced this in commit 7daffc6a90 on Jun 3, 2020
  43. laanwj referenced this in commit 39afe5b1c6 on Jun 4, 2020
  44. sidhujag referenced this in commit f5a1bed93b on Jun 4, 2020
  45. deadalnix referenced this in commit d6121c79b0 on Jun 24, 2020
  46. stackman27 referenced this in commit 35dc414588 on Jun 26, 2020
  47. ftrader referenced this in commit 9b59864b98 on Aug 17, 2020
  48. Fabcien referenced this in commit 2a3ba49ebb on Mar 1, 2021
  49. Munkybooty referenced this in commit 66ab55baa1 on Aug 2, 2021
  50. Munkybooty referenced this in commit 1a47ad19a2 on Aug 3, 2021
  51. Munkybooty referenced this in commit c26cda307c on Aug 5, 2021
  52. Munkybooty referenced this in commit 76c1033677 on Aug 5, 2021
  53. Munkybooty referenced this in commit e65120cc0f on Aug 8, 2021
  54. Munkybooty referenced this in commit 92563a82f3 on Aug 11, 2021
  55. Munkybooty referenced this in commit 07b22f7b2b on Aug 11, 2021
  56. Munkybooty referenced this in commit 2d5a90f3b5 on Aug 15, 2021
  57. DrahtBot locked this on Aug 16, 2022


instagibbs promag Empact practicalswift scravy maguayo laanwj

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-29 04:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me