test : improve mempool_accept_wtxid.py #22532

pull naiza2000 wants to merge 1 commits into bitcoin:master from naiza2000:master changing 1 files +4 −7

naiza2000 commented at 6:17 PM on July 22, 2021: contributor

Follow-up to #22253 adding changes suggested in #22253 (review)
DrahtBot added the label Tests on Jul 22, 2021
glozow commented at 4:50 AM on July 23, 2021: member

Concept ACK, thanks for picking this up! Needs a rebase and squash.

in test/functional/mempool_accept_wtxid.py:83 in 1872e199b0 outdated

  78 | @@ -79,10 +79,7 @@ def run_test(self):
  79 |          child_one_txid = child_one.rehash()
  80 |  
  81 |          # Create another identical transaction with witness solving second branch
  82 | -        child_two = CTransaction()
  83 | -        child_two.vin.append(CTxIn(COutPoint(int(parent_txid, 16), 0), b""))
  84 | -        child_two.vout.append(CTxOut(int(9.99996 * COIN), child_script_pubkey))
  85 | -        child_two.wit.vtxinwit.append(CTxInWitness())
  86 | +        child_two = copy.deepcopy(child_one)

sriramdvt commented at 6:06 AM on July 24, 2021:

Importing copy using import copy at the start might solve the failing tests. It might be a better idea to import only deepcopy using from copy import deepcopy and then using the function directly here.

        child_two = deepcopy(child_one)

There are some other errors that I am getting in "Submit child_two to the mempool" even after making this change on my local machine.

brunoerg commented at 4:39 PM on July 24, 2021: member

@michaelfolkson gave me a tip in my first PR that might be helpful for you:

You can do a reset on your branch to the current top commit on master, then re-adding your commit and force pushing.

git reset --hard insert_current_top_commit_hash_on_master

Then add a single commit on top and force pushing
jnewbery commented at 10:04 AM on July 26, 2021: member

Concept ACK

Did you consider also doing:

I think the simplest parent witness script to create same-nonwitness-data txs would be OP_DROP OP_TRUE, with a different arbitrary witness stack element in each child transaction. OTOH multiple script branches are far more realistic, I guess that's the primary scenario :)

(https://github.com/bitcoin/bitcoin/pull/22253#pullrequestreview-703027841)
theStack commented at 2:26 PM on July 30, 2021: member

Concept ACK

Thanks for picking this up and welcome as a new contributor! 🎉 Happy to review/test as soon as the CI is green :)

in test/functional/mempool_accept_wtxid.py:9 in 1514930337 outdated

   3 | @@ -4,9 +4,9 @@
   4 |  # file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 |  """
   6 |  Test mempool acceptance in case of an already known transaction
   7 | -with identical non-witness data different witness.
   8 | +with identical non-witness data but different witness.
   9 |  """
  10 | -
  11 | +import copy

glozow commented at 9:30 AM on August 2, 2021:

Per PEP8, you want a line between standard library and local imports.

Also, as sriramdvt mentioned earlier, a more granular import of just deepcopy would be preferred.

in test/functional/mempool_accept_wtxid.py:104 in 1514930337 outdated

 100 | @@ -104,13 +101,12 @@ def run_test(self):
 101 |              "allowed": False,
 102 |              "reject-reason": "txn-already-in-mempool"
 103 |          }])
 104 | -        testres_child_two = node.testmempoolaccept([child_two.serialize().hex()])[0]
 105 | -        assert_equal(testres_child_two, {
 106 | +        assert_equal(node.testmempoolaccept([child_two.serialize().hex()]), [{

glozow commented at 9:32 AM on August 2, 2021:

Keeping the index would reduce the diff

        assert_equal(node.testmempoolaccept([child_two.serialize().hex()])[0], {

naiza2000 commented at 1:25 AM on August 3, 2021:

Done!

glozow commented at 1:33 PM on August 2, 2021: member

(Also needs a squash)

Improve mempool_accept_wtxid.py

Improve mempool_accept_wtxid.py

Improve mempool_accept_wtxid.py

Improve mempool_accept_wtxid.py

Improve mempool_accept_wtxid.py

91b05974fc

glozow commented at 8:55 AM on August 3, 2021: member

utACK 91b05974fc1ea38062f12d36152201af81bda1a2
MarcoFalke merged this on Aug 3, 2021
MarcoFalke closed this on Aug 3, 2021
theStack commented at 12:29 PM on August 3, 2021: member

post-merge ACK 91b05974fc1ea38062f12d36152201af81bda1a2 🍺

nit: not a big issue, but next time, please take care of the commit message body, the 4x"Improve mempool_accept_wtxid.py" doesn't contain any information :)
naiza2000 commented at 3:45 PM on August 3, 2021: contributor

post-merge ACK 91b0597 🍺

nit: not a big issue, but next time, please take care of the commit message body, the 4x"Improve mempool_accept_wtxid.py" doesn't contain any information :)

Oh yes, thank you for the guidance. Will certainly take care of that in the future.
sidhujag referenced this in commit 4fb6f0ee51 on Aug 4, 2021
DrahtBot locked this on Aug 18, 2022

Contributors

Labels

Linked (view graph)

#22253 validation: distinguish between same tx and same-nonwitness-data tx in mempool