Skip to content

Conversation

@rjl493456442
Copy link
Member

No description provided.

@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

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

gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Sep 29, 2025
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Sep 29, 2025
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Sep 30, 2025
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Oct 8, 2025
gzliudan added a commit to XinFinOrg/XDPoSChain that referenced this pull request Oct 8, 2025
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Nov 13, 2025
AnilChinchawale pushed a commit to XinFinOrg/XDPoSChain that referenced this pull request Nov 15, 2025
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