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

candidate fix for missing parent worldstate #4094

Merged
merged 12 commits into from
Jul 18, 2022

Conversation

matkt
Copy link
Contributor

@matkt matkt commented Jul 13, 2022

Signed-off-by: Karim TAAM karim.t2am@gmail.com

PR description

when we remember block after the merge we must persist the trielog. because if we lose the trielog in the memory (ex restart besu) we can end up in a case where the block is saved but not the trielog. this can prevent the rollback or the rollforward to work because besu will consider that it can do it as the block is present but it will failed because trielog is missing.

may be one of the reasons for the problem found on a ropsten test node

A possible edgecase description :

  • the head is block 5
  • we remember block 6
  • as we remember block 6 , we save trielog 6 only in memory
  • CL is saying to go to block 7
    now we have
  • block5 in database + trielog 5 in database
  • block6 in database + trielog 6 in memory
  • block7 in database + trielog 7 in database
  • we have a reorg to block 6
  • we rollback 7 to 6 and we persist again block 6. as trielog 6 is not in the database we save invalid trielog 6 (7->6)

if we restart before the reorg trielog 6 is missing

this PR also fix an issue related to an invalid implementation of BonsaiInmemoryWorldstate

java.lang.AssertionError: Node hash 0x9289cda321472949c843ca0b1c6e08011c2608436898699468f49b72d2d8533e not equal to expected 0x03ac2557b39e96bc541432719417ca05360530f915ae8e53d8bf2591bee9c883
	at org.hyperledger.besu.ethereum.trie.StoredNodeFactory.lambda$retrieve$1(StoredNodeFactory.java:103)
	at java.base/java.util.Optional.map(Optional.java:265)
	at org.hyperledger.besu.ethereum.trie.StoredNodeFactory.retrieve(StoredNodeFactory.java:97)
	at org.hyperledger.besu.ethereum.trie.StoredNode.load(StoredNode.java:130)
	at org.hyperledger.besu.ethereum.trie.StoredNode.accept(StoredNode.java:63)
	at org.hyperledger.besu.ethereum.trie.StoredMerklePatriciaTrie.put(StoredMerklePatriciaTrie.java:142)
	at org.hyperledger.besu.ethereum.bonsai.BonsaiPersistedWorldState.calculateRootHash(BonsaiPersistedWorldState.java:227)
	at org.hyperledger.besu.ethereum.bonsai.BonsaiPersistedWorldState.calculateRootHash(BonsaiPersistedWorldState.java:104)
	at org.hyperledger.besu.ethereum.bonsai.BonsaiInMemoryWorldState.rootHash(BonsaiInMemoryWorldState.java:34)
	at org.hyperledger.besu.ethereum.MainnetBlockValidator.validateAndProcessBlock(MainnetBlockValidator.java:119)
	at org.hyperledger.besu.consensus.merge.blockcreation.MergeCoordinator.validateBlock(MergeCoordinator.java:256)
	at org.hyperledger.besu.consensus.merge.blockcreation.MergeCoordinator.rememberBlock(MergeCoordinator.java:272)
	at org.hyperledger.besu.consensus.merge.blockcreation.TransitionCoordinator.rememberBlock(TransitionCoordinator.java:137)
	at org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.engine.EngineNewPayload.syncResponse(EngineNewPayload.java:189)
	at org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod.lambda$response$0(ExecutionEngineJsonRpcMethod.java:62)
	at io.vertx.core.impl.ContextImpl.lambda$null$0(ContextImpl.java:159)
	at io.vertx.core.impl.AbstractContext.dispatch(AbstractContext.java:100)
	at io.vertx.core.impl.ContextImpl.lambda$executeBlocking$1(ContextImpl.java:157)
	at io.vertx.core.impl.TaskQueue.run(TaskQueue.java:76)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:829)

Fixed Issue(s)

#4084

Documentation

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

Changelog

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
worldStateBlockHash = blockHeader.getHash();
worldStateRootHash = newWorldStateRootHash;
} finally {
stateUpdater.commit();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be up at line 47/50 instead? Do we want to commit the state updater if persistTrieLog throws an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you right. Good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done thanks

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
@garyschulte garyschulte marked this pull request as ready for review July 16, 2022 02:24
@garyschulte
Copy link
Contributor

🚢

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
@jflo jflo enabled auto-merge (squash) July 18, 2022 18:37
@jflo jflo merged commit 3ce7f0f into hyperledger:main Jul 18, 2022
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
* candidate fix for missing parent worldstate
* add rollback if failure
* fix persist trie log issue
* add trielog manager
* fix inmemory issue for bonsai

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>

Co-authored-by: garyschulte <garyschulte@gmail.com>
Co-authored-by: Justin Florentine <justin+github@florentine.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants