test: Add test for conflicted wallet tx notifications

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

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

fanquake added the label Tests on May 5, 2020

DrahtBot commented at 9:24 am on May 5, 2020: member

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

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.

MarcoFalke added the label Needs backport (0.20) on May 5, 2020

MarcoFalke added this to the milestone 0.20.0 on May 5, 2020

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:

0            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

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)

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

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

MarcoFalke approved

MarcoFalke commented at 3:00 pm on May 11, 2020: member

Nice! ACK

jonatack commented at 1:26 pm on May 12, 2020: member

ACK 2c1c162cd5f60e649d

ryanofsky force-pushed on May 13, 2020

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

fanquake requested review from MarcoFalke on May 13, 2020

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

jonatack commented at 1:30 pm on May 13, 2020: member

re-ACK f963a680515eda66429b3d1537a7baf281ab9283

MarcoFalke commented at 1:30 pm on May 13, 2020: member

ACK f963a680515eda66429b3d1537a7baf281ab9283

laanwj merged this on May 13, 2020

laanwj closed this on May 13, 2020

sidhujag referenced this in commit a84dd1fa2e on May 14, 2020

fanquake referenced this in commit 9c193e4d0a on May 14, 2020

fanquake removed the label Needs backport (0.20) on May 14, 2020

fanquake referenced this in commit ed0afe8c1f on May 15, 2020

MarcoFalke referenced this in commit 17bdf2afae on May 15, 2020

backpacker69 referenced this in commit 364a2d33dd on Mar 28, 2021

Fabcien referenced this in commit 2f45cfea78 on Aug 30, 2021

DrahtBot locked this on Feb 15, 2022

test: Add test for conflicted wallet tx notifications #18878

Conflicts