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

Only act on confirmed spends #1531

Merged
merged 15 commits into from Jul 24, 2018

Conversation

Projects
None yet
4 participants
@halseth
Copy link
Collaborator

halseth commented Jul 10, 2018

This PR removes the option to register spend notifications that will be triggered on spends detected in the mempool. This is done to ensure unified behavior across all backends, as they don't necessarily (most notably Neutrino) have a mempool.

This is a step in enabling integration tests also with bitcoind and neutrino backends.

Structure

The commits are linearly ordered in such a way that one–by–one spend registration is changed over from a mempool spend to a confirmed spend, accompanied by any test changes that are needed to make them pass. This means that any commit that doesn't have a corresponding test change, should pass the tests without additional changes.

Finally we remove the option for mempool spends altogether, when all usages are eliminated.

Integration test changes

The most notable test change is modifications to lnd_test, which must be modified to account for the nodes not reacting to spends before they are confirmed. Additional comments and checks are added to make sure this works as expected.

TODO

  • Why are we mining CSV_DELAY + 1 blocks to trigger act on force closes?
  • Must be rebased on #1554

@Roasbeef Roasbeef added the P2 label Jul 11, 2018

@Roasbeef Roasbeef requested a review from wpaulino Jul 11, 2018

@Roasbeef
Copy link
Member

Roasbeef left a comment

Nice! Looking forward to finally removing mempool dispatching all together! There're a few areas where I think we may still want to allow the callers to still dispatch based on confirmations.

return fmt.Errorf("quitting")
}

// Now that the output has been spent, we'll also wait for the

This comment has been minimized.

@Roasbeef

Roasbeef Jul 13, 2018

Member

We still may want to actually wait for an additional, or set of additional confirmations...

This comment has been minimized.

@halseth

halseth Jul 16, 2018

Collaborator

Note that this is equivalent to the previous logic:

Before this commit we wait for a spend (any spend, even mempool), then check that it has at least 1 conf.
After this commit we wait for a confirmed spend.

Are you suggestion that we should wait for additional confirmations of the spend? Should we do 6 conf?

This comment has been minimized.

@Roasbeef

Roasbeef Jul 21, 2018

Member

Yes suggesting that we may want to wait for additional confirmations. This isn't a blocker for this PR though, as it would be a project wide change if we went down this route.

@@ -514,13 +514,16 @@ func (c *ChainArbitrator) Stop() error {
// NOTE: This must be launched as a goroutine.
func (c *ChainArbitrator) watchForChannelClose(closeInfo *channeldb.ChannelCloseSummary) {
spendNtfn, err := c.cfg.Notifier.RegisterSpendNtfn(
&closeInfo.ChanPoint, closeInfo.CloseHeight, true,
&closeInfo.ChanPoint, closeInfo.CloseHeight, false,

This comment has been minimized.

@Roasbeef

Roasbeef Jul 13, 2018

Member

Similar comment here w.r.t also enabling callers to wait on an additional set of confirmations.

@@ -5551,40 +5551,65 @@ func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness,
// Query the mempool for Dave's justice transaction, this should be
// broadcast as Carol's contract breaching transaction gets confirmed
// above. Since Carol might have had the time to take some of the HTLC
// outputs to the second level before Alice broadcasts her justice tx,
// outputs to the second level before Dave broadcasts his justice tx,

This comment has been minimized.

@Roasbeef

Roasbeef Jul 13, 2018

Member

Broadcast failure? Why's it possible for this to happen? Is it due to the new requirement of ensue the second level spends are now confirmed?

This comment has been minimized.

@halseth

halseth Jul 16, 2018

Collaborator

Yes, since Dave would previously act directly on the second level spend in the mempool, this would eventually resolve. Now, with the removal of mempool spend checks, we need to mine an additional block for Dave to get notified of the second level tx.

lnd_test.go Outdated
return true
}, time.Second*10)
if err != nil && predErr == errNotFound {
fmt.Println("not found, mine new block")

This comment has been minimized.

@Roasbeef

Roasbeef Jul 13, 2018

Member

Stray print statement.

@halseth

This comment has been minimized.

Copy link
Collaborator

halseth commented Jul 16, 2018

Blocked by #1554

@halseth halseth force-pushed the halseth:only-vonfirmed-spends branch from 57a3b1c to 8d548e1 Jul 16, 2018

go func() {
err := hn.cmd.Wait()
defer hn.wg.Done()

This comment has been minimized.

@wpaulino

wpaulino Jul 17, 2018

Collaborator

👍

case <-spentIntent.Spend:
t.Fatalf("did not expect to get notification before " +
"block was mined")
case <-time.After(50 * time.Millisecond):

This comment has been minimized.

@wpaulino

wpaulino Jul 17, 2018

Collaborator

Perhaps we should make this a bit longer to ensure the notification was never sent?

@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Jul 17, 2018

Needs a rebase now that #1554 has landed.

@Roasbeef Roasbeef added this to the 0.5 milestone Jul 17, 2018

@halseth halseth force-pushed the halseth:only-vonfirmed-spends branch 2 times, most recently from d9e98b3 to 3cfbae9 Jul 17, 2018

@halseth

This comment has been minimized.

Copy link
Collaborator

halseth commented Jul 17, 2018

Rebased.

@halseth halseth force-pushed the halseth:only-vonfirmed-spends branch from 3cfbae9 to ac00d08 Jul 17, 2018

@cfromknecht
Copy link
Collaborator

cfromknecht left a comment

@halseth nice set of changes, should be a big improvement in reliability of our persistent state machines!

Any hunches into why we need a +1 in the itests?

@Roasbeef
Copy link
Member

Roasbeef left a comment

After a rebase....

LGTM 🥁

Travis failure on current build is a known flake which should be fixed by one of the suite of pending PR's that claim to address flakes present in Travis.

@halseth halseth force-pushed the halseth:only-vonfirmed-spends branch from ac00d08 to cbba88c Jul 21, 2018

@halseth

This comment has been minimized.

Copy link
Collaborator

halseth commented Jul 21, 2018

Rebased! 💪

For the record: we discussed the reason for mining +1 blocks during tests, and this is because the UTXO nursery broadcasts the transaction one block after it is actually first valid. We decided to keep this as is, since it doesn't really matter and many of the tests are written with this in mind. I modified my tests to match the patter: mine a block that includes the CSV locked tx, then mine CSV_DELAY number of blocks before sweep.

halseth added some commits Jul 17, 2018

@halseth halseth force-pushed the halseth:only-vonfirmed-spends branch from cbba88c to e8c62ca Jul 22, 2018

@halseth

This comment has been minimized.

Copy link
Collaborator

halseth commented Jul 22, 2018

Travis started failing after rebase on master. Investigating...

halseth added some commits Jul 17, 2018

halseth added some commits Jul 17, 2018

@halseth halseth force-pushed the halseth:only-vonfirmed-spends branch from e8c62ca to 181014c Jul 22, 2018

@halseth

This comment has been minimized.

Copy link
Collaborator

halseth commented Jul 22, 2018

We green 💚

@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 🌤

@Roasbeef Roasbeef merged commit 2eeced5 into lightningnetwork:master Jul 24, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 54.644%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment