test: add tests for datacarrier and datacarriersize options #25792

pull w0xlt wants to merge 1 commits into bitcoin:master from w0xlt:test_data_carrier changing 3 files +75 −0
  1. w0xlt commented at 6:00 am on August 6, 2022: contributor

    As suggested in #25787, this PR adds tests for datacarrier and datacarriersize initialization options.

    Close #25787.

  2. DrahtBot added the label Tests on Aug 6, 2022
  3. theStack commented at 2:12 am on August 7, 2022: contributor

    Concept ACK

    Thanks for improving test coverage! Seems like the implementation can be simplified in many ways, I’d suggest to:

    • using MiniWallet’s default mode (ADDRESS_OP_TRUE), since we are only concerned with modifying outputs here; there is no need to use the P2PK mode
    • use the UTXOs from the pre-mined test chain instead of generating new ones; they can be imported via self.wallet.rescan_utxos() (see many other tests, e.g. p2p_filter.py)
    • shorten the create_null_data_transaction helper by creating a zero-fee template tx with MiniWallet’s create_self_transfer (we are not interested in setting the inputs manually), append the datacarrier output and then deduct the fee manually, e.g. something like this should be sufficient (admittedly, still a bit hacky though):
    0tx = self.wallet.create_self_transfer(fee_rate=0)["tx"]
1tx.vout.append(CTxOut(nValue=0, scriptPubKey=script))
2tx.vout[0].nValue -= tx.get_vsize()  # simply pay 1sat/vbyte fee
3return tx
  4. w0xlt force-pushed on Aug 7, 2022
  5. w0xlt force-pushed on Aug 7, 2022
  6. w0xlt force-pushed on Aug 7, 2022
  7. w0xlt force-pushed on Aug 7, 2022
  8. w0xlt commented at 7:55 am on August 7, 2022: contributor
    @theStack thanks for detailed review and suggestions. They really simplified the code. I added you as co-author.
  9. in test/functional/test_runner.py:323 in 8677781f45 outdated 
    319@@ -320,6 +320,7 @@
320     'feature_unsupported_utxo_db.py',
321     'feature_logging.py',
322     'feature_anchors.py',
323+    'feature_datacarrier.py',
    glozow commented at 3:29 pm on August 8, 2022:
    I’d maybe suggest naming this mempool_datacarrier.py.
  10. in test/functional/feature_datacarrier.py:52 in 8677781f45 outdated 
    47+        tx = self.create_null_data_transaction(script)
48+
49+        self.test_transaction(tx, success=True)
50+
51+        self.log.info("Testing a null data transaction larger than allowed by the default -datacarriersize value.")
52+        data = b"Hello world!" * 10
    glozow commented at 3:39 pm on August 8, 2022:

    data = b"Hello world!" * 7 would have worked just as well here.

    I’d recommend making these values just above the limits to detect off-by-one errors. For example, instead of composing it of “hello world” strings, use a MAX_OP_RETURN_RELAY-length string.

    w0xlt commented at 3:07 pm on August 9, 2022:
    Done in https://github.com/bitcoin/bitcoin/pull/25792/commits/10bfe1204fed71ff10d6bbac90912804894d454b. Thanks for the review.
  11. glozow commented at 3:42 pm on August 8, 2022: member

    Concept ACK, thanks for working on this.

    To test differences between a default and a configured node, I’d recommend you create 2 nodes with different policies and submit transactions to both, instead of restarting.

  12. brunoerg commented at 9:01 pm on August 8, 2022: contributor
    Concept ACK
  13. in test/functional/feature_datacarrier.py:42 in 8677781f45 outdated 
    37+        tx.vout[0].nValue -= tx.get_vsize()  # simply pay 1sat/vbyte fee
38+        return tx
39+
40+    def run_test(self):
41+        self.node = self.nodes[0]
42+        self.wallet = MiniWallet(self.nodes[0])
    brunoerg commented at 9:02 pm on August 8, 2022:

    nit (feel free to ignore)

    0        self.wallet = MiniWallet(self.node)
    stickies-v commented at 10:44 am on August 9, 2022:
    Or if self.node is initialized in set_test_params(), test_transaction() can also use it.
    w0xlt commented at 3:10 pm on August 9, 2022:
    The test now is using 3 nodes to avoid restarting, as suggested in #25792#pullrequestreview-1065378600. So self.node is no longer being used. Thanks for the review.
  14. in test/functional/feature_datacarrier.py:19 in 8677781f45 outdated 
    14+from test_framework.messages import (
15+    CTxOut,
16+)
17+from test_framework.util import (
18+    assert_raises_rpc_error,
19+)
    stickies-v commented at 10:38 am on August 9, 2022:

    nit: I think one-line alphabetically sorted is a bit easier to read

    0from test_framework.messages import CTxOut
1from test_framework.script import CScript, OP_RETURN
2from test_framework.test_framework import BitcoinTestFramework
3from test_framework.util import assert_raises_rpc_error
4from test_framework.wallet import MiniWallet
    theStack commented at 4:19 pm on August 9, 2022:
    AFAIK, we usually prefer multi-line over single-line imports in order to avoid potential merge conflicts. I opened a PR to hopefully get some consensus on how strict this (so far unwritten) rule should be followed: https://github.com/bitcoin/bitcoin/pull/25811
  15. in test/functional/feature_datacarrier.py:21 in 8677781f45 outdated 
    16+)
17+from test_framework.util import (
18+    assert_raises_rpc_error,
19+)
20+
21+class DataCarrierTest(BitcoinTestFramework):
    stickies-v commented at 10:39 am on August 9, 2022:
    nit: PEP8 requires 2 blank lines before class definition
  16. in test/functional/feature_datacarrier.py:30 in 8677781f45 outdated 
    25+    def test_transaction(self, tx, success):
26+        tx_hex = tx.serialize().hex()
27+
28+        if success:
29+            self.wallet.sendrawtransaction(from_node=self.nodes[0], tx_hex=tx_hex)
30+            assert(tx.rehash() in self.nodes[0].getrawmempool(True))
    stickies-v commented at 10:41 am on August 9, 2022:

    adding a short message can help debugging:

    0            assert(tx.rehash() in self.nodes[0].getrawmempool(True), f"{tx_hex} not in mempool")
  17. in test/functional/feature_datacarrier.py:71 in 8677781f45 outdated 
    66+        self.restart_node(0, extra_args=["-datacarrier=true", "-datacarriersize=5"])
67+
68+        tx4 = self.create_null_data_transaction(script)
69+
70+        self.test_transaction(tx4, success=False)
71+
    stickies-v commented at 10:46 am on August 9, 2022:
    nit: missing whiteline
  18. in test/functional/feature_datacarrier.py:34 in 8677781f45 outdated 
    29+            self.wallet.sendrawtransaction(from_node=self.nodes[0], tx_hex=tx_hex)
30+            assert(tx.rehash() in self.nodes[0].getrawmempool(True))
31+        else:
32+            assert_raises_rpc_error(-26, "scriptpubkey", self.wallet.sendrawtransaction, from_node=self.nodes[0], tx_hex=tx_hex)
33+
34+    def create_null_data_transaction(self, script):
    stickies-v commented at 10:52 am on August 9, 2022:

    nit: the style guidelines suggest adding type hints

    0    def create_null_data_transaction(self, script: CScript) -> CTransaction:
  19. in test/functional/feature_datacarrier.py:53 in 8677781f45 outdated 
    48+
49+        self.test_transaction(tx, success=True)
50+
51+        self.log.info("Testing a null data transaction larger than allowed by the default -datacarriersize value.")
52+        data = b"Hello world!" * 10
53+        script2 = CScript([OP_RETURN, data])
    stickies-v commented at 10:55 am on August 9, 2022:

    nit: I think this variable name could be a bit more descriptive (and script could be renamed to short_script)

    0        too_long_script = CScript([OP_RETURN, data])
    w0xlt commented at 3:11 pm on August 9, 2022:
    Done in https://github.com/bitcoin/bitcoin/commit/10bfe1204fed71ff10d6bbac90912804894d454b. Thanks for the review.
  20. stickies-v commented at 11:37 am on August 9, 2022: contributor

    Approach ACK 8677781f4

    Just left a few style nits

  21. w0xlt force-pushed on Aug 9, 2022
  22. w0xlt force-pushed on Aug 9, 2022
  23. w0xlt force-pushed on Aug 9, 2022
  24. in test/functional/mempool_datacarrier.py:43 in 10bfe1204f outdated 
    38+        MAX_OP_RETURN_DATA_SIZE = 80
39+
40+        self.wallet = MiniWallet(self.nodes[0])
41+        self.wallet.rescan_utxos()
42+
43+        data = os.urandom(MAX_OP_RETURN_DATA_SIZE)
    theStack commented at 4:31 pm on August 9, 2022:
    Would suggest to use the deterministic random_bytes helper instead (to be found in test_framework.util). (Background: also used the urandom approach for generating random data recently in a PR and got a review with good counter-arguments: #25625 (review))
    w0xlt commented at 10:13 pm on August 9, 2022:
    Great suggestions! All have been addressed in 1650f9457bc1c3cfecde554522f3c38ca18ef04c.
  25. theStack commented at 4:52 pm on August 9, 2022: contributor

    LGTM. Some more suggestions (feel free to ignore, or do in a follow-up):

    • The interface for the create_null_data_transaction method would be IMHO more logical if only the null-data payload (type bytes) is passed; the scriptPubKey with prepended OP_RETURN could then be created in there instead of the need to do that repeatedly at the caller’s side
    • The same approach of testing the exact boundaries (as suggested by glozow) could also be applied to the custom set datacarriersize limit
    • It’s usually preferred to name policy- or consensus-relevant constants in the functional test framework the same as in the C++ codebase (see e.g. #25596), i.e. in this case this would be MAX_OP_RETURN_RELAY (which is 83 and not 80 though, as it denotes the serialized size of the whole scriptPubKey; “+1 for OP_RETURN, 2 for the pushdata opcodes”)
  26. w0xlt force-pushed on Aug 9, 2022
  27. w0xlt force-pushed on Aug 9, 2022
  28. w0xlt force-pushed on Aug 9, 2022
  29. DrahtBot commented at 10:52 pm on August 9, 2022: contributor

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

    Conflicts

    No conflicts as of last run.

  30. stickies-v approved
  31. stickies-v commented at 11:33 am on August 10, 2022: contributor

    ACK 1650f9457bc1c3cfecde554522f3c38ca18ef04c

    edit: modulo MarcoFalke’s comment regarding -datacarrier=true

  32. in test/functional/mempool_datacarrier.py:30 in 1650f9457b outdated 
    25+    def set_test_params(self):
26+        self.num_nodes = 3
27+        self.extra_args = [
28+            [],
29+            ["-datacarrier=false"],
30+            ["-datacarrier=true", f"-datacarriersize={MAX_OP_RETURN_RELAY - 1}"]
    MarcoFalke commented at 11:50 am on August 10, 2022:

    This parses as =0 (The test fails regardless of the second option)

    Probably a good time to make this an init error finally.

    stickies-v commented at 1:30 pm on August 10, 2022:

    FYI for anyone else confused about this, see the docs of InterpretBool():

    non-numeric string values like “foo”, return 0

    so -datacarrier=false and -datacarrier=true all resolve to -datacarrier=0.

    As such, should be:

    0            ["-datacarrier=0"],
1            ["-datacarrier=1", f"-datacarriersize={MAX_OP_RETURN_RELAY - 1}"]

    Good catch MarcoFalke!

    w0xlt commented at 3:45 pm on August 10, 2022:
    Interesting! I applied the requested change and added a new test that checks that “-datacarrier=1” works.
    MarcoFalke commented at 5:32 pm on August 10, 2022:
    See also https://github.com/bitcoin/bitcoin/pull/16545
  33. MarcoFalke changes_requested
  34. w0xlt force-pushed on Aug 10, 2022
  35. MarcoFalke referenced this in commit f89ce1fdb5 on Aug 10, 2022
  36. DrahtBot added the label Needs rebase on Aug 10, 2022
  37. w0xlt force-pushed on Aug 10, 2022
  38. w0xlt commented at 7:11 pm on August 10, 2022: contributor
    Rebased.
  39. DrahtBot removed the label Needs rebase on Aug 10, 2022
  40. sidhujag referenced this in commit 02fb80156c on Aug 11, 2022
  41. in test/functional/mempool_datacarrier.py:8 in 4bebb81622 outdated 
    0@@ -0,0 +1,79 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2020-2021 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test datacarrier functionality"""
6+from test_framework.messages import (
7+    CTxOut,
8+    CTransaction,
    theStack commented at 11:10 am on August 11, 2022:

    yocto-nit: sorted order

    0    CTransaction,
1    CTxOut,
  42. theStack approved
  43. theStack commented at 11:15 am on August 11, 2022: contributor

    ACK 4bebb81622eb71a83db1d46ca84c09f7e1ab7f9a

    I think the methods test_transaction and create_null_data_transaction could be merged into one (e.g. something like test_null_data_transaction with node, data and success parameters) to reduce it to even less code at the callers side, since we don’t reuse any of the transactions anyway, but no hard feelings.

  44. stickies-v approved
  45. stickies-v commented at 11:25 am on August 11, 2022: contributor

    re-ACK 4bebb81 - verified changes are limited to fixing the startup arguments and adding a new test to check the happy path with a lower-than-default datacarriersize

     01:  1650f9457 ! 1:  4bebb8162 test: add tests for `datacarrier` and `datacarriersize` options
 1    @@ test/functional/mempool_datacarrier.py (new)
 2     +        self.num_nodes = 3
 3     +        self.extra_args = [
 4     +            [],
 5    -+            ["-datacarrier=false"],
 6    -+            ["-datacarrier=true", f"-datacarriersize={MAX_OP_RETURN_RELAY - 1}"]
 7    ++            ["-datacarrier=0"],
 8    ++            ["-datacarrier=1", f"-datacarriersize={MAX_OP_RETURN_RELAY - 1}"]
 9     +        ]
10     +
11     +    def test_transaction(self, node: TestNode, tx: CTransaction, success: bool):
12    @@ test/functional/mempool_datacarrier.py (new)
13     +        # By default, only 80 bytes are used for data (+1 for OP_RETURN, +2 for the pushdata opcodes).
14     +        default_size_data = random_bytes(MAX_OP_RETURN_RELAY - 3)
15     +        too_long_data = random_bytes(MAX_OP_RETURN_RELAY - 2)
16    ++        small_data = random_bytes(MAX_OP_RETURN_RELAY - 4)
17     +
18     +        self.log.info("Testing null data transaction with default -datacarrier and -datacarriersize values.")
19     +        tx = self.create_null_data_transaction(default_size_data)
20    @@ test/functional/mempool_datacarrier.py (new)
21     +        tx3 = self.create_null_data_transaction(default_size_data)
22     +        self.test_transaction(self.nodes[1], tx3, success=False)
23     +
24    -+        self.log.info("Testing a null data transaction with a small -datacarriersize value.")
25    ++        self.log.info("Testing a null data transaction with a size larger than accepted by -datacarriersize.")
26     +        tx4 = self.create_null_data_transaction(default_size_data)
27     +        self.test_transaction(self.nodes[2], tx4, success=False)
28     +
29    ++        self.log.info("Testing a null data transaction with a size smaller than accepted by -datacarriersize.")
30    ++        tx5 = self.create_null_data_transaction(small_data)
31    ++        self.test_transaction(self.nodes[2], tx5, success=True)
32    ++
33     +
34     +if __name__ == '__main__':
35     +    DataCarrierTest().main()
36     
37      ## test/functional/test_framework/messages.py ##
38    -@@ test/functional/test_framework/messages.py: FILTER_TYPE_BASIC = 0
39    - 
40    - WITNESS_SCALE_FACTOR = 4
41    +@@ test/functional/test_framework/messages.py: WITNESS_SCALE_FACTOR = 4
42    + DEFAULT_ANCESTOR_LIMIT = 25    # default max number of in-mempool ancestors
43    + DEFAULT_DESCENDANT_LIMIT = 25  # default max number of in-mempool descendants
44      
45     +# Default setting for -datacarriersize. 80 bytes of data, +1 for OP_RETURN, +2 for the pushdata opcodes.
46     +MAX_OP_RETURN_RELAY = 83
  46. w0xlt force-pushed on Aug 11, 2022
  47. w0xlt force-pushed on Aug 11, 2022
  48. w0xlt commented at 2:58 pm on August 11, 2022: contributor
    #25792#pullrequestreview-1069545978 addressed in 8b3d2bbd0d3c87f1a68586822ddc468e0c2f9b52
  49. test: add tests for `datacarrier` and `datacarriersize` options 
    Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
    8b3d2bbd0d
  50. w0xlt force-pushed on Aug 11, 2022
  51. theStack approved
  52. theStack commented at 3:26 pm on August 11, 2022: contributor
    re-ACK 8b3d2bbd0d3c87f1a68586822ddc468e0c2f9b52
  53. stickies-v approved
  55. MarcoFalke merged this on Aug 11, 2022
  56. MarcoFalke closed this on Aug 11, 2022
  57. in test/functional/test_framework/messages.py:72 in 8b3d2bbd0d 
    67@@ -68,6 +68,9 @@
68 DEFAULT_ANCESTOR_LIMIT = 25    # default max number of in-mempool ancestors
69 DEFAULT_DESCENDANT_LIMIT = 25  # default max number of in-mempool descendants
70 
71+# Default setting for -datacarriersize. 80 bytes of data, +1 for OP_RETURN, +2 for the pushdata opcodes.
72+MAX_OP_RETURN_RELAY = 83
    MarcoFalke commented at 4:05 pm on August 11, 2022:
    Not sure if we want to use messages.py for default values of settings. Maybe leave them in the only test that needs them for now (if there is only one), or create a new default_settings.py module (or a mempool.py one?)
  58. w0xlt deleted the branch on Aug 11, 2022
  59. sidhujag referenced this in commit 0860e44a43 on Aug 11, 2022
  60. bitcoin locked this on Aug 11, 2023


w0xlt theStack glozow brunoerg stickies-v DrahtBot MarcoFalke

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