Test transactions conflicted by double spend in listtransactions #23146

pull jonatack wants to merge 1 commits into bitcoin:master from jonatack:test-txns-conflicted-by-double-spend-in-listtransactions changing 1 files +29 −0

jonatack commented at 3:26 PM on September 30, 2021: member

Test the properties of transactions conflicted by a double spend as returned by RPC listtransactions in the abandoned, confirmations, trusted and walletconflicts fields. These fields are also returned by RPCs listsinceblock and gettransactions.
DrahtBot added the label Tests on Sep 30, 2021
brunoerg commented at 11:02 AM on October 1, 2021: member

tACK c43b7f4b774fc414fbecdf5edf1f3d912ef0f0f1

Test transactions conflicted by double spend in listtransactions

Test the properties of transactions conflicted by a double
spend as returned by RPC listtransactions in the "abandoned",
"confirmations", "trusted" and "walletconflicts" fields.

These fields are also returned by RPCs listsinceblock and
gettransactions.

502f50da12

jonatack force-pushed on Oct 2, 2021
jonatack commented at 1:30 PM on October 2, 2021: member

Thanks @brunoerg! I pushed an improved version of the test per git diff c43b7f4 502f50d.
brunoerg commented at 9:35 PM on October 2, 2021: member

tACK 502f50da12694dd0193744427f0f89238e612f54
rajarshimaitra commented at 5:55 PM on October 3, 2021: contributor

Concept + tACK https://github.com/bitcoin/bitcoin/pull/23146/commits/502f50da12694dd0193744427f0f89238e612f54

Will give another deeper pass to ponder over the behavior.
in test/functional/wallet_abandonconflict.py:172 in 502f50da12
```
 163 | @@ -163,6 +164,34 @@ def run_test(self):
 164 |          self.connect_nodes(0, 1)
 165 |          self.sync_blocks()
 166 |  
 167 | +        tx_list = self.nodes[0].listtransactions()
 168 | +
 169 | +        conflicted = [tx for tx in tx_list if tx["confirmations"] < 0]
 170 | +        assert_equal(4, len(conflicted))
 171 | +
 172 | +        wallet_conflicts = [tx for tx in conflicted if tx["walletconflicts"]]
```
rajarshimaitra commented at 7:38 PM on October 4, 2021:

Probably it would read a little clearly if instead of counting walletconflicts in conflicted, we count it in tx_list?

That will give 3 wallet conflicts. Not that its important for assertion, because the third conflict is just asserted below this. But it did confused me for a moment in first pass why only 2 walletconflictsinstead of 3.

jonatack commented at 9:50 AM on October 5, 2021:

That would mean iterating through the (quite a bit longer) tx_list, and I only want to look through the conflicted transactions here to check them versus the double spend. Happy to look at a code proposal though.

rajarshimaitra commented at 7:57 PM on October 5, 2021:

Ya I think that makes sense, we do need to iterate over a longer list then. Its not that important. It was a dumb confusion I had while reading. But the code is correct.
rajarshimaitra commented at 7:39 PM on October 4, 2021: contributor

Code review ACK.

Verified that the intended categories are taking changing as expected. There can be few more redundant checks added, like doing the same thing for other RPC calls that return category. But probably not that useful.

I do have some general comments around rest of the tests, but that's outside of the scope of this PR, and better done elsewhere.

Just one ignore-able nit
MarcoFalke merged this on Oct 7, 2021
MarcoFalke closed this on Oct 7, 2021
jonatack deleted the branch on Oct 7, 2021
sidhujag referenced this in commit b4975f657c on Oct 7, 2021
PastaPastaPasta referenced this in commit bcb9cb546c on Apr 3, 2022
DrahtBot locked this on Oct 30, 2022

Contributors

Labels