Skip to content

Conversation

aakselrod
Copy link
Contributor

@aakselrod aakselrod commented Jul 11, 2017

This PR adds a new test to make sure reorganizations work correctly.

@mention-bot
Copy link

@aakselrod, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Roasbeef, @tmc and @bryanvu to be potential reviewers.

@Roasbeef
Copy link
Member

Once this is rebased, it'll be able to build against the latest version of master which has the dependent changes of btcwallet merged in.

@aakselrod aakselrod force-pushed the reorg-test branch 3 times, most recently from 1882407 to ca2f81b Compare July 15, 2017 00:16
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent test addition! This should serve as a guard against future regressions within the btcwallet implementation of the WalletController, and also to ensure that any other future WalletController implementations properly handle large repeated re-orgs.

My comments are pretty minor, so once this is rebased to the latest master, we should be able to get it merged quickly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit: missing a new line above here, and also below the call to Generate. There're a few other instances of this within this PR. However, to avoid notification spam, I'll only comment once.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit: missing a new line above here. And also below the call to Generate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the latest change to the way the these tests are run, this logging statement is no longer needed. When you rebase to master, you'll instead tag a name onto this test case at the bottom of this file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than a hard coded sleep here (which may have weird issues when running on Travis), we can instead use the IsSynced method on the WalletController interface. The same swap can be repeated several times in this test case.

@aakselrod aakselrod changed the title lnwallet: add reorg test (incomplete) lnwallet: add reorg test Aug 3, 2017
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left over from debugging?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good catch. Thanks! Removing now.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@Roasbeef Roasbeef merged commit 0994852 into lightningnetwork:master Oct 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants