test: Test p2sh-witness and bech32 in wallet_import_rescan #16465

pull MarcoFalke wants to merge 4 commits into bitcoin:master from MarcoFalke:1907-testAllAddressTypesImport changing 5 files +113 −28
  1. MarcoFalke commented at 9:05 pm on July 25, 2019: member
    This adds test coverage for segwit in the wallet_import_rescan test, among other cleanups.
  2. MarcoFalke force-pushed on Jul 25, 2019
  3. MarcoFalke force-pushed on Jul 25, 2019
  4. MarcoFalke force-pushed on Jul 25, 2019
  5. MarcoFalke force-pushed on Jul 25, 2019
  6. MarcoFalke force-pushed on Jul 25, 2019
  7. MarcoFalke commented at 9:46 pm on July 25, 2019: member
    Fixed linter by adding the false-positive to a whitelist 😬
  8. DrahtBot added the label Tests on Jul 25, 2019
  9. MarcoFalke force-pushed on Jul 25, 2019
  10. DrahtBot commented at 0:13 am on July 26, 2019: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #16597 ([WIP] Travis: run full test suite on native macOS by Sjors)
    • #15845 (wallet: Fast rescan with BIP157 block filters by MarcoFalke)
    • #15257 (Scripts and tools: Bump flake8 to 3.7.8 by Empact)
    • #13728 (lint: Run the CI lint stage on mac by Empact)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

    Coverage

    Coverage Change (pull 16465, ba118a39c84bb79e5ed12db0050ff18d1595544c) Reference (master, e5fdda68c6d2313edb74443f0d1e6d2ce2d97f5e)
    Lines +0.0337 % 89.2260 %
    Functions -0.0157 % 82.1944 %
    Branches +0.0175 % 46.6152 %

    Updated at: 2019-08-07T11:47:01.569823.

  11. Sjors commented at 6:20 pm on July 26, 2019: member

    More dead code hits? https://gist.github.com/Sjors/e2704ff54fdc85b2b5833242a1d1e3e3

    Tests pass on top of #16301.

  12. MarcoFalke commented at 7:17 pm on July 26, 2019: member

    Tests pass on top of #16301

    Thanks, was about to test this.

  13. Sjors commented at 7:09 pm on August 6, 2019: member
    ACK fa2a2bab0ffef281940c5383f2f4528145707ee6
  14. MarcoFalke closed this on Aug 7, 2019
  15. MarcoFalke reopened this on Aug 7, 2019
  16. practicalswift commented at 12:16 pm on August 7, 2019: contributor
    @Sjors Regarding your posted gist: according to lint-python-dead-code.sh vulture should only look at files returned by git ls-files. Really weird that it checks .git/logs/ and depends/. Files under these directories should never be returned by git ls-files, right?
  17. in test/functional/wallet_import_rescan.py:180 in fa2a2bab0f outdated 
    180+            variant.timestamp = self.nodes[0].getblockheader(self.nodes[0].getbestblockhash())["time"]
181 
182-        # Generate a block containing the initial transactions, then another
183-        # block further in the future (past the rescan window).
184-        self.nodes[0].generate(1)
185+        # Genrate a block further in the future (past the rescan window).
    jnewbery commented at 4:07 pm on August 14, 2019:
    Typo introduced: ‘Genrate’ -> ‘Generate’
    MarcoFalke commented at 8:27 pm on August 14, 2019:
    fixed typo
  18. in test/functional/wallet_import_rescan.py:164 in fa2a2bab0f outdated 
    160@@ -143,21 +161,28 @@ def setup_network(self):
161             connect_nodes(self.nodes[i], 0)
162 
163     def run_test(self):
164+        self.random_gen = random.Random(123456)
    jnewbery commented at 6:36 pm on August 14, 2019:
    No need to reset the seed here. It should be left as set by the test_framework (where it can be set deterministically).
    MarcoFalke commented at 8:27 pm on August 14, 2019:
    Good point. I didn’t recall this.
  19. in test/functional/wallet_import_rescan.py:174 in fa2a2bab0f outdated 
    171+                label=variant.label,
172+                address_type=variant.address_type.value,
173+            ))
174             variant.key = self.nodes[1].dumpprivkey(variant.address["address"])
175-            variant.initial_amount = 1 - (i + 1) / 64
176+            variant.initial_amount = Decimal(str(self.random_gen.random())[:10])  # random amount with 8 decimal places
    jnewbery commented at 6:38 pm on August 14, 2019:
    nit: prefer round(random.random(), 8) over string slicing.
    MarcoFalke commented at 7:55 pm on August 14, 2019:

    Can you explain how this is supposed to work? Be reminded that floating point has no fixed number of decimal places.

    0Decimal(round(random.random(), 8))
