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

Jump to bottom

Bugfix snapshot transaction segfaults after storage truncation #4786

Merged

garyschulte merged 20 commits into hyperledger:main from garyschulte:bugfix/snapshot-archive-eviction

Dec 22, 2022

Contributor

garyschulte commented Dec 7, 2022 •

edited

Loading

PR description

This PR addresses a race where snap and checkpoint syncs can cause a segfault when starting a worldstate download. The segfault occurs when rocksdb snapshot transactions are read/written after a storage truncation. To prevent this and to ensure future uses of clear() and clearFlatDatabase() cannot cause segfaults, this PR:

adds BonsaiStorageSubscriber type to handle storage events
removes addCachedLayer from TrieLogManager, moves to AbstractTrieLogManager
TrieLogManager takes a snapshot immediately in addCachedLayer rather than deferring it
notifies subscribers on events that can affect the worldstate, specifically:
- clear (truncate)
- clearFlatDatabase (truncating a subset of storage)
- close (closing the worldstate storage)

Additionally, only drop cached layers by distance from head when the worldstate archive has more than the configured number of retained layers.

Fixed Issue(s)

addresses #4765

Documentation

I thought about documentation and added the doc-change-required label to this PR if
updates are required.

Changelog

I thought about the changelog and included a changelog update if required.

garyschulte force-pushed the bugfix/snapshot-archive-eviction branch from 50ee949 to cf85130 Compare

December 7, 2022 21:26

garyschulte changed the title ~~Bugfix startup worldstate race with checkpoint sync~~ Bugfix startup race with checkpoint sync

garyschulte changed the title ~~Bugfix startup race with checkpoint sync~~ Bugfix startup race with checkpoint sync and bonsai snapshots

garyschulte force-pushed the bugfix/snapshot-archive-eviction branch from 9fd132a to 8e8a72f Compare

December 8, 2022 00:41

garyschulte marked this pull request as ready for review

December 8, 2022 00:41

garyschulte marked this pull request as draft

December 8, 2022 02:05

garyschulte force-pushed the bugfix/snapshot-archive-eviction branch 2 times, most recently from 61169d0 to 8e8a72f Compare

December 8, 2022 17:10

garyschulte marked this pull request as ready for review

December 9, 2022 00:46

garyschulte requested a review from matkt

December 9, 2022 00:47

garyschulte mentioned this pull request

Besu crash on mainnet #4765

Closed

garyschulte force-pushed the bugfix/snapshot-archive-eviction branch from 376705b to 8558206 Compare

December 9, 2022 06:41

garyschulte requested a review from ahamlat

December 9, 2022 06:43

garyschulte changed the title ~~Bugfix startup race with checkpoint sync and bonsai snapshots~~ Bugfix snapshot transaction segfaults after storage truncation

garyschulte force-pushed the bugfix/snapshot-archive-eviction branch from d2e0f82 to f6cd5f8 Compare

December 9, 2022 07:58

Contributor

matkt commented Dec 9, 2022

we must check the impact on performance at the time of persistence because we are creating the snapshot at this time

matkt previously requested changes

View reviewed changes

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/BonsaiSnapshotWorldState.java Outdated Show resolved Hide resolved

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/SnapshotTrieLogManager.java Outdated Show resolved Hide resolved

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/BonsaiSnapshotWorldState.java Outdated Show resolved Hide resolved

...core/src/main/java/org/hyperledger/besu/ethereum/bonsai/BonsaiWorldStateKeyValueStorage.java Outdated Show resolved Hide resolved

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/SnapshotTrieLogManager.java Outdated Show resolved Hide resolved

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/SnapshotTrieLogManager.java Show resolved Hide resolved

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/SnapshotTrieLogManager.java Outdated Show resolved Hide resolved

Contributor

ahamlat commented Dec 12, 2022

we must check the impact on performance at the time of persistence because we are creating the snapshot at this time

I didn't see any performance impact on block processing time, after 3 hours of execution

garyschulte force-pushed the bugfix/snapshot-archive-eviction branch from f6cd5f8 to 5a2d7d1 Compare

December 13, 2022 00:17

garyschulte commented

View reviewed changes

...creation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockCreator.java Show resolved Hide resolved

ahamlat approved these changes

View reviewed changes

Contributor

ahamlat left a comment

Approve performance overhead. This PR does not have an effect on block processing performance

garyschulte force-pushed the bugfix/snapshot-archive-eviction branch 3 times, most recently from 4564c26 to 5d3a53a Compare

December 15, 2022 06:50

jframe force-pushed the main branch from bd467a1 to 2b9956a Compare

December 16, 2022 01:43

garyschulte force-pushed the bugfix/snapshot-archive-eviction branch 5 times, most recently from 9f48fba to 8890a82 Compare

December 21, 2022 19:28

garyschulte added 17 commits

December 22, 2022 12:09


          use worldStateKeyValueStorage from AbstractTrieLogManager to create s…

5c0a547

…napshot

remove MutableWorldState parameter from saveTrieLog
make addCachedLayer protected in AbstractTrieLogManager, remove from TrieLogManager interface

Signed-off-by: garyschulte <garyschulte@gmail.com>


          drop all SnapshotTrieLogManager cached worldstates on clear of Bonsai…

1d8ba53

… KeyValue storage

Signed-off-by: garyschulte <garyschulte@gmail.com>


          removed unnecessary/unimplemented onClear events from snapshot cached…

f971b3e

… trie logs

Signed-off-by: garyschulte <garyschulte@gmail.com>


          removed debugging log message

f121402

Signed-off-by: garyschulte <garyschulte@gmail.com>


          minor spotless

6dc7828

Signed-off-by: garyschulte <garyschulte@gmail.com>


          subscribe snapshot worldstates to parent worldstate storage events li…

