Page MenuHomePhabricator

Avoid sync_all in rpc_listtransactions

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


Group Reviewers
Restricted Project

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/ rpc_listtransactions

Diff Detail

rABC Bitcoin ABC
Lint OK
No Unit Test Coverage
Build Status
Buildable 5108
Build 8279: Bitcoin ABC Teamcity Staging
Build 8278: arc lint + arc unit

Event Timeline

Fabien created this revision.Feb 28 2019, 15:52
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 28 2019, 15:53
Herald added a subscriber: schancel. · View Herald Transcript

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 ?

Fabien added a comment.Feb 28 2019, 18:01

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
Fabien abandoned this revision.Mar 1 2019, 14:14

Concept NAK