test: Mockwallet #19800

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2008-testMiniWallet changing 2 files +75 −17
  1. MarcoFalke commented at 12:33 pm on August 25, 2020: member
    This introduces a minimalistic test wallet, which can be used as a drop in replacement for the Bitcoin Core wallet to create dummy transactions with a given fee rate.
  2. test: inline hashToHex fa39c62eb7
  3. fanquake added the label Tests on Aug 25, 2020
  4. MarcoFalke commented at 12:34 pm on August 25, 2020: member
    Requested by @jnewbery in #19315 (review)
  5. in test/functional/test_framework/wallet_util.py:138 in faa2aa276d outdated 
    133@@ -119,6 +134,44 @@ def test_address(node, address, **kwargs):
134         elif addr_info[key] != value:
135             raise AssertionError("key {} value {} did not match expected value {}".format(key, addr_info[key], value))
136 
137+
138+class MiniWallet():
    jnewbery commented at 12:38 pm on August 25, 2020:
    do you think this should live in wallet_util (the docstring says “Useful util functions for testing the wallet”) or should it be in its own module?
    MarcoFalke commented at 12:43 pm on August 25, 2020:
    I think a new wallet.py makes sense. Any objections?
    jnewbery commented at 12:44 pm on August 25, 2020:
    No objections!
  6. jnewbery commented at 12:38 pm on August 25, 2020: member
    Very cool. Strong concept ACK if this allows us to build transactions outside the wallet more easily.
  7. MarcoFalke force-pushed on Aug 25, 2020
  8. MarcoFalke deleted a comment on Aug 25, 2020
  9. MarcoFalke force-pushed on Aug 25, 2020
  10. MarcoFalke deleted a comment on Aug 25, 2020
  11. MarcoFalke commented at 1:44 pm on August 25, 2020: member
    nowallet build passed: https://travis-ci.org/github/bitcoin/bitcoin/jobs/720986253#L3238
  12. in test/functional/test_framework/wallet.py:14 in faf4f21da2 outdated 
     9+    COIN,
10+    CTransaction,
11+    CTxIn,
12+    COutPoint,
13+    CTxOut,
14+    CTxInWitness,
    jonatack commented at 1:47 pm on August 25, 2020:
    nit: sort
    MarcoFalke commented at 2:23 pm on August 25, 2020:
    thx. fixed all nits
  13. in test/functional/test_framework/wallet.py:28 in faf4f21da2 outdated 
    23+    satoshi_round,
24+)
25+from decimal import Decimal
26+
27+
28+class MiniWallet():
    jonatack commented at 1:47 pm on August 25, 2020: 
    0class MiniWallet:
  14. in test/functional/test_framework/wallet.py:50 in faf4f21da2 outdated 
    45+    def send_self_transfer(self, *, fee_rate, from_node=None):
46+        if from_node is None:
47+            from_node = self._test_node
48+        self._utxos = sorted(self._utxos, key=lambda k: -k['value'])
49+        largest_utxo = self._utxos.pop()  # Pick the largest utxo and hope it covers the fee
50+        vsize = Decimal('96')
    jonatack commented at 2:05 pm on August 25, 2020:

    no need for quotes?

    0        vsize = Decimal(96)
  15. jonatack commented at 2:07 pm on August 25, 2020: member

    Concept ACK, tested ACK, very nice.

    A few style nits below, feel free to ignore.

  16. MarcoFalke force-pushed on Aug 25, 2020
  17. in test/functional/test_framework/wallet.py:40 in fa170e5a24 outdated 
    34+
35+    def generate(self, num_blocks):
36+        blocks = self._test_node.generatetoaddress(num_blocks, self.getnewaddress())
37+        for b in blocks:
38+            cb_tx = self._test_node.getblock(b, 2)['tx'][0]
39+            self._utxos.append({'txid': cb_tx['txid'], 'vout': 0, 'value': cb_tx['vout'][0]['value']})
    instagibbs commented at 2:57 pm on August 25, 2020:
    Seems like the “wallet” doesn’t know which funds are mature?
    MarcoFalke commented at 4:24 pm on August 25, 2020:
    The wallet will “sign” any transaction you ask it to sign. Also, coin selection is deterministic, so if immature funds are spent, it will be noticed when writing the test.
    instagibbs commented at 4:28 pm on August 25, 2020:
    I think the real answer is you’re relying on FIFO which picks oldest first.
    MarcoFalke commented at 4:32 pm on August 25, 2020:

    The wallet doesn’t currently query the chain in any way, so it has no notion of number of confirmations.

    If this is ever needed, it can be added later

  18. amitiuttarwar commented at 3:57 pm on August 25, 2020: contributor
    concept ACK!! very nice :)
  19. theStack commented at 4:07 am on August 26, 2020: member
    Concept ACK
  20. MarcoFalke commented at 6:26 am on September 7, 2020: member
    Would be good to make progress here, so that the mockwallet can be used in new tests
  21. in test/functional/test_framework/wallet.py:25 in fa170e5a24 outdated 
    20+from test_framework.util import (
21+    assert_equal,
22+    hex_str_to_bytes,
23+    satoshi_round,
24+)
25+from decimal import Decimal
    jnewbery commented at 11:06 am on September 7, 2020:
    nit: standard library imports before local imports
    MarcoFalke commented at 12:47 pm on September 7, 2020:
    done
  22. in test/functional/test_framework/wallet.py:32 in fa170e5a24 outdated 
    27+
28+class MiniWallet:
29+    def __init__(self, test_node):
30+        self._test_node = test_node
31+        self._utxos = []
32+        self._address = ADDRESS_BCRT1_P2WSH_OP_TRUE
    jnewbery commented at 11:08 am on September 7, 2020:
    why is the _address member needed? Why not always just use the ADDRESS_BCRT1_P2WSH_OP_TRUE constant directly?
    MarcoFalke commented at 12:49 pm on September 7, 2020:
    If the address is changed in the future, it might be easier if there is only one occurrence of ADDRESS_BCRT1_P2WSH_OP_TRUE. Not sure though
    jnewbery commented at 12:59 pm on September 7, 2020:
    I think it’s easy either way, and best not to add code until it’s needed. Doesn’t make a big difference though
  23. in test/functional/test_framework/wallet.py:42 in fa170e5a24 outdated 
    37+        for b in blocks:
38+            cb_tx = self._test_node.getblock(b, 2)['tx'][0]
39+            self._utxos.append({'txid': cb_tx['txid'], 'vout': 0, 'value': cb_tx['vout'][0]['value']})
40+        return blocks
41+
42+    def getnewaddress(self):
    jnewbery commented at 11:12 am on September 7, 2020:
    May not be needed (unless you expect individual tests to use this in future)?
    MarcoFalke commented at 12:50 pm on September 7, 2020:
    thx removed
  24. in test/functional/test_framework/wallet.py:38 in fa170e5a24 outdated 
    33+        self._scriptPubKey = hex_str_to_bytes(self._test_node.validateaddress(self._address)['scriptPubKey'])
34+
35+    def generate(self, num_blocks):
36+        blocks = self._test_node.generatetoaddress(num_blocks, self.getnewaddress())
37+        for b in blocks:
38+            cb_tx = self._test_node.getblock(b, 2)['tx'][0]
    jnewbery commented at 11:13 am on September 7, 2020:
    use named argument verbosity=2 for clarity?
    MarcoFalke commented at 12:50 pm on September 7, 2020:
    thx changed
  25. in test/functional/test_framework/wallet.py:35 in fa170e5a24 outdated 
    30+        self._test_node = test_node
31+        self._utxos = []
32+        self._address = ADDRESS_BCRT1_P2WSH_OP_TRUE
33+        self._scriptPubKey = hex_str_to_bytes(self._test_node.validateaddress(self._address)['scriptPubKey'])
34+
35+    def generate(self, num_blocks):
    jnewbery commented at 11:14 am on September 7, 2020:
    Maybe add a function docstring: “Generate blocks with an anyone-can-spend coinbase outputs, and add outputs to internal list to spend later” or similar.
    MarcoFalke commented at 12:50 pm on September 7, 2020:
    thx, added docstring
  26. in test/functional/test_framework/wallet.py:46 in fa170e5a24 outdated 
    42+    def getnewaddress(self):
43+        return self._address
44+
45+    def send_self_transfer(self, *, fee_rate, from_node):
46+        self._utxos = sorted(self._utxos, key=lambda k: -k['value'])
47+        largest_utxo = self._utxos.pop()  # Pick the largest utxo and hope it covers the fee
    jnewbery commented at 11:16 am on September 7, 2020:
    Rather than hoping, can you assert that send_value below is +ve, for a more helpful error message.
    MarcoFalke commented at 12:50 pm on September 7, 2020:
    added assert
  27. in test/functional/test_framework/wallet.py:43 in fa170e5a24 outdated 
    40+        return blocks
41+
42+    def getnewaddress(self):
43+        return self._address
44+
45+    def send_self_transfer(self, *, fee_rate, from_node):
    jnewbery commented at 11:18 am on September 7, 2020:
    Add a docstring. What’s the guarantee to the caller here? That the transaction’s feerate will be strictly greater than the fee_rate parameter provided?
    jnewbery commented at 11:19 am on September 7, 2020:
    It feels to me like the transaction construction/signing should be separated from the relaying. ie this function’s responsibility should be to return a serialized transaction that the caller can then submit from any node (or manually over a p2p connection)
    MarcoFalke commented at 12:51 pm on September 7, 2020:
    tx fee will be exact (or off by at most one satoshi). Added docstring
    MarcoFalke commented at 12:52 pm on September 7, 2020:
    Agree, but I can’t have the asserts here without testmempoolaccept returning the vsize and fee. I will fix this when the functionality is needed.
  28. in test/functional/p2p_feefilter.py:82 in fa170e5a24 outdated 
    76@@ -83,27 +77,27 @@ def test_feefilter_forcerelay(self):
77     def test_feefilter(self):
78         node1 = self.nodes[1]
79         node0 = self.nodes[0]
80+        miniwallet = MiniWallet(node1)
81+        miniwallet.generate(5)
    jnewbery commented at 11:27 am on September 7, 2020:

    I think this can just be generate(1) since the output UTXO can always be used in subsequent tests.

    Could probably do with a comment so it’s obvious why two different generate commands are being used here.

    MarcoFalke commented at 12:53 pm on September 7, 2020:
    Not sure if we want package fee rats in this test. I’ve kept the previous behavior (packets of size 1)
    jnewbery commented at 1:00 pm on September 7, 2020:
    I still think a comment would be useful for people reading this file for the first time. Not essential though.
    MarcoFalke commented at 1:07 pm on September 7, 2020:
    added comment
  29. jnewbery commented at 11:27 am on September 7, 2020: member
    This is great. I’ve left lots of small style comments. Nothing blocking though.
  30. MarcoFalke force-pushed on Sep 7, 2020
  31. MarcoFalke commented at 12:54 pm on September 7, 2020: member
    Force pushed second commit to address @jnewbery’s feedback
  32. jnewbery commented at 12:58 pm on September 7, 2020: member
    utACK fae0de34945dce7582ac53c90c7b84e1c478a8bc
  33. test: Use MiniWalet in p2p_feefilter fa188c9c59
  34. MarcoFalke force-pushed on Sep 7, 2020
  35. jnewbery commented at 1:08 pm on September 7, 2020: member
    utACK fa188c9c59b8c3e43c31be01797f073e27a7bc10
  36. MarcoFalke merged this on Sep 9, 2020
  37. MarcoFalke closed this on Sep 9, 2020
  38. MarcoFalke deleted the branch on Sep 9, 2020
  39. sidhujag referenced this in commit 11d807e79d on Sep 9, 2020
  40. Fabcien referenced this in commit 088ba20c3e on Oct 11, 2021
  41. DrahtBot locked this on Feb 15, 2022


MarcoFalke jnewbery jonatack instagibbs amitiuttarwar theStack

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