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.

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: persist bad blocks #21827

Merged
merged 9 commits into from
Jan 10, 2021
Merged

core: persist bad blocks #21827

merged 9 commits into from
Jan 10, 2021

Conversation

rjl493456442
Copy link
Member

No description provided.

core/blockchain.go Outdated Show resolved Hide resolved
@holiman
Copy link
Contributor

holiman commented Nov 25, 2020

I'm not sure... The existing semantics (which is now also supported by Besu) when just making a queiry without parameters is basically " give me the N (at most ten) bad blocks that you have seen recently"

I think we should keep that as is. The usecase that this PR adds support for is basically:

  1. to not forget blocks if the node is restarted
  2. to have more than ten blocks

The latter is not really required, IMO, because bad blocks are pretty rare. The former is nice, however..
Two ways to keep current semantics and also fix up the forgetfulness:

  1. Serialize to disk, as you do, and on startup, load the N most recent blocks into the LRU
  2. Do as you did in this PR, but keep debug_getBadBlocks + LRU as is, and add additional (optional) parameter(s) to specify older/more diskloaded blocks.

The nodemonitor will query getbadblocks once every few minutes, I think the LRU is nice to have there

@rjl493456442
Copy link
Member Author

@holiman yup, i will make the change.

@fjl
Copy link
Contributor

fjl commented Dec 15, 2020

I agree with @holiman here, we should limit the number of bad blocks on disk so it can't be used to attack the node. It might even be OK to remove the in-memory LRU.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

core/rawdb/schema.go Outdated Show resolved Hide resolved
Copy link

@DGKSK8LIFE DGKSK8LIFE left a comment

Choose a reason for hiding this comment

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

LGTM.

}

// WriteBadBlock serializes the bad block into the database. If the cumulated
// bad blocks exceeds the limitation, the oldest will be dropped.
Copy link
Member

Choose a reason for hiding this comment

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

Hmm thinking out loud: the first bad block is usually the most interesting as it contains the chain split. Maybe it's not a great idea to yeet it out

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but usually all the following blocks will be skipped. So we meet this specific bad block over and over again.

Header: block.Header(),
Body: block.Body(),
})
sort.Reverse(badBlocks)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, wait, is there anything here which makes the blocks unique? It may well happen that the blockchain tells the db to save the same exact bad block several times, so we shouldn't store a list with ten instances of the same block.

Copy link
Member Author

@rjl493456442 rjl493456442 Jan 10, 2021

Choose a reason for hiding this comment

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

Thanks for pointing it out. I have fixed it(also another sort issue). Please take another look.

TxHash: types.EmptyRootHash,
ReceiptHash: types.EmptyRootHash,
})
WriteBadBlock(db, blockTwo)
Copy link
Contributor

Choose a reason for hiding this comment

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

Test uniqueness too:

Suggested change
WriteBadBlock(db, blockTwo)
WriteBadBlock(db, blockTwo)
WriteBadBlock(db, blockOne)

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

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.

5 participants