ac252e3

…ke clear and clearFlatDatabase and close as appropriate

protect RocksDBSnapshotTransactions to prevent reading or writing to a closed transaction

Signed-off-by: garyschulte <garyschulte@gmail.com>


          reorder BonsaiPersistedWorldState.persist() to save the trie log AFTE…

a43cbd9

…R the worldstate is committed.

Signed-off-by: garyschulte <garyschulte@gmail.com>


          spotless

9f77a5b

Signed-off-by: garyschulte <garyschulte@gmail.com>


          use newWorldStateRootHash for trieLog in BonsaiPersistedWorldState.pe…

f685296

…rsist()

Signed-off-by: garyschulte <garyschulte@gmail.com>


          spotless and test mocks

a15f11f

Signed-off-by: garyschulte <garyschulte@gmail.com>


          address PR feedback

7e7b021

Signed-off-by: garyschulte <garyschulte@gmail.com>


          remove unnecessary close() of non-persistable worldstates in Abstract…

b62559b

…BlockCreator

Signed-off-by: garyschulte <garyschulte@gmail.com>


          spotless

ce17181

Signed-off-by: garyschulte <garyschulte@gmail.com>


          fix for direct snapshot creation when using snapshot archive

f7a482e

   adds subscriptions to BonsaiPersistedWorldState
   use worldstate accessor in AbstractTrieLogManager rather than member persisted state
   cached snapshots subscribe to their respective worldstates
   prevent closing of snapshots unless subscriber count is 0

Signed-off-by: garyschulte <garyschulte@gmail.com>


           moved snapshot subscriptions into the snapshot keyvalue storage rath…

91ffa8e

…er than in the snapshot itself, so we can more easily handle truncate events

Signed-off-by: garyschulte <garyschulte@gmail.com>


          layered worldstate test coverage and minor refactor to support test a…

…ssertions

Signed-off-by: garyschulte <garyschulte@gmail.com>


          remove unnecessary mocks

f781b32

Signed-off-by: garyschulte <garyschulte@gmail.com>

garyschulte force-pushed the bugfix/snapshot-archive-eviction branch from 8890a82 to f781b32 Compare

December 22, 2022 20:10

garyschulte dismissed matkt’s stale review

December 22, 2022 20:10

resolved issues

garyschulte enabled auto-merge (squash)

December 22, 2022 20:14

garyschulte merged commit 2b17e04 into hyperledger:main

garyschulte deleted the bugfix/snapshot-archive-eviction branch

December 22, 2022 22:22

macfarla pushed a commit to macfarla/besu that referenced this pull request


          Bugfix snapshot transaction segfaults after storage truncation (hyper…

3b41d5c

…ledger#4786)

* subscribe snapshot worldstates to parent worldstate storage events like clear and clearFlatDatabase and close as appropriate to avoid segfaults
* fix for direct snapshot creation when using snapshot archive
* ensure we only prune bonsai worldstates if we have more than the configured number of retained states

Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

non-fungible-nelson mentioned this pull request

Unable to copy layered worldstate on 22.10.2 - Snapshots=False #4784

Closed

garyschulte added a commit to garyschulte/besu that referenced this pull request


          Bugfix snapshot transaction segfaults after storage truncation (hyper…

418d392

…ledger#4786)

* subscribe snapshot worldstates to parent worldstate storage events like clear and clearFlatDatabase and close as appropriate to avoid segfaults
* fix for direct snapshot creation when using snapshot archive
* ensure we only prune bonsai worldstates if we have more than the configured number of retained states

Signed-off-by: garyschulte <garyschulte@gmail.com>

garyschulte added a commit to garyschulte/besu that referenced this pull request


          Bugfix snapshot transaction segfaults after storage truncation (hyper…

2a89edd

…ledger#4786)

* subscribe snapshot worldstates to parent worldstate storage events like clear and clearFlatDatabase and close as appropriate to avoid segfaults
* fix for direct snapshot creation when using snapshot archive
* ensure we only prune bonsai worldstates if we have more than the configured number of retained states

Signed-off-by: garyschulte <garyschulte@gmail.com>

garyschulte mentioned this pull request

Cherry pick burn-in release 22.10.4 #4927

Merged

2 tasks

garyschulte added a commit to garyschulte/besu that referenced this pull request


          Bugfix snapshot transaction segfaults after storage truncation (hyper…

78c1325

…ledger#4786)

* subscribe snapshot worldstates to parent worldstate storage events like clear and clearFlatDatabase and close as appropriate to avoid segfaults
* fix for direct snapshot creation when using snapshot archive
* ensure we only prune bonsai worldstates if we have more than the configured number of retained states

Signed-off-by: garyschulte <garyschulte@gmail.com>

garyschulte added a commit that referenced this pull request


          Cherry pick burn-in release 22.10.4 (#4927)

f997066

* Use safe block as pivot block suring snapsync (#4819)
* Bugfix snapshot transaction segfaults after storage truncation (#4786)
* Bugfix potential chain head and worldstate inconsistency (#4862)
* Peering - disconnect worst peer (#4888)
* Attempt to fix CPU spikes issue (#4867)
* not block subscribe when the worldstate storage is open (#4912)

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>

eum602 pushed a commit to lacchain/besu that referenced this pull request


          Bugfix snapshot transaction segfaults after storage truncation (hyper…

411815d

…ledger#4786)

* subscribe snapshot worldstates to parent worldstate storage events like clear and clearFlatDatabase and close as appropriate to avoid segfaults
* fix for direct snapshot creation when using snapshot archive
* ensure we only prune bonsai worldstates if we have more than the configured number of retained states

Signed-off-by: garyschulte <garyschulte@gmail.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet