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/bitcoin#14300.
This request fixes the erroneous version attribute used in the p2p_segwit.py functional tests. This pull includes the commit from bitcoin/bitcoin#14300.
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.
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
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.
770@@ -771,8 +771,8 @@ def test_p2sh_witness(self):
771 # segwit-aware would also reject this for failing CLEANSTACK.
772 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.
770@@ -771,8 +771,8 @@ def test_p2sh_witness(self):
771 # segwit-aware would also reject this for failing CLEANSTACK.
772 test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False)
773
774- # Try to put the witness script in the script_sig, should also fail.
775- spend_tx.vin[0].script_sig = CScript([p2wsh_pubkey, b'a'])
776+ # Try to put the witness script in the scriptSig, should also fail.
777+ spend_tx.vin[0].scriptSig = CScript([p2wsh_pubkey, b'a'])
778 spend_tx.rehash()
779 test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False)
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)
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.
@MarcoFalke I’ve created an alternate implementation that uses a freezing decorator as you’ve suggested:
https://github.com/bitcoin/bitcoin/compare/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.
__slots__ work.
__slots__ for this case.
777- spend_tx.vin[0].script_sig = CScript([p2wsh_pubkey, b'a'])
778+ # Try to put the witness script in the scriptSig, should also fail.
779+ spend_tx.vin[0].scriptSig = CScript([p2wsh_pubkey, b'a'])
780 spend_tx.rehash()
781- test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False)
782+ 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)'
768@@ -769,12 +769,14 @@ def test_p2sh_witness(self):
769 # will require a witness to spend a witness program regardless of
770 # segwit activation. Note that older bitcoind's that are not
771 # segwit-aware would also reject this for failing CLEANSTACK.
772- test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False)
773+ 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)'
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.
__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.
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:
:-)
367+class CScriptNum:
368+ __slots__ = ("value",)
369+
370 def __init__(self, d=0):
371 self.value = d
372
__slots__ = () for CScript below?
I avoided it because it used to be documented as impossible for variable length sequences. Looks like recent revisions have been updated to clarify that it’s ok as long as it’s an empty __slots__.
I’ll revise, test-to-confirm, and adjust the commit.
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.
Additionally, removed redundant parentheses and added PEP-8 compliant
spacing around those classes.
Per @jimmysong's suggestion in bitcoin/bitcoin#14305. Also corrects
module for network objects and wrappers.
Thanks for the changes!
ACK