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: member

    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: member

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

  8. 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)

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

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

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

    Nice! ACK

  13. jonatack commented at 1:26 PM on May 12, 2020: member

    ACK 2c1c162cd5f60e649d

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

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

  16. fanquake requested review from MarcoFalke on May 13, 2020
  17. 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

  18. jonatack commented at 1:30 PM on May 13, 2020: member

    re-ACK f963a680515eda66429b3d1537a7baf281ab9283

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

    ACK f963a680515eda66429b3d1537a7baf281ab9283

  20. laanwj merged this on May 13, 2020
  21. laanwj closed this on May 13, 2020
  22. sidhujag referenced this in commit a84dd1fa2e on May 14, 2020
  23. fanquake referenced this in commit 9c193e4d0a on May 14, 2020
  24. fanquake removed the label Needs backport (0.20) on May 14, 2020
  25. fanquake referenced this in commit ed0afe8c1f on May 15, 2020
  26. MarcoFalke referenced this in commit 17bdf2afae on May 15, 2020
  27. backpacker69 referenced this in commit 364a2d33dd on Mar 28, 2021
  28. Fabcien referenced this in commit 2f45cfea78 on Aug 30, 2021
  29. DrahtBot 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-02 18:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me