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

light: fix txpool causes panic on reorgOnNewHead #54

Merged
merged 6 commits into from
Mar 13, 2020
Merged

light: fix txpool causes panic on reorgOnNewHead #54

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f6ed5d4
light: fix txpool causes panic on reorgOnNewHead
meowsbits Mar 4, 2020
7e98c38
light: revert f6ed5d4bbc55afd2918b7847231f8a3f126d1fbd
meowsbits Mar 12, 2020
e651942
core: add headerchain checks for nil
meowsbits Mar 12, 2020
99059d3
light: add txpool edge case tests and fix reorg method can panic
meowsbits Mar 12, 2020
c3de2c9
light: handle case if chain is at or near geneesis
meowsbits Mar 12, 2020
687e626
light: remove typo dupe line, use Fatal instead of panic
meowsbits Mar 12, 2020
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
48 changes: 28 additions & 20 deletions light/txpool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,30 +224,38 @@ func (pool *TxPool) reorgOnNewHead(ctx context.Context, newHeader *types.Header)
txc := make(txStateChanges)
oldh := pool.chain.GetHeaderByHash(pool.head)
newh := newHeader
// find common ancestor, create list of rolled back and new block hashes
var oldHashes, newHashes []common.Hash
for oldh.Hash() != newh.Hash() {
if oldh.Number.Uint64() >= newh.Number.Uint64() {
oldHashes = append(oldHashes, oldh.Hash())
oldh = pool.chain.GetHeader(oldh.ParentHash, oldh.Number.Uint64()-1)
}
if oldh.Number.Uint64() < newh.Number.Uint64() {
newHashes = append(newHashes, newh.Hash())
newh = pool.chain.GetHeader(newh.ParentHash, newh.Number.Uint64()-1)
if newh == nil {
// happens when CHT syncing, nothing to do
newh = oldh

if oldh != nil {
// find common ancestor, create list of rolled back and new block hashes
for oldh.Hash() != newh.Hash() {
if oldh.Number.Uint64() >= newh.Number.Uint64() {
oldHashes = append(oldHashes, oldh.Hash())
oldh = pool.chain.GetHeader(oldh.ParentHash, oldh.Number.Uint64()-1)
}
if oldh.Number.Uint64() < newh.Number.Uint64() {
newHashes = append(newHashes, newh.Hash())
newh = pool.chain.GetHeader(newh.ParentHash, newh.Number.Uint64()-1)
if newh == nil {
// happens when CHT syncing, nothing to do
newh = oldh
}
}
}
if oldh.Number.Uint64() < pool.clearIdx {
pool.clearIdx = oldh.Number.Uint64()
}
// roll back old blocks
for _, hash := range oldHashes {
pool.rollbackTxs(hash, txc)
}
pool.head = oldh.Hash()
}
if oldh.Number.Uint64() < pool.clearIdx {
pool.clearIdx = oldh.Number.Uint64()
}
// roll back old blocks
for _, hash := range oldHashes {
pool.rollbackTxs(hash, txc)

if newHeader == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This statement doesn't make much sense, especially in here.
Above, we assign newHeader to newh, and operate on newh. If newHeader was indeed nil, this would fail (except degenerated case when both newh/newHeader and oldHeader are nil).

If it actually can happen, or we want to be extra defensive, move the check above line 229.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was assuming oldh was nil...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PTAL at changes thru 687e626.

Have added some test cases and simplified the checks.

return txc, nil
}
pool.head = oldh.Hash()

// check mined txs of new blocks (array is in reversed order)
for i := len(newHashes) - 1; i >= 0; i-- {
hash := newHashes[i]
Expand Down Expand Up @@ -365,7 +373,7 @@ func (pool *TxPool) validateTx(ctx context.Context, tx *types.Transaction) error
// Check the transaction doesn't exceed the current
// block limit gas.
header := pool.chain.GetHeaderByHash(pool.head)
if header.GasLimit < tx.Gas() {
if header != nil && header.GasLimit < tx.Gas() {
return core.ErrGasLimit
}

Expand Down