1Decimal('0.5843940499999999982350118443719111382961273193359375')
    jnewbery commented at 7:57 pm on August 14, 2019:
    sorry, Decimal(str(round(random.random(), 8)))
    MarcoFalke commented at 8:26 pm on August 14, 2019:
    Changed
  20. in test/functional/wallet_import_rescan.py:146 in fa2a2bab0f outdated 
    142@@ -125,12 +143,12 @@ def skip_test_if_missing_module(self):
143         self.skip_if_no_wallet()
144 
145     def setup_network(self):
146-        extra_args = [["-addresstype=legacy"] for _ in range(self.num_nodes)]
147+        self.extra_args = [[] for _ in range(self.num_nodes)]
    jnewbery commented at 6:46 pm on August 14, 2019:
    nit: I think [[]] * self.num_nodes is slightly clearer (and also matches L154 below)
    MarcoFalke commented at 8:26 pm on August 14, 2019:
    Done
    ryanofsky commented at 5:55 pm on August 15, 2019:
    No big deal, but reason it was written the other way is so you could say args[1].append(“something”) without unintentionally adding “something” to args[0], [2], [3], and so on. Weirdness of list expressions in python.
  21. in test/functional/wallet_import_rescan.py:181 in fa2a2bab0f outdated 
    172+                address_type=variant.address_type.value,
173+            ))
174             variant.key = self.nodes[1].dumpprivkey(variant.address["address"])
175-            variant.initial_amount = 1 - (i + 1) / 64
176+            variant.initial_amount = Decimal(str(self.random_gen.random())[:10])  # random amount with 8 decimal places
177             variant.initial_txid = self.nodes[0].sendtoaddress(variant.address["address"], variant.initial_amount)
    jnewbery commented at 6:52 pm on August 14, 2019:
    This will fail if the value to send is too low. Because you set the seed, the ‘randomly’ picked number will not actually fail here, but if this test is changed (eg to support more address types) it could start failing here. I suggest you but a lower bound on the random amount.
    MarcoFalke commented at 8:26 pm on August 14, 2019:
    With “too low” you mean “dust”? Good catch! Fixed.
  22. in test/functional/test_framework/address.py:20 in fa2a2bab0f outdated 
    15 
16+
17+class AddressType(enum.Enum):
18+    bech32 = 'bech32'
19+    p2sh_segwit = 'p2sh-segwit'
20+    legacy = 'legacy'
    jnewbery commented at 6:54 pm on August 14, 2019:
    nit: add a comment here that this refers to P2PKH (P2SH is also a legacy address type)
    MarcoFalke commented at 8:26 pm on August 14, 2019:
    Added a comment
  23. in test/lint/lint-python-dead-code-whitelist:1 in fa2a2bab0f outdated 
    0@@ -0,0 +1,93 @@
1+_.posix_path  # unused attribute (contrib/macdeploy/custom_dsstore.py:45)
    jnewbery commented at 7:04 pm on August 14, 2019:
    Please remove comments with line numbers (which are not stable and will not be updated when those files are changed), and deduplicate this list.
    jnewbery commented at 7:21 pm on August 14, 2019:

    :+1: for moving these to one per line. I think you can keep the original vulture invocation by making this a list and then running:

    vulture --ignore-names=$(cat lint-python-dead-code-whitelist | tr '\n' ',') or similar.

    MarcoFalke commented at 8:25 pm on August 14, 2019:
    I think it wastes less time if we just keep the autogenerated file as is
    MarcoFalke commented at 8:25 pm on August 14, 2019:
    I think it wastes less time if we just keep the autogenerated file as is
    jnewbery commented at 8:59 pm on August 14, 2019:
    I don’t understand. Why add comments here that will be wrong almost instantly?
    MarcoFalke commented at 9:25 pm on August 14, 2019:

    Only the line number will be wrong. The rest should still be valid and might even be useful. I don’t see a reason to manually fix the line number every time this file is autogenerated.

    Edit: To obtain such a whitelist automatically, pass –make-whitelist to Vulture.

    jnewbery commented at 10:25 pm on August 14, 2019:

    I don’t get it. Every time a new function that triggers a false positive in vulture is added, this should be regenerated? That’ll create a huge diff. How is that supposed to be reviewed?

    In general, I don’t think it’s good practice to use the output of a tool to generate its own whitelist. The whitelist should be generated (and reviewed) by someone outside the tool declaring that these functions should be ignored.

    MarcoFalke commented at 12:08 pm on August 15, 2019:
    Ok, reviewed and manually fixed up the output of the tool
  24. jnewbery commented at 7:22 pm on August 14, 2019: member
    A few nits inline. Also, in commit test: Generate one block for each send in wallet_import_rescan, can you add a motivation to the commit log. I can see what it’s doing, but I don’t understand why.
  25. test: Generate one block for each send in wallet_import_rescan 
    This ...
* ensures that enough coins are available/spendable, even when more
  variants are added
* ensures that all mempool txs are mined, even when more variants are
  added
* makes the test more specific to test that the confirmation height
  is properly reported and timestamps are correctly handled in the test
  logic
* prepares the test for a future, where blocks are skipped for rescan if
  they are deemed irrelevant by a filter (c.f. BIP157)
    fac3dcf7d0
  26. in test/functional/wallet_import_rescan.py:31 in fa2a2bab0f outdated 
    26     assert_equal,
27     set_node_times,
28 )
29 
30 import collections
31+from decimal import Decimal
    jnewbery commented at 7:58 pm on August 14, 2019:
    this import is added in the wrong commit. It needs to be added in test: Replace fragile “rng” with call to random()
    MarcoFalke commented at 8:25 pm on August 14, 2019:
    Fixed
  27. MarcoFalke force-pushed on Aug 14, 2019
  28. MarcoFalke commented at 8:24 pm on August 14, 2019: member

    A few nits inline. Also, in commit test: Generate one block for each send in wallet_import_rescan, can you add a motivation to the commit log. I can see what it’s doing, but I don’t understand why.

    Added a few bullet points why generating a block each makes sense.

    Thanks for the review @jnewbery. I think I addressed all feedback

  29. MarcoFalke force-pushed on Aug 14, 2019
  30. in test/functional/wallet_import_rescan.py:140 in fa2db1044a outdated 
    133@@ -116,6 +134,14 @@ def check(self, txid=None, amount=None, confirmations=None):
134 # Rescans start at the earliest block up to 2 hours before the key timestamp.
135 TIMESTAMP_WINDOW = 2 * 60 * 60
136 
137+AMOUNT_DUST = Decimal('0.00000546')
138+
139+
140+def get_rand_amount():
    jnewbery commented at 9:08 pm on August 14, 2019:

    This returns None if the random value returned is less than AMOUNT_DUST.

    Why not use random.uniform(AMOUNT_DUST,1) rather than define this function?

    MarcoFalke commented at 9:23 pm on August 14, 2019:
    Switched to uniform
  31. in test/functional/wallet_import_rescan.py:153 in fa2db1044a outdated 
    150@@ -125,12 +151,12 @@ def skip_test_if_missing_module(self):
151         self.skip_if_no_wallet()
152 
153     def setup_network(self):
154-        extra_args = [["-addresstype=legacy"] for _ in range(self.num_nodes)]
155+        self.extra_args = [[]] * self.num_nodes
    jnewbery commented at 9:09 pm on August 14, 2019:
    is changing this to self.extra_args necessary? Does that member get used again? (I think not, since the nodes are not restarted)
    MarcoFalke commented at 9:13 pm on August 14, 2019:
    No, not necessary, but will make the code robust in case a restart is necessary.
  32. jnewbery commented at 9:11 pm on August 14, 2019: member
    Looks good. Couple more comments inline.
  33. test: Replace fragile "rng" with call to random() fa79af2989
  34. test: Test p2sh-witness and bech32 in wallet_import_rescan fa25668e1c
  35. MarcoFalke force-pushed on Aug 14, 2019
  36. in test/lint/lint-python-dead-code.sh:19 in faea9125ea outdated 
    14@@ -15,5 +15,5 @@ fi
