Skip to content

contractcourt: track best block height in chain arbitrator #5004

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

Closed

Conversation

carlaKC
Copy link
Collaborator

@carlaKC carlaKC commented Feb 10, 2021

The GetBestBlock call can be very slow when using bitcoind backend - up to 5 minutes as we found out in #4669.

When we start up arbitrator, we get our best block height every time from this API. This can slow us down on startup, and during the funding flow because we do a fresh lookup every time, the latter affecting applications like pool which require timely responses in the funding flow (thx @C-Otto for the slack report).

This PR makes use of the single block subscription added to the chain arbitrator in #4697 to store out latest known block height, and reduce our reliance on this slow API.

return 0, err
}

c.setBestBlock(height)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could it happen that if ChainIO.GetBestBlock() is taking too long then we'll override a recently set best block with a smaller one here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we'll only need to go to GetBestBlock on startup of chain arb (only time when we'll have a 0 height), I don't think that this will be a problem? But I can add a check to only update if > current height to be safe?

This commit adds tracking of our best block height to the chain
arbitrator, relying on our existing block dispatch goroutine to
update this value. This change decreases the number of times we
need to call GetBestBlock from once per channel, to a single call
on startup. This call is pretty slow in bitcoind (up to 5 minutes
sometimes, screm), and can slow down channel opening, so we move
towards less reliance on it.
@carlaKC carlaKC force-pushed the blockheight-contractcourt branch from 5b89cbe to 39241f9 Compare February 17, 2021 13:37
@cfromknecht
Copy link
Contributor

One comment here: if GetBestBlock is taking a long time, maybe it makes sense to cache this inside the ChainIO object, that way all subsystems could benefit from the speed up. That would prevent us from having to reimplement this caching logic across the codebase

@carlaKC
Copy link
Collaborator Author

carlaKC commented Feb 19, 2021

One comment here: if GetBestBlock is taking a long time, maybe it makes sense to cache this inside the ChainIO object, that way all subsystems could benefit from the speed up.

Definitely! Opened this because there were some issues reported on slack for funding open, but a more general solution sounds like a better idea. Will look into it.

@carlaKC
Copy link
Collaborator Author

carlaKC commented Mar 9, 2021

Closing this out for now. Working on this with @ellemouton in #5084.

@carlaKC carlaKC closed this Mar 9, 2021
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