Merge #16726: tests: Avoid common Python default parameter gotcha when mutable dict/list:s are used as default parameter values

e4f4ea47ebf7774fb6f445adde7bf7ea71fa05a1 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift)
25dd86715039586d92176eee16e9c6644d2547f0 Avoid using mutable default parameter values (practicalswift)

Pull request description:

  Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values.

  Examples of this gotcha caught during review:
  * https://github.com/bitcoin/bitcoin/pull/16673#discussion_r317415261
  * https://github.com/bitcoin/bitcoin/pull/14565#discussion_r241942304

  Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python:

  ```
  >>> def f(i, j=[], k={}):
  ...     j.append(i)
  ...     k[i] = True
  ...     return j, k
  ...
  >>> f(1)
  ([1], {1: True})
  >>> f(1)
  ([1, 1], {1: True})
  >>> f(2)
  ([1, 1, 2], {1: True, 2: True})
  ```

  In contrast to:

  ```
  >>> def f(i, j=None, k=None):
  ...     if j is None:
  ...         j = []
  ...     if k is None:
  ...         k = {}
  ...     j.append(i)
  ...     k[i] = True
  ...     return j, k
  ...
  >>> f(1)
  ([1], {1: True})
  >>> f(1)
  ([1], {1: True})
  >>> f(2)
  ([2], {2: True})
  ```

  The latter is typically the intended behaviour.

  This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-)

ACKs for top commit:
  Sjors:
    Oh Python... ACK e4f4ea47ebf7774fb6f445adde7bf7ea71fa05a1. Testing tip: swap the two commits.

Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f

test/functional/feature_dip3_deterministicmns.py

@@ -349,7 +349,9 @@ class DIP3Test(BitcoinTestFramework):
 
         return dummy_txin
 
-    def mine_block(self, node, vtx=[], miner_address=None, mn_payee=None, mn_amount=None, use_mnmerkleroot_from_tip=False, expected_error=None):
+    def mine_block(self, node, vtx=None, miner_address=None, mn_payee=None, mn_amount=None, use_mnmerkleroot_from_tip=False, expected_error=None):
+        if vtx is None:
+            vtx = []
         bt = node.getblocktemplate()
         height = bt['height']
         tip_hash = bt['previousblockhash']

test/functional/feature_llmq_data_recovery.py

@@ -28,9 +28,11 @@ class QuorumDataRecoveryTest(DashTestFramework):
         self.set_dash_test_params(9, 7, fast_dip3_enforcement=True, extra_args=extra_args)
         self.set_dash_llmq_test_params(4, 3)
 
-    def restart_mn(self, mn, reindex=False, qvvec_sync=[], qdata_recovery_enabled=True):
+    def restart_mn(self, mn, reindex=False, qvvec_sync=None, qdata_recovery_enabled=True):
         args = self.extra_args[mn.node.index] + ['-masternodeblsprivkey=%s' % mn.keyOperator,
                                               '-llmq-data-recovery=%d' % qdata_recovery_enabled]
+        if qvvec_sync is None:
+            qvvec_sync = []
         for llmq_sync in qvvec_sync:
             args.append('-llmq-qvvec-sync=%s:%d' % (llmq_type_strings[llmq_sync[0]], llmq_sync[1]))
         if reindex:
@@ -50,14 +52,22 @@ class QuorumDataRecoveryTest(DashTestFramework):
             time.sleep(1)
         self.sync_blocks()
 
-    def restart_mns(self, mns=None, exclude=[], reindex=False, qvvec_sync=[], qdata_recovery_enabled=True):
+    def restart_mns(self, mns=None, exclude=None, reindex=False, qvvec_sync=None, qdata_recovery_enabled=True):
+        if exclude is None:
+            exclude = []
+        if qvvec_sync is None:
+            qvvec_sync = []
         for mn in self.mninfo if mns is None else mns:
             if mn not in exclude:
                 self.restart_mn(mn, reindex, qvvec_sync, qdata_recovery_enabled)
         self.wait_for_sporks_same()
 
