gomod: fix btcd mempool leak fix and GetBlock heap escape#8401
Merged
Conversation
Contributor
|
Important Auto Review SkippedAuto reviews are limited to the following labels: llm-review. Please add one of these labels to enable auto reviews. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe updates in the release notes reflect a critical fix for a memory leak issue when running Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Member
Author
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between ba4021c and 3f27750691a81b02abc8968778f7087c8c26da85.Files ignored due to path filters (2)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sum
Files selected for processing (2)
- docs/release-notes/release-notes-0.17.3.md (1 hunks)
- docs/release-notes/release-notes-0.17.4.md (1 hunks)
Files skipped from review due to trivial changes (1)
- docs/release-notes/release-notes-0.17.3.md
Additional comments: 1
docs/release-notes/release-notes-0.17.4.md (1)
- 37-38: Ensure that Yong Yu's contributions are accurately reflected in the release notes.
yyforyongyu
force-pushed
the
update-btcd-ver
branch
from
3f27750 to
e53ef6a
Compare
yyforyongyu
changed the title
btcd version to include the mempool leak fixbtcd mempool leak fix and GetBlock heap escape
yyforyongyu
force-pushed
the
update-btcd-ver
branch
2 times, most recently
from
3c258be to
54bdd4b
Compare
ellemouton
reviewed
Jan 22, 2024
Collaborator
There was a problem hiding this comment.
a couple of questions (just for my understanding):
- are you saying that the pointer to the
getBlockImplgets stored on the heap? if so - im not sure i understand why that would be since we use it once here to produce a result & ie, we are not returning a pointer to the function right? - for making a copy of the block - I guess to me it only makes sense to make a copy of something before returning if the original object has references to some other info that is not needed once we return & so we want to get rid of those references. But here - are we removing any references? from the commit body it seems like you are saying that the block would have a reference to
getBlockImplbut im not sure if that is true? - also - we would want the actual block to be on the heap right?
- finally: did you notice this escape by eye or is there a tool you used to be able to identify this? asking cause my eye is definitely not familiar with this sort of gotcha
Member
Author
There was a problem hiding this comment.
To answer,
- nope not the
getBlockImplbut the data it uses, hence the block. - def, we can copy it at difference places - either in
btcd/rpcclient/chain.go'sGetBlock, or here. As that being said, I see a lot more issues with heap escaping inbtcdlib, kinda don't wanna touch it atm. - with the current design the block has to be on the heap because we store a pointer reference to that block in our cache. I have another local PR which removes the pointer references - don't think it helps tho, still digging as as you've said above, the block has to be cached.
- this is a pure result from looking at the profile above, after fixing
SendCmd, I was curious about why there was a heap allocation onGetBlock, then found thisblockcache. Then got even more curious, as we only allow 20MB of cache size, but it used more. Then I took a closer look at the stacks,
(pprof) list GetBlock
Total: 115.90MB
ROUTINE ======================== github.com/btcsuite/btcd/rpcclient.(*Client).GetBlock in /Users/yong/Projects/lightning_labs/btcsuite/btcd/rpcclient/chain.go
0 39.14MB (flat, cum) 33.77% of Total
. . 159:func (c *Client) GetBlock(blockHash *chainhash.Hash) (*wire.MsgBlock, error) {
. 39.14MB 160: return c.GetBlockAsync(blockHash).Receive()
. . 161:}
. . 162:
. . 163:// FutureGetBlockVerboseResult is a future promise to deliver the result of a
. . 164:// GetBlockVerboseAsync RPC invocation (or an applicable error).
. . 165:type FutureGetBlockVerboseResult struct {
ROUTINE ======================== github.com/btcsuite/btcd/rpcclient.FutureGetBlockResult.Receive in /Users/yong/Projects/lightning_labs/btcsuite/btcd/rpcclient/chain.go
0 39.14MB (flat, cum) 33.77% of Total
. . 108:func (r FutureGetBlockResult) Receive() (*wire.MsgBlock, error) {
. . 109: res, err := r.client.waitForGetBlockRes(r.Response, r.hash, false, false)
. . 110: if err != nil {
. . 111: return nil, err
. . 112: }
. . 113:
. . 114: // Unmarshal result as a string.
. . 115: var blockHex string
. . 116: err = json.Unmarshal(res, &blockHex)
. . 117: if err != nil {
. . 118: return nil, err
. . 119: }
. . 120:
. . 121: // Decode the serialized block hex to raw bytes.
. . 122: serializedBlock, err := hex.DecodeString(blockHex)
. . 123: if err != nil {
. . 124: return nil, err
. . 125: }
. . 126:
. . 127: // Deserialize the block and return it.
. . 128: var msgBlock wire.MsgBlock
. 39.14MB 129: err = msgBlock.Deserialize(bytes.NewReader(serializedBlock))
. . 130: if err != nil {
. . 131: return nil, err
. . 132: }
. . 133: return &msgBlock, nil
. . 134:}
ROUTINE ======================== github.com/btcsuite/btcwallet/chain.(*BitcoindClient).GetBlock in /Users/yong/Projects/lightning_labs/btcsuite/btcwallet/chain/bitcoind_client.go
0 39.14MB (flat, cum) 33.77% of Total
. . 143:func (c *BitcoindClient) GetBlock(hash *chainhash.Hash) (*wire.MsgBlock, error) {
. 39.14MB 144: return c.chainConn.GetBlock(hash)
. . 145:}
. . 146:
. . 147:// GetBlockVerbose returns a verbose block from the hash.
. . 148:func (c *BitcoindClient) GetBlockVerbose(
. . 149: hash *chainhash.Hash) (*btcjson.GetBlockVerboseResult, error) {
ROUTINE ======================== github.com/btcsuite/btcwallet/chain.(*BitcoindConn).GetBlock in /Users/yong/Projects/lightning_labs/btcsuite/btcwallet/chain/bitcoind_conn.go
0 39.14MB (flat, cum) 33.77% of Total
. . 458:func (c *BitcoindConn) GetBlock(hash *chainhash.Hash) (*wire.MsgBlock, error) {
. 39.14MB 459: block, err := c.client.GetBlock(hash)
. . 460: // Got the block from the backend successfully, return it.
. . 461: if err == nil {
. . 462: return block, nil
. . 463: }
. . 464:
ROUTINE ======================== github.com/lightningnetwork/lnd/blockcache.(*BlockCache).GetBlock in /Users/yong/Projects/lightning_labs/lnd/blockcache/blockcache.go
0 39.14MB (flat, cum) 33.77% of Total
. . 34:func (bc *BlockCache) GetBlock(hash *chainhash.Hash,
. . 35: getBlockImpl func(hash *chainhash.Hash) (*wire.MsgBlock,
. . 36: error)) (*wire.MsgBlock, error) {
. . 37:
. . 38: bc.HashMutex.Lock(lntypes.Hash(*hash))
. . 39: defer bc.HashMutex.Unlock(lntypes.Hash(*hash))
. . 40:
. . 41: // Create an inv vector for getting the block.
. . 42: inv := wire.NewInvVect(wire.InvTypeWitnessBlock, hash)
. . 43:
. . 44: // Check if the block corresponding to the given hash is already
. . 45: // stored in the blockCache and return it if it is.
. . 46: cacheBlock, err := bc.Cache.Get(*inv)
. . 47: if err != nil && err != cache.ErrElementNotFound {
. . 48: return nil, err
. . 49: }
. . 50: if cacheBlock != nil {
. . 51: return cacheBlock.MsgBlock(), nil
. . 52: }
. . 53:
. . 54: // Fetch the block from the chain backends.
. 39.14MB 55: block, err := getBlockImpl(hash)
. . 56: if err != nil {
. . 57: return nil, err
. . 58: }
. . 59:
. . 60: // Add the new block to blockCache. If the Cache is at its maximum
ROUTINE ======================== github.com/lightningnetwork/lnd/chainntnfs/bitcoindnotify.(*BitcoindNotifier).GetBlock in /Users/yong/Projects/lightning_labs/lnd/chainntnfs/bitcoindnotify/bitcoind.go
0 23.10MB (flat, cum) 19.93% of Total
. . 1042:func (b *BitcoindNotifier) GetBlock(hash *chainhash.Hash) (*wire.MsgBlock,
. . 1043: error) {
. . 1044:
. 23.10MB 1045: return b.blockCache.GetBlock(hash, b.chainConn.GetBlock)
. . 1046:}
. . 1047:
. . 1048:// SubscribeMempoolSpent allows the caller to register a subscription to watch
. . 1049:// for a spend of an outpoint in the mempool.The event will be dispatched once
. . 1050:// the outpoint is spent in the mempool.
ROUTINE ======================== github.com/lightningnetwork/lnd/lnwallet/btcwallet.(*BtcWallet).GetBlock in /Users/yong/Projects/lightning_labs/lnd/lnwallet/btcwallet/blockchain.go
0 16.04MB (flat, cum) 13.84% of Total
. . 136:func (b *BtcWallet) GetBlock(blockHash *chainhash.Hash) (*wire.MsgBlock, error) {
. . 137: _, ok := b.chain.(*chain.NeutrinoClient)
. . 138: if !ok {
. 16.04MB 139: return b.blockCache.GetBlock(blockHash, b.chain.GetBlock)
. . 140: }
. . 141:
. . 142: // For the neutrino implementation of lnwallet.BlockChainIO the neutrino
. . 143: // GetBlock function can be called directly since it uses the same block
. . 144: // cache. However, it does not lock the block cache mutex for the given
And also with the command go build -gcflags='-m=3' . |& grep escapes to figure out which line escapes. That being said, I don't think we need to concern about this when writing code, optimization should always come later, as it can be an endless, but it's very difficult for me to resist the fun🤓
That being said, more analysis shows the Copy doesn't actually save us more memory in this case, it does give a better pattern to recognize the memory allocation, as now it's,
(pprof) list GetBlock
Total: 110.41MB
ROUTINE ======================== github.com/lightningnetwork/lnd/blockcache.(*BlockCache).GetBlock in /Users/yong/Projects/lightning_labs/lnd/blockcache/blockcache.go
0 33.68MB (flat, cum) 30.51% of Total
. . 34:func (bc *BlockCache) GetBlock(hash *chainhash.Hash,
. . 35: getBlockImpl func(hash *chainhash.Hash) (*wire.MsgBlock,
. . 36: error)) (*wire.MsgBlock, error) {
. . 37:
. . 38: bc.HashMutex.Lock(lntypes.Hash(*hash))
. . 39: defer bc.HashMutex.Unlock(lntypes.Hash(*hash))
. . 40:
. . 41: // Create an inv vector for getting the block.
. . 42: inv := wire.NewInvVect(wire.InvTypeWitnessBlock, hash)
. . 43:
. . 44: // Check if the block corresponding to the given hash is already
. . 45: // stored in the blockCache and return it if it is.
. . 46: cacheBlock, err := bc.Cache.Get(*inv)
. . 47: if err != nil && err != cache.ErrElementNotFound {
. . 48: return nil, err
. . 49: }
. . 50: if cacheBlock != nil {
. . 51: return cacheBlock.MsgBlock(), nil
. . 52: }
. . 53:
. . 54: // Fetch the block from the chain backends.
. . 55: msgBlock, err := getBlockImpl(hash)
. . 56: if err != nil {
. . 57: return nil, err
. . 58: }
. . 59:
. . 60: // Make a copy of the block so it won't escape to the heap.
. 33.68MB 61: msgBlockCopy := msgBlock.Copy()
. . 62: block := btcutil.NewBlock(msgBlockCopy)
. . 63:
. . 64: // Add the new block to blockCache. If the Cache is at its maximum
. . 65: // capacity then the LFU item will be evicted in favour of this new
. . 66: // block.
ROUTINE ======================== github.com/lightningnetwork/lnd/chainntnfs/bitcoindnotify.(*BitcoindNotifier).GetBlock in /Users/yong/Projects/lightning_labs/lnd/chainntnfs/bitcoindnotify/bitcoind.go
0 33.68MB (flat, cum) 30.51% of Total
. . 1042:func (b *BitcoindNotifier) GetBlock(hash *chainhash.Hash) (*wire.MsgBlock,
. . 1043: error) {
. . 1044:
. 33.68MB 1045: return b.blockCache.GetBlock(hash, b.chainConn.GetBlock)
. . 1046:}
. . 1047:
. . 1048:// SubscribeMempoolSpent allows the caller to register a subscription to watch
. . 1049:// for a spend of an outpoint in the mempool.The event will be dispatched once
. . 1050:// the outpoint is spent in the mempool.
Roasbeef
approved these changes
Jan 23, 2024
Member
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
LGTM ⛷️
Just needs a rebase now for the release notes.
This commit fixes a heap escape found in `GetBlock`. This happens because the `msgBlock` is a pointer returned from `getBlockImpl`, and the whole `getBlockImpl` escapes to the heap because it's referenced in two places, - in the `Cache`, it's used as a reference type to create the new block. - this pointer is also returned and now needs to stay on the heap. The fix is simple, we now make a copy of the block and use the copy instead, freeing `getBlockImpl` from the heap.
yyforyongyu
force-pushed
the
update-btcd-ver
branch
from
54bdd4b to
ff1f33e
Compare
Member
Author
|
Since the latest |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This PR updates the
btcdversion to,blockcache.GetBlockWe can see the effects from the following profiles,
SendCmdis gone.GetBlockheap escape fix, notice theBtcDecodeis gone.Fix #8356
Summary by CodeRabbit