Randomize chain source selection in tests #769
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It's weird to have a special intermediary `setup_node` method if we have `TestConfig` for exactly that reason by now. So we move `async_payment_role` over.
.. all of our tests should be robust against switching chain sources. We here opt to pick a random one each time to considerably extend our test coverage, instead of just running some cases against non-Esplora chain sources.
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
Collaborator
Author
|
This will be flaky, at the very least until lightningdevkit/rust-lightning#4341 gets release, which would have been caught if we'd ever had run the 0conf test case with an Electrum chain source. |
TheBlueMatt
previously approved these changes
Jan 23, 2026
Contributor
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
makes sense to me. Hopefully CI fails until lightningdevkit/rust-lightning#4341
Collaborator
Author
I think I'll whack-a-mole a few more flakes/bugs before landing this. |
When we intially implemented `bitcoind` syncing polling the mempool was very frequent and rather inefficient so we made a choice not to unnecessarily update the payment store for mempool changes, especially since we only consider transactions `Succeeded` after `ANTI_REORG_DELAY` anyways. However, since then we made quite a few peformance improvements to the mempool syncing, and by now we should just update they payment store as not doing so will lead to rather unexpected behavior, making some tests fail for `TestChainSource::Bitcoind`, e.g., `channel_full_cycle_0conf`, which we fix here.
Collaborator
Author
|
Previously had failed, I want to double-check them once more before moving forward here. |
Previously, we fixed than a fresh node syncing via `bitcoind` RPC would resync all chain data back to genesis. However, while introducing a wallet birthday is great, it disallowed discovery of historical funds if a wallet would be imported from seed. Here, we add a recovery mode flag to the builder that explictly allows to re-enable resyncing from genesis in such a scenario. Going forward, we intend to reuse that API for an upcoming Lightning recoery flow, too.
tnull
force-pushed
the
2026-01-test-setup
branch
from
96b6f29 to
b8179ca
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
.. all of our tests should be robust against switching chain sources. We
here opt to pick a random one each time to considerably extend our test
coverage, instead of just running some cases against non-Esplora chain
sources.