-    def test_mns(self, quorum_type_in, quorum_hash_in, valid_mns=[], all_mns=[], test_secret=True, expect_secret=True,
+    def test_mns(self, quorum_type_in, quorum_hash_in, valid_mns=None, all_mns=None, test_secret=True, expect_secret=True,
                  recover=False, timeout=120):
+        if valid_mns is None:
+            valid_mns = []
+        if all_mns is None:
+            all_mns = []
         for mn in all_mns:
             if mn not in valid_mns:
                 assert not self.test_mn_quorum_data(mn, quorum_type_in, quorum_hash_in, test_secret, False)
@@ -118,8 +128,8 @@ class QuorumDataRecoveryTest(DashTestFramework):
                     members_only_in_1 = self.get_subset_only_in_left(member_mns_1, member_mns_2)
                     members_only_in_2 = self.get_subset_only_in_left(member_mns_2, member_mns_1)
                 # So far the nodes of quorum_1 shouldn't have the quorum verification vector of quorum_2 and vice versa
-                self.test_mns(llmq_type, quorum_hash_2, valid_mns=[], all_mns=members_only_in_1, expect_secret=False)
-                self.test_mns(llmq_type, quorum_hash_1, valid_mns=[], all_mns=members_only_in_2, expect_secret=False)
+                self.test_mns(llmq_type, quorum_hash_2, all_mns=members_only_in_1, expect_secret=False)
+                self.test_mns(llmq_type, quorum_hash_1, all_mns=members_only_in_2, expect_secret=False)
                 # Now restart with recovery enabled
                 self.restart_mns(qvvec_sync=llmq_sync_entries)
                 # Members which are only in quorum 2 should request the qvvec from quorum 1 from the members of quorum 1

test/functional/feature_llmq_is_cl_conflicts.py

@@ -276,7 +276,9 @@ class LLMQ_IS_CL_Conflicts(DashTestFramework):
             # Previous tip should be marked as conflicting now
             assert_equal(node.getchaintips(2)[1]["status"], "conflicting")
 
-    def create_block(self, node, vtx=[]):
+    def create_block(self, node, vtx=None):
+        if vtx is None:
+            vtx = []
         bt = node.getblocktemplate()
         height = bt['height']
         tip_hash = bt['previousblockhash']

test/functional/test_framework/messages.py

@@ -922,7 +922,9 @@ class HeaderAndShortIDs:
         key1 = struct.unpack("<Q", hash_header_nonce_as_str[8:16])[0]
         return [ key0, key1 ]
 
-    def initialize_from_block(self, block, nonce=0, prefill_list = [0]):
+    def initialize_from_block(self, block, nonce=0, prefill_list=None):
+        if prefill_list is None:
+            prefill_list = [0]
         self.header = CBlockHeader(block)
         self.nonce = nonce
         self.prefilled_txn = [ PrefilledTransaction(i, block.vtx[i]) for i in prefill_list ]
@@ -2030,8 +2032,8 @@ class msg_islock:
     __slots__ = ("inputs", "txid", "sig",)
     command = b"islock"
 
-    def __init__(self, inputs=[], txid=0, sig=b'\x00' * 96):
-        self.inputs = inputs
+    def __init__(self, inputs=None, txid=0, sig=b'\x00' * 96):
+        self.inputs = inputs if inputs is not None else []
         self.txid = txid
         self.sig = sig
 
@@ -2055,9 +2057,9 @@ class msg_isdlock:
     __slots__ = ("nVersion", "inputs", "txid", "cycleHash", "sig")
     command = b"isdlock"
 
-    def __init__(self, nVersion=1, inputs=[], txid=0, cycleHash=0, sig=b'\x00' * 96):
+    def __init__(self, nVersion=1, inputs=None, txid=0, cycleHash=0, sig=b'\x00' * 96):
         self.nVersion = nVersion
-        self.inputs = inputs
+        self.inputs = inputs if inputs is not None else []
         self.txid = txid
         self.cycleHash = cycleHash
         self.sig = sig
@@ -2087,8 +2089,8 @@ class msg_qsigshare:
     __slots__ = ("sig_shares",)
     command = b"qsigshare"
 
-    def __init__(self, sig_shares=[]):
-        self.sig_shares = sig_shares
+    def __init__(self, sig_shares=None):
+        self.sig_shares = sig_shares if sig_shares is not None else []
 
     def deserialize(self, f):
         self.sig_shares = deser_vector(f, CSigShare)

test/functional/wallet_importmulti.py

@@ -44,8 +44,10 @@ class ImportMultiTest(BitcoinTestFramework):
     def setup_network(self):
         self.setup_nodes()
 
-    def test_importmulti(self, req, success, error_code=None, error_message=None, warnings=[]):
+    def test_importmulti(self, req, success, error_code=None, error_message=None, warnings=None):
         """Run importmulti and assert success"""
+        if warnings is None:
+            warnings = []
         result = self.nodes[1].importmulti([req])
         observed_warnings = []
         if 'warnings' in result[0]:

test/lint/lint-python-mutable-default-parameters.sh

@@ -0,0 +1,52 @@
+#!/usr/bin/env bash
+#
+# Copyright (c) 2019 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+#
+# Detect when a mutable list or dict is used as a default parameter value in a Python function.
+
+export LC_ALL=C
+EXIT_CODE=0
+OUTPUT=$(git grep -E '^\s*def [a-zA-Z0-9_]+\(.*=\s*(\[|\{)' -- "*.py")
+if [[ ${OUTPUT} != "" ]]; then
+    echo "A mutable list or dict seems to be used as default parameter value:"
+    echo
+    echo "${OUTPUT}"
+    echo
+    cat << EXAMPLE
+This is how mutable list and dict default parameter values behave:
+
+>>> def f(i, j=[], k={}):
+...     j.append(i)
+...     k[i] = True
+...     return j, k
+...
+>>> f(1)
+([1], {1: True})
+>>> f(1)
+([1, 1], {1: True})
+>>> f(2)
+([1, 1, 2], {1: True, 2: True})
+
+The intended behaviour was likely:
+
+>>> def f(i, j=None, k=None):
+...     if j is None:
+...         j = []
+...     if k is None:
+...         k = {}
+...     j.append(i)
+...     k[i] = True
+...     return j, k
+...
+>>> f(1)
+([1], {1: True})
+>>> f(1)
+([1], {1: True})
+>>> f(2)
+([2], {2: True})
+EXAMPLE
+    EXIT_CODE=1
+fi
+exit ${EXIT_CODE}