test: add creating/spending validity checks for rare output scripts

theStack commented at 0:57 am on July 19, 2024: contributor

This PR adds a functional test with a collection of output scripts that are “rare”/obscure, in the sense that they wouldn’t be used or make sense within a regular payment transaction, but creating them is still considered standard, which is often considered surprising and counter-intuitive. They either

use a weird pubkey encoding (-> hybrid pubkeys [in types P2PK, P2MS]),
are provably unspendable (-> not-on-curve pubkeys [in types P2PK, P2MS, P2TR]), or
are spendable by anyone with current consensus rules (-> future segwit versions)

For all of those, the creation is considered standard, whereas spending for some is only consensus-valid; for not-on-curve pubkeys, spending is obviously impossible for mathematical reasons. The behaviour for future segwit versions is already tested in feature_segwit.py, so there is a certain overlap, but I think it stlll makes thematically sense to collect them here as well (if this PR gets Concept ACKed in general). OP_RETURN outputs don’t fall into this “rare” category as they are more or less widespread and it’s well-known that they can be used to embed arbitrary data (we already test them in mempool_datacarrier.py). Happy to include other examples as well if I missed any.

test: add creating/spending validity checks for rare output scripts 006d835acf

DrahtBot commented at 0:57 am on July 19, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	tdb3

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Jul 19, 2024

in test/functional/feature_rare_output_scripts.py:124 in 006d835acf

119+
120+    def test_future_segwit_versions(self):
121+        self.log.info("Check that creating future segwit outputs (2 <= version <= 16) is standard")
122+        funding_txs = []
123+        for future_ver in range(2, 16+1):
124+            witness_program = random.randbytes(random.randrange(2, 40+1))

tdb3 commented at 5:44 pm on July 19, 2024:

Thinking out loud: Initially thought of ensuring the edge cases (2 and 40) are explicitly added in addition to random program size selection, but this would probably be overkill.

in test/functional/feature_rare_output_scripts.py:69 in 006d835acf

64+
65+    def test_hybrid_pubkeys(self):
66+        self.log.info("Check that creating hybrid pubkey P2PK outputs is standard")
67+        privkey_even, pubkey_even = self.generate_uncompressed_keypair(is_even=True)
68+        assert pubkey_even.p.y.is_even()
69+        pubkey_even_script = key_to_p2pk_script(b'\x06' + pubkey_even.get_bytes()[1:])

tdb3 commented at 5:48 pm on July 19, 2024:

Thinking out loud: Might be nice to enhance ECPubKey to handle hybrid keys (prevents having to do byte manipulation here). Seems outside the scope of this PR though.

instagibbs commented at 12:22 pm on July 25, 2024:

not sure it’s worth the effort for such a niche thing that already has unit test coverage

tdb3 approved

tdb3 commented at 5:55 pm on July 19, 2024: contributor

ACK 006d835acf98ebceaaaead7aea41f512ed5023ad Seems like a good addition. Observed a runtime of around 1s, so minimal. Left some thoughts. Will also think about other potential candidate rare output scripts and comment if any come to mind.

fanquake requested review from instagibbs on Jul 25, 2024

instagibbs commented at 12:24 pm on July 25, 2024: member

~0 on this file tbh

it’s kind of a hodgepodge of things being tested that might already have functional test coverage, like future witness spends which is an intended upgrade hook and should be well-tested?

If they already don’t have functional coverage maybe there are better spots for each?

theStack commented at 1:47 pm on July 25, 2024: contributor

~0 on this file tbh

it’s kind of a hodgepodge of things being tested that might already have functional test coverage, like future witness spends which is an intended upgrade hook and should be well-tested?

If they already don’t have functional coverage maybe there are better spots for each?

Fair enough. I originally created this file a while ago being annoyed to repeatedly forget which kind of “rare”/weird output scripts are standard/non-standard for creating/spending, and thought it makes sense to have that nicely summarized as single file in our test suite, even with certain test overlaps like the future witness spends. But yeah, this is indeed more like a “museum collection” of loosely connected stuff rather than something systematic. Closing this PR for now.

theStack closed this on Jul 25, 2024

test: add creating/spending validity checks for rare output scripts #30481

Code Coverage

Reviews