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

v2/consortium_test: fix for pbss: do not insert inserted blocks #613

Conversation

Francesco4203
Copy link
Contributor

@Francesco4203 Francesco4203 commented Oct 25, 2024

This PR is an alternative for #612

Before this PR, in TestIsPeriodBlock and TestIsTrippEffective, the chain of blocks is inserted while it still includes some previously inserted blocks, causing the below error when running on path scheme.

Potential reason:

  • There are still logics for correctly handle inserting inserted blocks.
  • However, in pathdb, the blocks older than 128 blocks are not directly retrievable from the layer tree, which are already capped to the disk layer. (there are 128 diff layers) => the parent of the being-inserted block cannot be retrieved for processing.
  • In hashdb, the old blocks can still be retrieved from disk.
  • Therefore, in this 2 tests, the number of blocks inserted are too large to be retrieve, causing the error.

This PR is to fix it by only inserting newly created blocks.

Run test before this PR

--- FAIL: TestIsTrippEffective (0.19s)
panic: triedb parent [0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421] layer missing [recovered]
        panic: triedb parent [0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421] layer missing

goroutine 4016 [running]:
testing.tRunner.func1.2({0x105c97d20, 0x140002d1770})
        /opt/homebrew/Cellar/go/1.22.3/libexec/src/testing/testing.go:1631 +0x1c4
testing.tRunner.func1()
        /opt/homebrew/Cellar/go/1.22.3/libexec/src/testing/testing.go:1634 +0x33c
panic({0x105c97d20?, 0x140002d1770?})
        /opt/homebrew/Cellar/go/1.22.3/libexec/src/runtime/panic.go:770 +0x124
github.com/ethereum/go-ethereum/consensus/consortium/v2.testIsTrippEffective(0x140000c3a00, {0x10592235d, 0x4})
        /Users/long.nguyen/Documents/GitHub/ronin/consensus/consortium/v2/consortium_test.go:2437 +0xe5c
github.com/ethereum/go-ethereum/consensus/consortium/v2.TestIsTrippEffective(0x140000c3a00)
        /Users/long.nguyen/Documents/GitHub/ronin/consensus/consortium/v2/consortium_test.go:2341 +0x40
testing.tRunner(0x140000c3a00, 0x105df9818)
        /opt/homebrew/Cellar/go/1.22.3/libexec/src/testing/testing.go:1689 +0xec
created by testing.(*T).Run in goroutine 1
        /opt/homebrew/Cellar/go/1.22.3/libexec/src/testing/testing.go:1742 +0x318
exit status 2
FAIL    github.com/ethereum/go-ethereum/consensus/consortium/v2 1.631s

Run test after this PR

PASS
ok      github.com/ethereum/go-ethereum/consensus/consortium/v2 1.530s

@Francesco4203 Francesco4203 force-pushed the pbss-fix-isperiodblock-test branch from 46ac8f4 to 6db1c2d Compare November 14, 2024 09:33
@Francesco4203 Francesco4203 marked this pull request as ready for review November 14, 2024 09:42
@@ -2417,7 +2411,7 @@ func testIsTrippEffective(t *testing.T, scheme string) {
block, _ := core.GenerateChain(&chainConfig, bs[len(bs)-1], ethash.NewFaker(), db, 1, callback, true)
bs = append(bs, block...)
}
Copy link
Collaborator

@huyngopt1994 huyngopt1994 Nov 14, 2024

Choose a reason for hiding this comment

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

Could u added some comment explain related to max layers of Path base then we need to insert bs[:]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@huyngopt1994 huyngopt1994 merged commit 4187e39 into axieinfinity:path-base-implementing Nov 14, 2024
1 check passed
huyngopt1994 pushed a commit that referenced this pull request Nov 21, 2024
* v2/consortium_test: only insert newly created blocks

* consortium_test.go: add comments
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.

2 participants