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

chainntnfs: re-enable height hint cache #2044

Merged
merged 9 commits into from Nov 1, 2018

Conversation

@wpaulino
Copy link
Collaborator

@wpaulino wpaulino commented Oct 12, 2018

In this PR, we attempt to re-enable the height hint cache for the different ChainNotifier implementations. This will lead to performance improvements upon startup when running a full node backend without txindex or when running as a light client, like Neutrino.

Depends on #1787 and #2004.

@wpaulino wpaulino force-pushed the enable-height-hint-cache branch 6 times, most recently from 951a8b2 to ee60405 Oct 15, 2018
@wpaulino
Copy link
Collaborator Author

@wpaulino wpaulino commented Oct 15, 2018

Ready for review! There are some additional commits (might seem unrelated) that solve some integration test flakes due to timing issues upon re-enabling the height hint cache.

One thing that's left to address, which was brought up by @cfromknecht, is whether we want to clear the existing state of users' caches. I believe this won't be needed in the case that users have successfully started up after the cache was disabled, as a rescan will occur from the broadcast height and catch everything since then. If this hasn't happened however, it's possible that they begin their rescan after the fact due to previous incorrect behavior of the cache.

@Roasbeef Roasbeef added this to the 0.5.1 milestone Oct 18, 2018
@wpaulino wpaulino force-pushed the enable-height-hint-cache branch 3 times, most recently from 024bf0e to 8ef6a5e Oct 24, 2018
@wpaulino wpaulino force-pushed the enable-height-hint-cache branch 3 times, most recently from f1de5fc to c17b73c Oct 29, 2018
chainntnfs/txnotifier.go Outdated Show resolved Hide resolved
chainntnfs/bitcoindnotify/bitcoind.go Outdated Show resolved Hide resolved
chainntnfs/bitcoindnotify/bitcoind.go Show resolved Hide resolved
// In addition to sending this edge's policy, we'll also
// attempt to send the backing node's announcement if we
// happen to know about it.
pubKey, _ := chanInfo.NodeKey1()
Copy link
Collaborator

@halseth halseth Oct 30, 2018

How is this change related to the rest of the PR? Could it be made its own PR?

Copy link
Collaborator Author

@wpaulino wpaulino Oct 31, 2018

It's related in that it solves some integration test issues that became more apparent with the cache enabled. Not opposed to making it its own PR - let me know what you think.

Copy link
Collaborator

@halseth halseth Oct 31, 2018

Yeah, let's make it a own PR, should be fairly easy to review and get in.

Copy link
Collaborator

@halseth halseth Oct 31, 2018

It should also have an accompanying test to demonstrate the problem.

Copy link
Collaborator Author

@wpaulino wpaulino Oct 31, 2018

Turns out these changes are actually no longer needed due to #1981!

@Roasbeef
Copy link
Member

@Roasbeef Roasbeef commented Oct 31, 2018

Needs a rebase!

@wpaulino wpaulino force-pushed the enable-height-hint-cache branch from c17b73c to 254c3ae Oct 31, 2018
chainntnfs/txnotifier.go Outdated Show resolved Hide resolved
// NotifyHeight dispatches confirmation and spend notifications to the clients
// who registered for a notification which has been fulfilled at the passed
// height.
func (n *TxNotifier) NotifyHeight(height uint32) error {
Copy link
Collaborator

@cfromknecht cfromknecht Oct 31, 2018

great addition to the interface, external consistency ftw :D

wpaulino added 3 commits Oct 31, 2018
…tTip

In this commit, we modify the TxNotifier's ConnectTip method to no
longer dispatch notifications to any clients who had a request fulfilled
within the height connected. Instead, it will queue the notifications
for dispatch and we add a new method NotifyHeight, which will actually
dispatch them. We do this to allow the users of the TxNotifier to be
more flexible when dispatching notifications.
In this commit, we alter the different ChainNotifier implementations to
dispatch confirmation and spend notifications after blocks. We do this
to ensure the external consistency of our registered clients.
@wpaulino wpaulino force-pushed the enable-height-hint-cache branch from 254c3ae to e402a4e Oct 31, 2018
@wpaulino
Copy link
Collaborator Author

@wpaulino wpaulino commented Oct 31, 2018

There shouldn't be any issues with the integration tests other than:

    --- FAIL: TestLightningNetworkDaemon/revoked_uncooperative_close_retribution_zero_value_remote_output (35.39s)
        lnd_test.go:74: Failed: (revoked uncooperative close retribution zero value remote output): exited with error:
            *errors.errorString unable to find Carol's force close tx in mempool: wanted 1, found 2 txs in mempool: [98075d6b639105d4ed259c77ed171c7e14f3d9daeb22fce5b31a5287668d52f2 b8b042732be88531c640f3f4179cfde1a5baf8a3cd5423c3df0723598c39c0e2]
            /Users/wilmer/go/src/github.com/lightningnetwork/lnd/lnd_test.go:6417 (0x17ade6a)
            	testRevokedCloseRetributionZeroValueRemoteOutput: t.Fatalf("unable to find Carol's force close tx in mempool: %v",
            /Users/wilmer/go/src/github.com/lightningnetwork/lnd/lnd_test.go:99 (0x1788ae6)
            	(*harnessTest).RunTestCase: testCase.test(net, h)
            /Users/wilmer/go/src/github.com/lightningnetwork/lnd/lnd_test.go:12591 (0x17e1c44)
            	TestLightningNetworkDaemon.func3: ht.RunTestCase(testCase, lndHarness)
            /usr/local/Cellar/go/1.11.1/libexec/src/testing/testing.go:827 (0x10f590f)
            	tRunner: fn(t)
            /usr/local/Cellar/go/1.11.1/libexec/src/runtime/asm_amd64.s:1333 (0x105dcb1)
            	goexit: BYTE	$0x90	// NOP

I rarely hit this on Travis though, can only reproduce it reliably locally. This will be resolved by dbc11c7 in #1621.

@halseth
Copy link
Collaborator

@halseth halseth commented Oct 31, 2018

Also note that I split the integration test fix into #2116, so we can get them in before doing the bigger multi-backend change.

@Roasbeef
Copy link
Member

@Roasbeef Roasbeef commented Oct 31, 2018

I don't think we need to worry about clearing any existing caches, as it was never included in a major release. Additionally, any entries that users had in their caches are now long expired since many weeks have passed since the initial implementation, and the commit that disabled all the caches. The users that had prior height hints also in a way will benefit from them as they won't need to start from the initial broadcast (or confirm or w/e height) and can start a bit earlier in the chain.

Copy link
Member

@Roasbeef Roasbeef left a comment

LGTM 🧞‍♂️

Copy link
Collaborator

@cfromknecht cfromknecht left a comment

LGTM

@Roasbeef Roasbeef merged commit c3546c2 into lightningnetwork:master Nov 1, 2018
2 checks passed
@wpaulino wpaulino deleted the enable-height-hint-cache branch Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants