Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

Projects
None yet
10 participants
@JustinTArthur
Copy link
Contributor

commented Sep 24, 2018

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.

@fanquake fanquake added the Tests label Sep 24, 2018

@JustinTArthur

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

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

@JustinTArthur JustinTArthur force-pushed the JustinTArthur:functional-test-attr-enforcement branch Sep 24, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

Reviewers, this pull request conflicts with the following ones:
  • #14318 (Relay ancestors of transactions by sdaftuar)
  • #14312 (tests: Remove unused testing code by practicalswift)
  • #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.

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

@practicalswift

This comment has been minimized.

Copy link
Member

commented Sep 24, 2018

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

This comment has been minimized.

Copy link
Member

commented Sep 24, 2018

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.

test/functional/p2p_segwit.py Outdated
@@ -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)

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 24, 2018

Member

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.

This comment has been minimized.

Copy link
@JustinTArthur

JustinTArthur Sep 26, 2018

Author Contributor

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.

test/functional/p2p_segwit.py Outdated
# 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)

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 24, 2018

Member

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

This comment has been minimized.

Copy link
@JustinTArthur

JustinTArthur Sep 26, 2018

Author Contributor

Addressed in a new commit.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 24, 2018

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

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

@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.

@JustinTArthur 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

@ken2812221

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

Concept ACK. Will try to learn how __slots__ work.

@promag

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

Concept ACK, +1 __slots__ for this case.

@MarcoFalke
Copy link
Member

left a comment

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.

test/functional/p2p_segwit.py Outdated
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')):

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 26, 2018

Member

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

This comment has been minimized.

Copy link
@JustinTArthur

JustinTArthur Sep 27, 2018

Author Contributor

Reasonable, will adjust commit.

test/functional/p2p_segwit.py Outdated
@@ -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')):

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 26, 2018

Member

nit: Could use the full error message here?

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

This comment has been minimized.

Copy link
@JustinTArthur

JustinTArthur Sep 27, 2018

Author Contributor

Reasonable, will adjust commit.

@jnewbery

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

Ok, let's keep the slots then.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

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
Copy link
Contributor

left a comment

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.

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

JustinTArthur added some commits Sep 24, 2018

Strictly enforce instance attrs in critical functional test classes.
Additionally, removed redundant parentheses and added PEP-8 compliant
spacing around those classes.
Document fixed attribute behavior in critical test framework classes.
Per @jimmysong's suggestion in #14305. Also corrects
module for network objects and wrappers.

@JustinTArthur JustinTArthur force-pushed the JustinTArthur:functional-test-attr-enforcement branch to e460232 Sep 27, 2018

@JustinTArthur

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

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

@jimmysong
Copy link
Contributor

left a comment

Thanks for the changes!

ACK

@jnewbery

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

utACK e460232

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

utACK e460232

@MarcoFalke MarcoFalke merged commit e460232 into bitcoin:master Sep 27, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Sep 27, 2018

Merge #14305: Tests: enforce critical class instance attributes in fu…
…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

@JustinTArthur JustinTArthur deleted the JustinTArthur:functional-test-attr-enforcement branch Sep 28, 2018

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

Document fixed attribute behavior in critical test framework classes.
Per @jimmysong's suggestion in bitcoin/bitcoin#14305. Also corrects
module for network objects and wrappers.

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

Document fixed attribute behavior in critical test framework classes.
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.
Per @jimmysong's suggestion in bitcoin#14305. Also corrects
module for network objects and wrappers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.