[electrum] add a functional test for a reorg situation
ClosedPublic
Actions

Authored by PiRK on Nov 14 2023, 15:46.

Details

Reviewers

Fabien
bytesofman

Group Reviewers

Restricted Project

Commits

rABCbfb68bb3d2da: [electrum] add a functional test for a reorg situation

Summary

This tests an important feature: how the wallet handles changes in transaction history caused by funny business on-chain.

Test Plan

pytest electrumabc/tests/regtest/test_reorg.py

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Nov 14 2023, 15:46

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 14 2023, 15:46

PiRK requested review of this revision.Nov 14 2023, 15:46

Harbormaster completed remote builds in B25614: Diff 43079.Nov 14 2023, 15:47

PiRK added a child revision: D14703: [electrum] Performance: make receive_history_callback faster for larger histories.Nov 14 2023, 15:58

PiRK added inline comments.Nov 14 2023, 16:03

electrum/electrumabc/tests/regtest/test_reorg.py
63 ↗	(On Diff #43079)	`$` as a JSONPath means the root element of the JSON response (which here is just a boolean) See https://goessner.net/articles/JsonPath/ for the documentation about JSONPath
98 ↗	(On Diff #43079)	Here the RPC call returns a list of dicts. So the JSON path gets the "height" field in the first dict. >>> from jsonpath_ng import parse >>> parse("[0].height") Child(Index(index=0), Fields('height'))

so is this a case of expected behavior just happening to be how electrum is currently working? Seems kinda fortunate.

The main reason for this test, besides demonstrating that we currently do the right thing, is to give us some more confidence that we are not breaking things when optimizing performance critical codepaths (D14703)

Fabien requested changes to this revision.Nov 15 2023, 08:24

Fabien added a subscriber: Fabien.

Fabien added inline comments.

electrum/electrumabc/tests/regtest/test_reorg.py
81 ↗	(On Diff #43079)	After reading it until the end I feel like this test would be easier to read if: All the code was using height relative to blockheight Even better: assert the height is what you expect (it should be constant on regtest) and just use hardcoded values. The test is small enough that it makes the expected behavior very explicit.
87 ↗	(On Diff #43079)	You could make a wait_until() function similar to what the test framework does to avoid code duplication and bugs, e.g. you're missing a time.sleep() in the loop
92 ↗	(On Diff #43079)	This could really be a single assert
95 ↗	(On Diff #43079)	Block height is == current tip height because the tx is back to mempool ?

This revision now requires changes to proceed.Nov 15 2023, 08:24

PiRK added inline comments.Nov 15 2023, 08:46

electrum/electrumabc/tests/regtest/test_reorg.py
81 ↗	(On Diff #43079)	For now the tests all share a same chain, and can run in any order. When I find a way to restart a clean instance of bitcoind + fulcrum between tests, hardcoding the expected block height will be done.