Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coop-close channel integration testing #37

Conversation

vincenzopalazzo
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo commented Mar 3, 2022

Build on top of #47

This is one of my first attempts to implement the interoperability test for the shutdown workflow described in BOLT2

This should clarify the idea on the implementation side about the following discussion lightning/bolts#964 and lightning/bolts#970

This will override the c-lightning CI flaky test https://github.com/ElementsProject/lightning/pull/4985/files/93f8a11bf60ad9809ced69c9be0ae7ccf922b2eb..7530018014128a24a42f816ab0676cb85f9e96c8#diff-72d78e7a968ac6b078b7a2d84e56d2e026c54f60e56781f878664bcaae4b2bfbR3441

@vincenzopalazzo vincenzopalazzo added the enhancement New feature or request label Mar 3, 2022
@vincenzopalazzo vincenzopalazzo marked this pull request as draft March 3, 2022 14:11
vincenzopalazzo added a commit that referenced this pull request Mar 9, 2022
66b5a94 lnprototest: remove the default __enter__ __exit__ implementation (Vincenzo Palazzo)
fd9bae5 lnprototest: remove info logging when it is not necessary (Vincenzo Palazzo)
c7144c5 doc: adding log information in the  Readme (Vincenzo Palazzo)
8ff83a9 ci: during the test in the ci increase verbosity of the tests (Vincenzo Palazzo)
363b9e5 lnprototest: adding teardown directory at the end of the tests (Vincenzo Palazzo)
4b4823c fmt: formatting code (Vincenzo Palazzo)
e0cce0a lnprototest: refactroing abstract class and fixed the #14 (Vincenzo Palazzo)

Pull request description:

  kill all the processes when the class was removed from the scope.

  Fixes #14

  This is a refactoring PR cherry-pitched by the PR #37 but it is cleaner to introduce these changes in another PR.

  Signed-off-by: Vincenzo Palazzo <[email protected]>

Top commit has no ACKs.

Tree-SHA512: f2bd5f609caecd9a9bb3bfcd4f02fe7bd73a69a63265c5ee9fca73b7c34116ea044c84fabec0c3d009f7604e63121c0d18fa0f54d5eda19cf730130d8f55d0d1
@vincenzopalazzo vincenzopalazzo force-pushed the vincenzopalazzo/bolt2_close_channel branch 2 times, most recently from c464f7d to f202fb3 Compare March 15, 2022 16:47
@vincenzopalazzo vincenzopalazzo added SPEC Related to lightning network specification SPEC: BOLT 2 Related to BOLT 2 in general and removed enhancement New feature or request labels Mar 15, 2022
@vincenzopalazzo vincenzopalazzo self-assigned this Mar 15, 2022
@vincenzopalazzo vincenzopalazzo force-pushed the vincenzopalazzo/bolt2_close_channel branch 3 times, most recently from 42c10dc to 4ca53ae Compare March 15, 2022 17:10
@michaelfolkson
Copy link

Concept ACK. More testing around cooperative closing is good :)

The motivation for this (as I understand from the c-lightning call) is when a lnprototest test fails bitcoind isn't stopped? Firstly I'm not sure why bitcoind should be stopped if a lnprototest test fails (I guess it depends on the test) and secondly I'm not sure how the code in this PR resolves that. Can you explain?

@vincenzopalazzo
Copy link
Collaborator Author

Concept ACK. More testing around cooperative closing is good :)

Yes, also because we have a little bit of confusion across definition of coop-close in the spec like lightning/bolts#970 (comment) and lightning/bolts#964

The motivation for this (as I understand from the c-lightning call) is when a lnprototest test fails bitcoind isn't stopped? Firstly I'm not sure why bitcoind should be stopped if a lnprototest test fails (I guess it depends on the test) and secondly I'm not sure how the code in this PR resolves that. Can you explain?

Sorry if I miss the point to the PR, this problem is fixed by the following PR #38 In particular if a test fails or lnprototest was not able to clear the work dir in the /tmp and sto bitcoind to work because the following code was missing

def shutdown(self) -> None:
for cb in self.cleanup_callbacks:
cb()
self.rpc.stop()
self.bitcoind.stop()

and also all the lnprototest flow between start -> shutdown -> stop was rewritten

@vincenzopalazzo vincenzopalazzo force-pushed the vincenzopalazzo/bolt2_close_channel branch 4 times, most recently from 442f33e to 3932da3 Compare March 24, 2022 10:23
@vincenzopalazzo vincenzopalazzo changed the title [WIP] coop-close channel integration testing coop-close channel integration testing Mar 24, 2022
@vincenzopalazzo vincenzopalazzo force-pushed the vincenzopalazzo/bolt2_close_channel branch from 3932da3 to 7f1ba78 Compare March 24, 2022 10:27
@vincenzopalazzo vincenzopalazzo force-pushed the vincenzopalazzo/bolt2_close_channel branch 8 times, most recently from c1ff0a1 to 1c6c016 Compare March 30, 2022 16:40
@vincenzopalazzo vincenzopalazzo force-pushed the vincenzopalazzo/bolt2_close_channel branch 2 times, most recently from f51122b to 4b1a993 Compare September 19, 2022 15:48
@vincenzopalazzo vincenzopalazzo force-pushed the vincenzopalazzo/bolt2_close_channel branch from 4b1a993 to ed9d1d6 Compare September 19, 2022 15:48
@vincenzopalazzo
Copy link
Collaborator Author

ovveride by #62

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SPEC: BOLT 2 - Close Channel Related to BOLT 2, with more focus to the close channel side SPEC: BOLT 2 Related to BOLT 2 in general SPEC Related to lightning network specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants