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.

Sign up for GitHub

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

core: add txlookup lock #29343

Merged
merged 1 commit into from
Apr 9, 2024
Merged

core: add txlookup lock #29343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions core/blockchain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,8 @@ type BlockChain struct {
bodyRLPCache *lru.Cache[common.Hash, rlp.RawValue]
receiptsCache *lru.Cache[common.Hash, []*types.Receipt]
blockCache *lru.Cache[common.Hash, *types.Block]

txLookupLock sync.RWMutex
txLookupCache *lru.Cache[common.Hash, txLookup]

wg sync.WaitGroup
Expand Down Expand Up @@ -2298,14 +2300,14 @@ func (bc *BlockChain) reorg(oldHead *types.Header, newHead *types.Block) error {
// rewind the canonical chain to a lower point.
log.Error("Impossible reorg, please file an issue", "oldnum", oldBlock.Number(), "oldhash", oldBlock.Hash(), "oldblocks", len(oldChain), "newnum", newBlock.Number(), "newhash", newBlock.Hash(), "newblocks", len(newChain))
}
// Reset the tx lookup cache in case to clear stale txlookups.
// This is done before writing any new chain data to avoid the
// weird scenario that canonical chain is changed while the
// stale lookups are still cached.
bc.txLookupCache.Purge()
// Acquire the tx-lookup lock before mutation. This step is essential
// as the txlookups should be changed atomically, and all subsequent
// reads should be blocked until the mutation is complete.
bc.txLookupLock.Lock()

// Insert the new chain(except the head block(reverse order)),
// taking care of the proper incremental order.
// Insert the new chain segment in incremental order, from the old
// to the new. The new chain head (newChain[0]) is not inserted here,
// as it will be handled separately outside of this function
for i := len(newChain) - 1; i >= 1; i-- {
// Insert the block in the canonical way, re-writing history
bc.writeHeadBlock(newChain[i])
Expand Down Expand Up @@ -2342,6 +2344,11 @@ func (bc *BlockChain) reorg(oldHead *types.Header, newHead *types.Block) error {
if err := indexesBatch.Write(); err != nil {
log.Crit("Failed to delete useless indexes", "err", err)
}
// Reset the tx lookup cache to clear stale txlookup cache.
bc.txLookupCache.Purge()

// Release the tx-lookup lock after mutation.
bc.txLookupLock.Unlock()

// Send out events for logs from the old canon chain, and 'reborn'
// logs from the new canon chain. The number of logs can be very
Expand Down
3 changes: 3 additions & 0 deletions core/blockchain_reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,9 @@ func (bc *BlockChain) GetAncestor(hash common.Hash, number, ancestor uint64, max
// transaction indexing is already finished. The transaction is not existent
// from the node's perspective.
func (bc *BlockChain) GetTransactionLookup(hash common.Hash) (*rawdb.LegacyTxLookupEntry, *types.Transaction, error) {
bc.txLookupLock.RLock()
defer bc.txLookupLock.RUnlock()
Comment on lines +269 to +270
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit hesitant here. Basically, if we ever run into one of those "very large reorg" scenarios, or something is really slow because "reasons", this will add to the pile-up of things hanging.

Would it make sense to have some sort of TryLock mechanism here, so that if we don't obtain the lock, then fine, we'll just not use the cache right now?

I'm not sure, maybe this is fine


// Short circuit if the txlookup already in the cache, retrieve otherwise
if item, exist := bc.txLookupCache.Get(hash); exist {
return item.lookup, item.transaction, nil
Expand Down
Loading