test: add tests for `datacarrier` and `datacarriersize` options #25792

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

Close #25787.

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

tx = self.wallet.create_self_transfer(fee_rate=0)["tx"]
tx.vout.append(CTxOut(nValue=0, scriptPubKey=script))
tx.vout[0].nValue -= tx.get_vsize()  # simply pay 1sat/vbyte fee
return tx

@theStack thanks for detailed review and suggestions. They really simplified the code. I added you as co-author.

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.

Concept ACK

Approach ACK 8677781f4

Just left a few style nits

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

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

ACK 1650f9457bc1c3cfecde554522f3c38ca18ef04c

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

Rebased.

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.

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

<details> <summary>git range-diff 93999a5fb 1650f94 4bebb81</summary>

1:  1650f9457 ! 1:  4bebb8162 test: add tests for `datacarrier` and `datacarriersize` options
    @@ test/functional/mempool_datacarrier.py (new)
     +        self.num_nodes = 3
     +        self.extra_args = [
     +            [],
    -+            ["-datacarrier=false"],
    -+            ["-datacarrier=true", f"-datacarriersize={MAX_OP_RETURN_RELAY - 1}"]
    ++            ["-datacarrier=0"],
    ++            ["-datacarrier=1", f"-datacarriersize={MAX_OP_RETURN_RELAY - 1}"]
     +        ]
     +
     +    def test_transaction(self, node: TestNode, tx: CTransaction, success: bool):
    @@ test/functional/mempool_datacarrier.py (new)
     +        # By default, only 80 bytes are used for data (+1 for OP_RETURN, +2 for the pushdata opcodes).
     +        default_size_data = random_bytes(MAX_OP_RETURN_RELAY - 3)
     +        too_long_data = random_bytes(MAX_OP_RETURN_RELAY - 2)
    ++        small_data = random_bytes(MAX_OP_RETURN_RELAY - 4)
     +
     +        self.log.info("Testing null data transaction with default -datacarrier and -datacarriersize values.")
     +        tx = self.create_null_data_transaction(default_size_data)
    @@ test/functional/mempool_datacarrier.py (new)
     +        tx3 = self.create_null_data_transaction(default_size_data)
     +        self.test_transaction(self.nodes[1], tx3, success=False)
     +
    -+        self.log.info("Testing a null data transaction with a small -datacarriersize value.")
    ++        self.log.info("Testing a null data transaction with a size larger than accepted by -datacarriersize.")
     +        tx4 = self.create_null_data_transaction(default_size_data)
     +        self.test_transaction(self.nodes[2], tx4, success=False)
     +
    ++        self.log.info("Testing a null data transaction with a size smaller than accepted by -datacarriersize.")
    ++        tx5 = self.create_null_data_transaction(small_data)
    ++        self.test_transaction(self.nodes[2], tx5, success=True)
    ++
     +
     +if __name__ == '__main__':
     +    DataCarrierTest().main()
     
      ## test/functional/test_framework/messages.py ##
    -@@ test/functional/test_framework/messages.py: FILTER_TYPE_BASIC = 0
    - 
    - WITNESS_SCALE_FACTOR = 4
    +@@ test/functional/test_framework/messages.py: WITNESS_SCALE_FACTOR = 4
    + DEFAULT_ANCESTOR_LIMIT = 25    # default max number of in-mempool ancestors
    + DEFAULT_DESCENDANT_LIMIT = 25  # default max number of in-mempool descendants
      
     +# Default setting for -datacarriersize. 80 bytes of data, +1 for OP_RETURN, +2 for the pushdata opcodes.
     +MAX_OP_RETURN_RELAY = 83

</details>

#25792#pullrequestreview-1069545978 addressed in 8b3d2bbd0d3c87f1a68586822ddc468e0c2f9b52

re-ACK 8b3d2bbd0d3c87f1a68586822ddc468e0c2f9b52

re-ACK https://github.com/bitcoin/bitcoin/commit/8b3d2bbd0d3c87f1a68586822ddc468e0c2f9b52