test: Add test for conflicted wallet tx notifications #18878

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/nonot changing 1 files +67 −1
  1. ryanofsky commented at 3:20 AM on May 5, 2020: contributor

    Add test coverage for conflicted wallet transaction notifications so we can improve current behavior and avoid future regressions

    #9240 - accidental break #9479 - bug report #9371 - fix #16624 - accidental break #18325 - bug report #18600 - potential fix

  2. test: Add test for conflicted wallet tx notifications
    Add test coverage for conflicted wallet transaction notifications so we improve
current behavior and avoid future regressions

https://github.com/bitcoin/bitcoin/pull/9240 - accidental break
https://github.com/bitcoin/bitcoin/issues/9479 - bug report
https://github.com/bitcoin/bitcoin/pull/9371 - fix
https://github.com/bitcoin/bitcoin/pull/16624 - accidental break
https://github.com/bitcoin/bitcoin/issues/18325 - bug report
https://github.com/bitcoin/bitcoin/pull/18600 - potential fix
    f963a68051
  3. fanquake added the label Tests on May 5, 2020
  4. DrahtBot commented at 9:24 AM on May 5, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #15861 (Restore warning for individual unknown version bits, as well as unknown version schemas by luke-jr)

    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.

  5. MarcoFalke added the label Needs backport (0.20) on May 5, 2020
  6. MarcoFalke added this to the milestone 0.20.0 on May 5, 2020
  7. DrahtBot cross-referenced this on May 5, 2020 from issue Restore warning for individual unknown version bits, as well as unknown version schemas by luke-jr
  8. in test/functional/feature_notifications.py:93 in 2c1c162cd5 outdated 
      88 | +
  89 | +            # Conflicting transactions tests. Give node 0 same wallet seed as
  90 | +            # node 1, generate spends from node 0, and check notifications
  91 | +            # triggered by node 1
  92 | +            self.log.info("test -walletnotify with conflicting transactions")
  93 | +            self.nodes[0].sethdseed(True, self.nodes[1].dumpprivkey(keyhash_to_p2pkh(hex_str_to_bytes(self.nodes[1].getwalletinfo()['hdseedid'])[::-1])))
    MarcoFalke commented at 2:54 PM on May 11, 2020: 
                self.nodes[0].sethdseed(seed= self.nodes[1].dumpprivkey(keyhash_to_p2pkh(hex_str_to_bytes(self.nodes[1].getwalletinfo()['hdseedid'])[::-1])))

    nit: Could either leave this out or use named arguments newkeypool, so that people like me don't have to look this up in the docs?

    ryanofsky commented at 11:44 PM on May 12, 2020:

    re: #18878 (review)

    nit: Could either leave this out or use named arguments newkeypool, so that people like me don't have to look this up in the docs?

    Thanks, updated

  9. in test/functional/feature_notifications.py:102 in 2c1c162cd5 outdated 
      97 | +            # Generate transaction on node 0, sync mempools, and check for
  98 | +            # notification on node 1.
  99 | +            tx1 = self.nodes[0].sendtoaddress(address=ADDRESS_BCRT1_UNSPENDABLE, amount=1, replaceable=True)
 100 | +            self.sync_mempools()
 101 | +            assert_equal(tx1 in self.nodes[0].getrawmempool(), True)
 102 | +            assert_equal(tx1 in self.nodes[1].getrawmempool(), True)
    MarcoFalke commented at 2:56 PM on May 11, 2020:

    Are those two needed? sync_mempools should already do the assert

    ryanofsky commented at 11:51 PM on May 12, 2020:

    re: #18878 (review)

    Are those two needed? sync_mempools should already do the assert

    Thanks, replaced with a more compact mempool check (seems good for clarity & debugging to verify the transaction reached the mempool)

  10. in test/functional/feature_notifications.py:112 in 2c1c162cd5 outdated 
     107 | +            # https://github.com/bitcoin/bitcoin/pull/9371, it might be better
 108 | +            # to have notifications for both tx1 and bump1.
 109 | +            bump1 = self.nodes[0].bumpfee(tx1)["txid"]
 110 | +            self.sync_mempools()
 111 | +            assert_equal(bump1 in self.nodes[0].getrawmempool(), True)
 112 | +            assert_equal(bump1 in self.nodes[1].getrawmempool(), True)
    MarcoFalke commented at 2:57 PM on May 11, 2020:

    Same

  11. in test/functional/feature_notifications.py:126 in 2c1c162cd5 outdated 
     121 | +
 122 | +            # Generate a second transaction to be bumped.
 123 | +            tx2 = self.nodes[0].sendtoaddress(address=ADDRESS_BCRT1_UNSPENDABLE, amount=1, replaceable=True)
 124 | +            self.sync_mempools()
 125 | +            assert_equal(tx2 in self.nodes[0].getrawmempool(), True)
 126 | +            assert_equal(tx2 in self.nodes[1].getrawmempool(), True)
    MarcoFalke commented at 2:58 PM on May 11, 2020:

    Same

  12. MarcoFalke approved
  13. MarcoFalke commented at 3:00 PM on May 11, 2020: member

    Nice! ACK

  14. jonatack commented at 1:26 PM on May 12, 2020: contributor

    ACK 2c1c162cd5f60e649d

  15. ryanofsky force-pushed on May 13, 2020
  16. ryanofsky commented at 1:42 AM on May 13, 2020: contributor

    Thanks for reviews, updated 2c1c162cd5f60e649d9d55f6c99301c4985d5bab -> f963a680515eda66429b3d1537a7baf281ab9283 (pr/nonot.2 -> pr/nonot.3, compare) with suggestions

  17. fanquake requested review from MarcoFalke on May 13, 2020
  18. laanwj commented at 1:27 PM on May 13, 2020: member

    Good to have a test for this, it always used to be somewhat ill-defined functionality, apparently breaking many times (reading that list hurts!).

    ACK f963a680515eda66429b3d1537a7baf281ab9283

  19. jonatack commented at 1:30 PM on May 13, 2020: contributor

    re-ACK f963a680515eda66429b3d1537a7baf281ab9283

  20. MarcoFalke commented at 1:30 PM on May 13, 2020: member

    ACK f963a680515eda66429b3d1537a7baf281ab9283

  21. laanwj merged this on May 13, 2020
  22. laanwj closed this on May 13, 2020
  23. MarcoFalke cross-referenced this on May 13, 2020 from issue [wallet] Track conflicted transactions removed from mempool and fix UI notifications by ariard
  24. sidhujag referenced this in commit a84dd1fa2e on May 14, 2020
  25. fanquake referenced this in commit 9c193e4d0a on May 14, 2020
  26. fanquake removed the label Needs backport (0.20) on May 14, 2020
  27. fanquake cross-referenced this on May 14, 2020 from issue [0.20] Final backports for rc2 by fanquake
  28. fanquake referenced this in commit ed0afe8c1f on May 15, 2020
  29. MarcoFalke referenced this in commit 17bdf2afae on May 15, 2020
  30. random-zebra cross-referenced this on Mar 3, 2021 from issue Killing wallet and GUI cs_main locks by furszy
  31. backpacker69 referenced this in commit 364a2d33dd on Mar 28, 2021
  32. Fabcien referenced this in commit 2f45cfea78 on Aug 30, 2021
  33. bitcoin locked this on Feb 15, 2022
Contributors
ryanofskyDrahtBotMarcoFalkejonatacklaanwj


MarcoFalke

Labels
Tests

Milestone
0.20.0

Linked (view graph)
#9240 Remove txConflicted#9371 Notify on removal#9479 -walletnotify isn't fired on transactions that were in the mempool and then evicted due to a tx in a new block conflicting them.#16624 wallet: encapsulate transactions state#18325 Restore UI notifications and -walletnotify behavior for block conflicted transactions#18600 [wallet] Track conflicted transactions removed from mempool and fix UI notifications#18973 [0.20] Final backports for rc2
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:54 UTC