Tests: enforce critical class instance attributes in functional tests, fix segwit test specificity #14305

JustinTArthur · 2018-09-24T03:00:02Z

No extra attributes will be able to be added to instances of the C++ class ports or of other critical classes without causing an exception. Helps prevent adding or depending on attributes that aren't in the intended object structure. It may prevent issues such as the one fixed in #14300.

This request fixes the erroneous version attribute used in the p2p_segwit.py functional tests. This pull includes the commit from #14300.

JustinTArthur · 2018-09-24T03:15:06Z

~~Travis tests should be re-run following the merge of #14300.~~ Rebased this PR on #14300, so dependency isn't an issue.

DrahtBot · 2018-09-24T04:13:00Z

Reviewers, this pull request conflicts with the following ones:

Relay ancestors of transactions #14318 (Relay ancestors of transactions by sdaftuar)
tests: Remove unused testing code #14312 (tests: Remove unused testing code by practicalswift)
Transaction relay privacy bugfix #14220 (Transaction relay privacy bugfix by sdaftuar)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

practicalswift · 2018-09-24T05:49:51Z

What an excellent first-time contribution! Thanks!

This is my favourite type of contribution: it not only fixes current code but also fixes future code by introducing a mechanism that helps us avoid introducing new instances of the same class of issue. Exactly the type of contribution that I was referring to in #14217 (comment):

Personally I'm much more excited to see a PR submitted which strengthens the long term security of the project in some form compared to when I see a PR submitted which implements some shiny new feature :-)

In addition to the above: __slots__ is also an unambiguous improvement from a memory usage perspective.

utACK 965c8ff9c7539719eb36bd46297358e33b571766

jnewbery · 2018-09-24T12:23:54Z

Concept ACK.

There's some suggestion in this thread: https://stackoverflow.com/questions/472000/usage-of-slots that using __slots__ to constrain attribute creation is discouraged. However, I've searched for a few minutes and haven't been able to find a 'better' way to do it.

maflcko · 2018-09-24T12:47:19Z

test/functional/p2p_segwit.py

@@ -771,8 +771,8 @@ def test_p2sh_witness(self):
        # segwit-aware would also reject this for failing CLEANSTACK.
        test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False)


Would be nice to demonstrate that this fails due to the correct reason: "non-mandatory-script-verify-flag (Witness program was passed an empty witness)"

You can achieve this with the assert_debug_log helper.

I agree. Thanks for the tip on assert_debug_log. I'm more familiar with Python than I am of the test suite. Addressed in a new commit.

maflcko · 2018-09-24T12:49:00Z

test/functional/p2p_segwit.py

-        # Try to put the witness script in the script_sig, should also fail.
-        spend_tx.vin[0].script_sig = CScript([p2wsh_pubkey, b'a'])
+        # Try to put the witness script in the scriptSig, should also fail.
+        spend_tx.vin[0].scriptSig = CScript([p2wsh_pubkey, b'a'])
        spend_tx.rehash()
        test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False)


Same here: The reject reason is just the same as above, whereas it now should be "mandatory-script-verify-flag-failed (Script evaluated without error but finished with a false/empty top stack element)"

Addressed in a new commit.

maflcko · 2018-09-24T12:53:02Z

Concept ACK. I guess this could also be implemented with a "frozen" decorator, which freezes the attributes after __init__.

It worries me that the tests just pass with or without this fix, which doesn't add much value to the fix. Would you mind changing the tests itself, such that they would fail without, but pass with the change. (See inline comments)

gmaxwell · 2018-09-26T00:25:05Z

You are my hero of the day. I am python ignorant, and so I'm only concept ACKing because (1) I've run into this problem before and been frustrated, (2) I see people who know the tests better haven't opposed it, and (3) I asked for someone to fix this. :P

JustinTArthur · 2018-09-26T02:28:22Z

Concept ACK. I guess this could also be implemented with a "frozen" decorator, which freezes the attributes after __init__.

I'm not opposed to moving to that at some point. Using __slots__ everywhere makes the code more verbose, where Python can otherwise excel at being concise. Unfortunately, these tests seem to do a bit of assignment after initialization so some refactoring would be necessary.

Added some checks for specific transaction acceptance failures in the debug log per @MarcoFalke's suggestion. Now at least the scriptSig typo would have caused a failure.

Thanks for the feedback, @MarcoFalke @gmaxwell and @practicalswift.

JustinTArthur · 2018-09-26T05:27:48Z

@MarcoFalke I've created an alternate implementation that uses a freezing decorator as you've suggested:
master...JustinTArthur:functional-test-attr-enforcement-decorator
Decorator implementation isn't great to look at, but makes the classes easier on the eyes. We'd lose the performance benefits of __slots__, if that matters. Unlike the freezing method used by attrs and Python 3.7 data classes, this will allow re-assignment of initialized attributes.

If the new implementation is preferred across the board, I can replace this PR's commits with the alternate ones. For what it's worth, I prefer __slots__. I don't think it's an aspect of the language at risk of breaking in the Python 3.x timeframe and while using double-underscore stuff never looks great, I don't see __slots__ as being much different from supplying an __init__. We're starting to see __slots__ mentioned in more of the higher level Python docs.

ken2812221 · 2018-09-26T06:54:21Z

Concept ACK. Will try to learn how __slots__ work.

promag · 2018-09-26T08:43:21Z

Concept ACK, +1 __slots__ for this case.

maflcko

I slightly prefer the decorator for its brevity when used and thus easy reusability. We don't care about performance improvements if they are not significant or can not be measured.

Though, if slots are the commonly the preferred thing, I am fine with that.

maflcko · 2018-09-26T15:17:09Z

test/functional/p2p_segwit.py

        spend_tx.rehash()
-        test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False)
+        with self.nodes[0].assert_debug_log(expected_msgs=('Not relaying invalid transaction {}'.format(spend_tx.hash), 'mandatory-script-verify-flag-failed')):


nit: Could use the full error message here?

'was not accepted: mandatory-script-verify-flag-failed (Script evaluated without error but finished with a false/empty top stack element)'

Reasonable, will adjust commit.

maflcko · 2018-09-26T15:17:38Z

test/functional/p2p_segwit.py

@@ -769,12 +769,14 @@ def test_p2sh_witness(self):
        # will require a witness to spend a witness program regardless of
        # segwit activation.  Note that older bitcoind's that are not
        # segwit-aware would also reject this for failing CLEANSTACK.
-        test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False)
+        with self.nodes[0].assert_debug_log(expected_msgs=(spend_tx.hash, 'was not accepted: non-mandatory-script-verify-flag')):


nit: Could use the full error message here?

'was not accepted: non-mandatory-script-verify-flag (Witness program was passed an empty witness)'

Reasonable, will adjust commit.

jnewbery · 2018-09-26T15:42:53Z

Concept ACK either approach. I slightly prefer the __slots__ approach, since it seems more explicit to me and doesn't require us to roll our own attribute-fixing code, but I'm happy with either.

maflcko · 2018-09-26T16:01:56Z

Ok, let's keep the slots then.

practicalswift · 2018-09-26T16:49:58Z

Agree with @jnewbery – the explicitness of __slots__ is nice. Also: it is more idiomatic since it works out of the box without any extra code.

From the Zen of Python:

Explicit is better than implicit.
There should be one – and preferably only one – obvious way to do it.

:-)

jimmysong

ACK

Nice use of the slots feature! I would like to see something instructing new devs in the Python testing documentation (functional/README.md) so people don't trip over it.

test/functional/test_framework/script.py

Additionally, removed redundant parentheses and added PEP-8 compliant spacing around those classes.

@jimmysong

Per @jimmysong's suggestion in bitcoin#14305. Also corrects module for network objects and wrappers.

JustinTArthur · 2018-09-27T03:19:43Z

Revised commits to incorporate code comment suggestions from @MarcoFalke and @jimmysong.

jimmysong

Thanks for the changes!

ACK

jnewbery · 2018-09-27T14:52:27Z

utACK e460232

maflcko · 2018-09-27T15:10:04Z

utACK e460232

…nctional tests, fix segwit test specificity e460232 Document fixed attribute behavior in critical test framework classes. (Justin Turner Arthur) 17b42f4 Check for specific tx acceptance failures based on script signature (Justin Turner Arthur) 3a4449e Strictly enforce instance attrs in critical functional test classes. (Justin Turner Arthur) 1d0ce94 Fix for incorrect version attr set on functional test segwit block. (Justin Turner Arthur) ba923e3 test: Fix broken segwit test (practicalswift) Pull request description: No extra attributes will be able to be added to instances of the C++ class ports or of other critical classes without causing an exception. Helps prevent adding or depending on attributes that aren't in the intended object structure. It may prevent issues such as the one fixed in #14300. This request fixes the erroneous version attribute used in the p2p_segwit.py functional tests. This pull includes the commit from #14300. Tree-SHA512: 1b8c58e7aa0f71075ed5ff3e5be0a5182599108d8cd9bce682feac3b1842508124288e9335432b16a43f40f159c9710899e6d84af1b5868f48c947bc6f3e07ec

@jimmysong

Per @jimmysong's suggestion in bitcoin/bitcoin#14305. Also corrects module for network objects and wrappers.

@jimmysong

Per @jimmysong's suggestion in bitcoin#14305. Also corrects module for network objects and wrappers.

@jimmysong

Per @jimmysong's suggestion in bitcoin#14305. Also corrects module for network objects and wrappers.

…es in functional tests, fix segwit test specificity e460232 Document fixed attribute behavior in critical test framework classes. (Justin Turner Arthur) 17b42f4 Check for specific tx acceptance failures based on script signature (Justin Turner Arthur) 3a4449e Strictly enforce instance attrs in critical functional test classes. (Justin Turner Arthur) 1d0ce94 Fix for incorrect version attr set on functional test segwit block. (Justin Turner Arthur) ba923e3 test: Fix broken segwit test (practicalswift) Pull request description: No extra attributes will be able to be added to instances of the C++ class ports or of other critical classes without causing an exception. Helps prevent adding or depending on attributes that aren't in the intended object structure. It may prevent issues such as the one fixed in bitcoin#14300. This request fixes the erroneous version attribute used in the p2p_segwit.py functional tests. This pull includes the commit from bitcoin#14300. Tree-SHA512: 1b8c58e7aa0f71075ed5ff3e5be0a5182599108d8cd9bce682feac3b1842508124288e9335432b16a43f40f159c9710899e6d84af1b5868f48c947bc6f3e07ec

…s in functional tests, fix segwit test specificity e460232 Document fixed attribute behavior in critical test framework classes. (Justin Turner Arthur) 17b42f4 Check for specific tx acceptance failures based on script signature (Justin Turner Arthur) 3a4449e Strictly enforce instance attrs in critical functional test classes. (Justin Turner Arthur) 1d0ce94 Fix for incorrect version attr set on functional test segwit block. (Justin Turner Arthur) ba923e3 test: Fix broken segwit test (practicalswift) Pull request description: No extra attributes will be able to be added to instances of the C++ class ports or of other critical classes without causing an exception. Helps prevent adding or depending on attributes that aren't in the intended object structure. It may prevent issues such as the one fixed in bitcoin#14300. This request fixes the erroneous version attribute used in the p2p_segwit.py functional tests. This pull includes the commit from bitcoin#14300. Tree-SHA512: 1b8c58e7aa0f71075ed5ff3e5be0a5182599108d8cd9bce682feac3b1842508124288e9335432b16a43f40f159c9710899e6d84af1b5868f48c947bc6f3e07ec

test: Fix broken segwit test

ba923e3

fanquake added the Tests label Sep 24, 2018

Fix for incorrect version attr set on functional test segwit block.

1d0ce94

JustinTArthur force-pushed the functional-test-attr-enforcement branch from bb803fe to 965c8ff Compare September 24, 2018 03:31

DrahtBot mentioned this pull request Sep 24, 2018

[qa] Test inter-bucket privacy leakage #14240

Closed

JustinTArthur changed the title ~~Functional test attribute enforcement~~ Tests: enforce critical class instance attributes in functional tests Sep 24, 2018

maflcko mentioned this pull request Sep 24, 2018

test: Fix broken segwit test #14300

Closed

maflcko reviewed Sep 24, 2018

View reviewed changes

This was referenced Sep 24, 2018

tests: Remove unused testing code #14312

Closed

Relay ancestors of transactions #14318

Closed

Transaction relay privacy bugfix #14220

Closed

JustinTArthur changed the title ~~Tests: enforce critical class instance attributes in functional tests~~ Tests: enforce critical class instance attributes in functional tests, fix segwit test specificity Sep 26, 2018

maflcko approved these changes Sep 26, 2018

View reviewed changes

jimmysong reviewed Sep 26, 2018

View reviewed changes

test/functional/test_framework/script.py Show resolved Hide resolved

JustinTArthur added 2 commits September 26, 2018 21:50

Strictly enforce instance attrs in critical functional test classes.

3a4449e

Additionally, removed redundant parentheses and added PEP-8 compliant spacing around those classes.

Check for specific tx acceptance failures based on script signature

17b42f4

Document fixed attribute behavior in critical test framework classes.

e460232

Per @jimmysong's suggestion in bitcoin#14305. Also corrects module for network objects and wrappers.

JustinTArthur force-pushed the functional-test-attr-enforcement branch from 35c9a46 to e460232 Compare September 27, 2018 03:15

jimmysong approved these changes Sep 27, 2018

View reviewed changes

maflcko merged commit e460232 into bitcoin:master Sep 27, 2018

JustinTArthur deleted the functional-test-attr-enforcement branch September 28, 2018 05:20

Bushstar pushed a commit to Bushstar/Feathercoin that referenced this pull request Oct 9, 2018

Document fixed attribute behavior in critical test framework classes.

627988a

Per @jimmysong's suggestion in bitcoin/bitcoin#14305. Also corrects module for network objects and wrappers.

Bushstar mentioned this pull request Oct 11, 2018

Updates from bitcoin/master FeatherCoin/Feathercoin#366

Merged

jfhk pushed a commit to jfhk/bitcoin that referenced this pull request Nov 14, 2018

Document fixed attribute behavior in critical test framework classes.

fd01446

Per @jimmysong's suggestion in bitcoin#14305. Also corrects module for network objects and wrappers.

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Nov 26, 2018

Document fixed attribute behavior in critical test framework classes.

0a3efe1

Per @jimmysong's suggestion in bitcoin#14305. Also corrects module for network objects and wrappers.

pravblockc added a commit to pravblockc/dash that referenced this pull request Aug 2, 2021

apply dash specific changes for bitcoin#14305

63b069c

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests: enforce critical class instance attributes in functional tests, fix segwit test specificity #14305

Tests: enforce critical class instance attributes in functional tests, fix segwit test specificity #14305

JustinTArthur commented Sep 24, 2018 •

edited

Loading

JustinTArthur commented Sep 24, 2018 •

edited

Loading

DrahtBot commented Sep 24, 2018 •

edited

Loading

practicalswift commented Sep 24, 2018 •

edited

Loading

jnewbery commented Sep 24, 2018

maflcko Sep 24, 2018

JustinTArthur Sep 26, 2018

maflcko Sep 24, 2018

JustinTArthur Sep 26, 2018

maflcko commented Sep 24, 2018

gmaxwell commented Sep 26, 2018

JustinTArthur commented Sep 26, 2018

JustinTArthur commented Sep 26, 2018 •

edited

Loading

ken2812221 commented Sep 26, 2018

promag commented Sep 26, 2018

maflcko left a comment

maflcko Sep 26, 2018

JustinTArthur Sep 27, 2018

maflcko Sep 26, 2018

JustinTArthur Sep 27, 2018

jnewbery commented Sep 26, 2018

maflcko commented Sep 26, 2018

practicalswift commented Sep 26, 2018 •

edited

Loading

jimmysong left a comment

JustinTArthur commented Sep 27, 2018

jimmysong left a comment

jnewbery commented Sep 27, 2018

maflcko commented Sep 27, 2018

		@@ -771,8 +771,8 @@ def test_p2sh_witness(self):
		# segwit-aware would also reject this for failing CLEANSTACK.
		test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False)

Tests: enforce critical class instance attributes in functional tests, fix segwit test specificity #14305

Tests: enforce critical class instance attributes in functional tests, fix segwit test specificity #14305

Conversation

JustinTArthur commented Sep 24, 2018 • edited Loading

JustinTArthur commented Sep 24, 2018 • edited Loading

DrahtBot commented Sep 24, 2018 • edited Loading

practicalswift commented Sep 24, 2018 • edited Loading

jnewbery commented Sep 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko commented Sep 24, 2018

gmaxwell commented Sep 26, 2018

JustinTArthur commented Sep 26, 2018

JustinTArthur commented Sep 26, 2018 • edited Loading

ken2812221 commented Sep 26, 2018

promag commented Sep 26, 2018

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery commented Sep 26, 2018

maflcko commented Sep 26, 2018

practicalswift commented Sep 26, 2018 • edited Loading

jimmysong left a comment

Choose a reason for hiding this comment

JustinTArthur commented Sep 27, 2018

jimmysong left a comment

Choose a reason for hiding this comment

jnewbery commented Sep 27, 2018

maflcko commented Sep 27, 2018

JustinTArthur commented Sep 24, 2018 •

edited

Loading

JustinTArthur commented Sep 24, 2018 •

edited

Loading

DrahtBot commented Sep 24, 2018 •

edited

Loading

practicalswift commented Sep 24, 2018 •

edited

Loading

JustinTArthur commented Sep 26, 2018 •

edited

Loading

practicalswift commented Sep 26, 2018 •

edited

Loading