15 
16 vulture \
17     --min-confidence 60 \
18-    --ignore-names "argtypes,connection_lost,connection_made,converter,data_received,daemon,errcheck,is_compressed,is_valid,verify_ecdsa,msg_generic,on_*,optionxform,restype,profile_with_perf" \
19-    $(git ls-files -- "*.py" ":(exclude)contrib/" ":(exclude)test/functional/data/invalid_txs.py")
20+    $(git rev-parse --show-toplevel) \
21+    $(dirname "${BASH_SOURCE[0]}")/lint-python-dead-code-whitelist
    practicalswift commented at 7:36 am on August 15, 2019:
    Won’t this run vulture on the lint-python-dead-code-whitelist file? The intention was to use lint-python-dead-code-whitelist to build --exclude? :-)
    MarcoFalke commented at 12:09 pm on August 15, 2019:
    according to the vulture documentation this is the correct invocation
    practicalswift commented at 12:30 pm on August 15, 2019:
    TIL! Thanks!
  37. MarcoFalke force-pushed on Aug 15, 2019
  38. MarcoFalke force-pushed on Aug 15, 2019
  39. MarcoFalke force-pushed on Aug 15, 2019
  40. lint: Add false positive to python dead code linter fa3c6575ca
  41. MarcoFalke force-pushed on Aug 15, 2019
  42. MarcoFalke commented at 12:08 pm on August 15, 2019: member
    Reworked the whitelist, as requested by @jnewbery
  43. in test/lint/lint-python-dead-code-whitelist:1 in fa3c6575ca 
    0@@ -0,0 +1,45 @@
1+BadInputOutpointIndex  # unused class (test/functional/data/invalid_txs.py)
    jnewbery commented at 2:21 pm on August 15, 2019:
    nit: sort
    MarcoFalke commented at 2:28 pm on August 15, 2019:
    It is sorted, according to cat ./*whitelist|sort on my system
    jnewbery commented at 2:43 pm on August 15, 2019:

    Fascinating:

     0→ cat test/lint/lint-python-dead-code-whitelist | LC_ALL=en_US.UTF-8 sort
 1BadInputOutpointIndex  # unused class (test/functional/data/invalid_txs.py)
 2_.carbon_path  # unused attribute (contrib/macdeploy/custom_dsstore.py)
 3connection_lost  # unused function (test/functional/test_framework/mininode.py)
 4connection_made  # unused function (test/functional/test_framework/mininode.py)
 5[...]
 6→ cat test/lint/lint-python-dead-code-whitelist | LC_ALL=C sort
 7BadInputOutpointIndex  # unused class (test/functional/data/invalid_txs.py)
 8DuplicateInput  # unused class (test/functional/data/invalid_txs.py)
 9InvalidOPIFConstruction  # unused class (test/functional/data/invalid_txs.py)
10NonexistentInput  # unused class (test/functional/data/invalid_txs.py)
11OutputMissing  # unused class (test/functional/data/invalid_txs.py)
12SizeTooSmall  # unused class (test/functional/data/invalid_txs.py)
13SpendNegative  # unused class (test/functional/data/invalid_txs.py)
14SpendTooMuch  # unused class (test/functional/data/invalid_txs.py)
15TooManySigops  # unused class (test/functional/data/invalid_txs.py)
16_.carbon_path  # unused attribute (contrib/macdeploy/custom_dsstore.py)
17_.converter  # unused attribute (test/functional/test_framework/test_framework.py)
18[...]
  44. jnewbery commented at 2:21 pm on August 15, 2019: member
    ACK fa3c6575cac5e3841797980fe60b8368ae579dba
  45. MarcoFalke merged this on Aug 15, 2019
  46. MarcoFalke closed this on Aug 15, 2019
  47. MarcoFalke referenced this in commit 8bd5e0af99 on Aug 15, 2019
  48. MarcoFalke deleted the branch on Aug 15, 2019
  49. ryanofsky commented at 5:57 pm on August 15, 2019: member
    post-merge utACK fa3c6575cac5e3841797980fe60b8368ae579dba. Sorry for not getting to this. I looked a while ago but was thrown off by the first commit, because it seemed to be making the test slower and more complicated for no reason, but it’s very well explained now, and in general all the new improvements and suggestions are really nice.
  50. sidhujag referenced this in commit fc7329cb00 on Aug 17, 2019
  51. jasonbcox referenced this in commit f5766026ab on Oct 22, 2020
  52. jasonbcox referenced this in commit 08d16701b5 on Oct 22, 2020
  53. jasonbcox referenced this in commit 881ae3dd22 on Oct 22, 2020
  54. UdjinM6 referenced this in commit 8e5db79c32 on Sep 10, 2021
  55. UdjinM6 referenced this in commit 8c15e14def on Sep 24, 2021
  56. UdjinM6 referenced this in commit da7979ef0c on Oct 4, 2021
  57. UdjinM6 referenced this in commit 7aefec8de0 on Oct 5, 2021
  58. kittywhiskers referenced this in commit 9d9fc51623 on Oct 12, 2021
  59. pravblockc referenced this in commit ea25e79ee9 on Nov 18, 2021
  60. DrahtBot locked this on Dec 16, 2021


MarcoFalke DrahtBot Sjors practicalswift jnewbery ryanofsky

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-07-05 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me