Page MenuHomePhabricator

[electrum] add a functional test for a reorg situation

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



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/

Diff Detail

rABC Bitcoin ABC
Lint Not Applicable
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Tue, Nov 14, 15:46
63 ↗(On Diff #43079)

$ as a JSONPath means the root element of the JSON response (which here is just a boolean)

See 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.Wed, Nov 15, 08:24
Fabien added a subscriber: Fabien.
Fabien added inline comments.
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.Wed, Nov 15, 08:24
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.

add a wait_until helper and add comments to clarify why the block height is 0

better test_function name

This revision is now accepted and ready to land.Wed, Nov 15, 11:41