keychain+lnwallet: when fetching priv keys or signing try to use cache #5629
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Roasbeef
changed the title
guggero
approved these changes
Aug 16, 2021
keychain/btcwallet.go
Outdated
| // use to cache private keys to avoid DB and EC operations within the | ||
| // wallet. With the default sisize, we'll allocate up to 320 KB to | ||
| // caching private keys (ignoring pointer overhead, etc). | ||
| privKeyCacheSize = 10_000 |
| cleanUp, wallet, err := createTestBtcWallet( | ||
| CoinTypeBitcoin, | ||
| ) | ||
| if err != nil { |
| for i := 0; i < t.N; i++ { | ||
| privKey, err = keyRing.DerivePrivKey(keyDesc) | ||
| } | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Move those inside the loop?
carlaKC
approved these changes
Aug 18, 2021
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func BenchmarkDerivePrivKey(t *testing.B) { |
keychain/btcwallet.go
Outdated
|
|
||
| // privKeyCacheSize is the default size of the the LRU cache that we'll | ||
| // use to cache private keys to avoid DB and EC operations within the | ||
| // wallet. With the default sisize, we'll allocate up to 320 KB to |
| Branch: 0, | ||
| Index: keyDesc.Index, | ||
| } | ||
| privKey, err := scope.DeriveFromKeyPathCache(keyPath) |
There was a problem hiding this comment.
If err can be anything other that ErrAccountNotCached, do we want to log/handle it? Likewise below.
Roasbeef
force-pushed
the
priv-key-caching
branch
2 times, most recently
from
eb7b515
to
6d1da54
Compare
In this commit, we start to optimistically use the new private key cache that was added to btcwallet. As is, btcwallet will cache the decrypted account keys for each scope in memory. However, the existing methods to derive a child key from those account keys requires a write database transaction, and will re-derive the private key using BIP-32 each time. The newly added `DeriveFromKeyPathCache` lets us skip all this and directly use a cache assuming the account info is already cached. The new logic will try to use this method, but if it fails fall back to the existing `DeriveFromKeyPath` method. All calls after this will use this new cached key. Fixes lightningnetwork#5125. Basic benchmark before the btcwallet change and after: ``` benchmark old ns/op new ns/op delta BenchmarkDerivePrivKey-8 22840583 125 -100.00% benchmark old allocs new allocs delta BenchmarkDerivePrivKey-8 89 2 -97.75% benchmark old bytes new bytes delta BenchmarkDerivePrivKey-8 10225 24 -99.77% ```
Roasbeef
force-pushed
the
priv-key-caching
branch
from
6d1da54
to
abb2448
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In this commit, we start to optimistically use the new private key cache
that was added to btcwallet. As is, btcwallet will cache the decrypted
account keys for each scope in memory. However, the existing methods
to derive a child key from those account keys requires a write database
transaction, and will re-derive the private key using BIP-32 each time.
The newly added
DeriveFromKeyPathCachelets us skip all this anddirectly use a cache assuming the account info is already cached. The
new logic will try to use this method, but if it fails fall back to the
existing
DeriveFromKeyPathmethod. All calls after this will use thisnew cached key.
Fixes #5125.
Basic benchmark before the btcwallet change and after: