test: add tests for datacarrier and datacarriersize options
#25792
Conversation
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
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_transactionhelper by creating a zero-fee template tx with MiniWallet'screate_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
w0xlt
force-pushed
the
test_data_carrier
branch
4 times, most recently
from
65b11c8 to
8677781
Compare
|
@theStack thanks for detailed review and suggestions. |
glozow
left a comment
glozow
left a comment
There was a problem hiding this comment.
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 |
stickies-v
left a comment
stickies-v
left a comment
There was a problem hiding this comment.
Approach ACK 8677781f4
Just left a few style nits
There was a problem hiding this comment.
nit: I think one-line alphabetically sorted is a bit easier to read
| from test_framework.script import ( | |
| CScript, | |
| OP_RETURN, | |
| ) | |
| from test_framework.wallet import ( | |
| MiniWallet | |
| ) | |
| from test_framework.messages import ( | |
| CTxOut, | |
| ) | |
| from test_framework.util import ( | |
| assert_raises_rpc_error, | |
| ) | |
| from test_framework.messages import CTxOut | |
| from test_framework.script import CScript, OP_RETURN | |
| from test_framework.test_framework import BitcoinTestFramework | |
| from test_framework.util import assert_raises_rpc_error | |
| from test_framework.wallet import MiniWallet |
There was a problem hiding this comment.
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: #25811
There was a problem hiding this comment.
nit: PEP8 requires 2 blank lines before class definition
There was a problem hiding this comment.
adding a short message can help debugging:
| assert(tx.rehash() in self.nodes[0].getrawmempool(True)) | |
| assert(tx.rehash() in self.nodes[0].getrawmempool(True), f"{tx_hex} not in mempool") |
There was a problem hiding this comment.
nit: the style guidelines suggest adding type hints
| def create_null_data_transaction(self, script): | |
| def create_null_data_transaction(self, script: CScript) -> CTransaction: |
There was a problem hiding this comment.
nit: I think this variable name could be a bit more descriptive (and script could be renamed to short_script)
| script2 = CScript([OP_RETURN, data]) | |
| too_long_script = CScript([OP_RETURN, data]) |
w0xlt
force-pushed
the
test_data_carrier
branch
3 times, most recently
from
3588bc5 to
10bfe12
Compare
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
LGTM. Some more suggestions (feel free to ignore, or do in a follow-up):
- The interface for the
create_null_data_transactionmethod would be IMHO more logical if only the null-data payload (typebytes) 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
datacarriersizelimit - 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")
There was a problem hiding this comment.
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 (comment))
There was a problem hiding this comment.
Great suggestions! All have been addressed in 1650f9457bc1c3cfecde554522f3c38ca18ef04c.
w0xlt
force-pushed
the
test_data_carrier
branch
3 times, most recently
from
740db1b to
1650f94
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
There was a problem hiding this comment.
This parses as =0 (The test fails regardless of the second option)
Probably a good time to make this an init error finally.
There was a problem hiding this comment.
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:
["-datacarrier=0"],
["-datacarrier=1", f"-datacarriersize={MAX_OP_RETURN_RELAY - 1}"]Good catch MarcoFalke!
There was a problem hiding this comment.
Interesting! I applied the requested change and added a new test that checks that "-datacarrier=1" works.
w0xlt
force-pushed
the
test_data_carrier
branch
from
1650f94 to
02bcfe2
Compare
… functional test style guide 4edc689 doc: test: suggest multi-line imports in functional test style guide (Sebastian Falbesoner) Pull request description: As long as I remember contributing to functional tests (~2-3 years), it was always kind of an unwritten rule that multi-line imports are preferred over single-line imports in order to reduce the possibility of potential merge conflicts -- at least if more than one symbol from a module is imported. This PR adds this rule to the style guide and adapts the example test accordingly. (Inspired by bitcoin/bitcoin#25792 (comment)). ACKs for top commit: kouloumos: ACK 4edc689 1440000bytes: ACK bitcoin/bitcoin@4edc689 w0xlt: ACK bitcoin/bitcoin@4edc689 fanquake: ACK 4edc689 Tree-SHA512: c7b6ff62f601f4e57cc8334d291662987d6737ebca8d81c063280409f4412302172f1404ec16afc9a13007bcdba55bdab66b9b80363287e287888929cb386584
w0xlt
force-pushed
the
test_data_carrier
branch
from
02bcfe2 to
4bebb81
Compare
|
Rebased. |
…nal test style guide 4edc689 doc: test: suggest multi-line imports in functional test style guide (Sebastian Falbesoner) Pull request description: As long as I remember contributing to functional tests (~2-3 years), it was always kind of an unwritten rule that multi-line imports are preferred over single-line imports in order to reduce the possibility of potential merge conflicts -- at least if more than one symbol from a module is imported. This PR adds this rule to the style guide and adapts the example test accordingly. (Inspired by bitcoin#25792 (comment)). ACKs for top commit: kouloumos: ACK 4edc689 1440000bytes: ACK bitcoin@4edc689 w0xlt: ACK bitcoin@4edc689 fanquake: ACK 4edc689 Tree-SHA512: c7b6ff62f601f4e57cc8334d291662987d6737ebca8d81c063280409f4412302172f1404ec16afc9a13007bcdba55bdab66b9b80363287e287888929cb386584
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yocto-nit: sorted order
| CTxOut, | |
| CTransaction, | |
| CTransaction, | |
| CTxOut, |
stickies-v
left a comment
stickies-v
left a comment
There was a problem hiding this comment.
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
git range-diff 93999a5 1650f94 4bebb81
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
w0xlt
force-pushed
the
test_data_carrier
branch
2 times, most recently
from
1d64dc4 to
ea77b73
Compare
|
#25792 (review) addressed in 8b3d2bb |
Co-authored-by: Sebastian Falbesoner <[email protected]>
w0xlt
force-pushed
the
test_data_carrier
branch
from
ea77b73 to
8b3d2bb
Compare
| 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 |
There was a problem hiding this comment.
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?)
…ersize` options 8b3d2bb test: add tests for `datacarrier` and `datacarriersize` options (w0xlt) Pull request description: As suggested in bitcoin#25787, this PR adds tests for `datacarrier` and `datacarriersize` initialization options. Close bitcoin#25787. ACKs for top commit: theStack: re-ACK 8b3d2bb stickies-v: re-ACK bitcoin@8b3d2bb Tree-SHA512: 962638ac9659f9d641bc5d1eff0571a08085dc7d4981b534b7ede03e4c702abd7048d543c199a588e2f94567b6d2393280e686629b19d7f4b24d365662be5e40
7 participants
As suggested in #25787, this PR adds tests for
datacarrieranddatacarriersizeinitialization options.Close #25787.