Page MenuHomePhabricator

Avoid sync_all in rpc_listtransactions
AbandonedPublic

Authored by Fabien on Feb 28 2019, 15:52.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

sync_all() is used through the test to wether check for block or tx
sync, making it unclear what the test is actually checking.
Using wait_for_tip() and wait_for_tx() makes it slightly clearer.

Depends on D2629

Test Plan
./test/functional/test_runner.py rpc_listtransactions

Diff Detail

Repository
rABC Bitcoin ABC
Branch
rpc_listtransaction_remove_sync_all
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5108
Build 8279: Bitcoin ABC Buildbot (legacy)
Build 8278: arc lint + arc unit

Event Timeline

Why is that better at all to explicit a bunch of things that are not at all related to listtransactions, which presumably is what is checked ?

The main intent is to make more clear what step relies on what data to propagate. sync_all() always do both wait_for_tip() and wait_for_tx() every time, I found that separating these makes the test more easy to follow. As a bonus it eventually avoids having the test to pass due to side effects of sync_all() syncing both blocks and mempools when only one is expected, and should makes the test marginally faster.

deadalnix requested changes to this revision.Mar 1 2019, 13:58

The API surface this overs now has increase quite a bit - so the test will be more fragile, it's unclear what side effect of sync_all would cause this test to pass erroneously as this test is not testing how node synchronize, and the fact that marginally faster is not measured makes it a dubious argument at best.

I do not see a good reason to take over this test at this stage rather than simply backporting.

This revision now requires changes to proceed.Mar 1 2019, 13:58