-
Notifications
You must be signed in to change notification settings - Fork 20k
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
beacon, core, eth, miner: integrate witnesses into production Geth #30069
Conversation
@@ -1713,20 +1715,20 @@ func (bc *BlockChain) insertChain(chain types.Blocks, setHead bool) (int, error) | |||
if setHead { | |||
// First block is pruned, insert as sidechain and reorg only if TD grows enough | |||
log.Debug("Pruned ancestor, inserting as sidechain", "number", block.Number(), "hash", block.Hash()) | |||
return bc.insertSideChain(block, it) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
insertSideChain
is only called if: (a) parent state is pruned (b) the blocks are inserted via InsertChain
.
Given that InsertChain
is only used in downloader with the semantics of insert the block as canonical chain. Therefore, we don't need to bother the witness construction here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this part was needed so that I could run the state tests for pre-merge networks too? Can't really say for sure, but I kind of remember there was one code path that felt weird to me too, but was needed to allow running the old tests. Maybe not this one?
core/blockchain.go
Outdated
@@ -1713,20 +1715,20 @@ func (bc *BlockChain) insertChain(chain types.Blocks, setHead bool) (int, error) | |||
if setHead { | |||
// First block is pruned, insert as sidechain and reorg only if TD grows enough | |||
log.Debug("Pruned ancestor, inserting as sidechain", "number", block.Number(), "hash", block.Hash()) | |||
return bc.insertSideChain(block, it) | |||
return bc.insertSideChain(block, it, makeWitness) | |||
} else { | |||
// We're post-merge and the parent is pruned, try to recover the parent state | |||
log.Debug("Pruned ancestor", "number", block.Number(), "hash", block.Hash()) | |||
_, err := bc.recoverAncestors(block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should pass the makeWitness flag into recoverAncestors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And inside of recoverAncestors
, we only need to generate the witness if makeWitness
is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so we only want to generate a witness for the very last block. I.e. we only care about witnesses from newPayload calls. As far as I understand this code, we aare only recovering the parent, and for that we never want to make a witness. Later on line 1820 is when we actually make the witness. The reason we have the witness enabled on 1718 above is because that returns it directly, but the recover only preps the ancestor. At least accoring to the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The recoverAncestors
will be called in the call site of newPayload
, specifically:
newPayload
callsInsertBlockWithoutSetHead
InsertBlockWithoutSetHead
callsrecoverAncestors
if the state of parent block is not available- the block specified by
newPayload
is executed within therecoverAncestors
after all its ancestors
suggested diff
diff --git a/core/blockchain.go b/core/blockchain.go
index 8030fb84d1..f7c921fe64 100644
--- a/core/blockchain.go
+++ b/core/blockchain.go
@@ -1688,7 +1688,7 @@ func (bc *BlockChain) insertChain(chain types.Blocks, setHead bool, makeWitness
} else {
// We're post-merge and the parent is pruned, try to recover the parent state
log.Debug("Pruned ancestor", "number", block.Number(), "hash", block.Hash())
- _, err := bc.recoverAncestors(block)
+ _, err := bc.recoverAncestors(block, makeWitness)
return nil, it.index, err
}
// Some other error(except ErrKnownBlock) occurred, abort.
@@ -2110,7 +2110,7 @@ func (bc *BlockChain) insertSideChain(block *types.Block, it *insertIterator, ma
// all the ancestor blocks since that.
// recoverAncestors is only used post-merge.
// We return the hash of the latest block that we could correctly validate.
-func (bc *BlockChain) recoverAncestors(block *types.Block) (common.Hash, error) {
+func (bc *BlockChain) recoverAncestors(block *types.Block, makeWitness bool) (common.Hash, error) {
// Gather all the sidechain hashes (full blocks may be memory heavy)
var (
hashes []common.Hash
@@ -2150,7 +2150,7 @@ func (bc *BlockChain) recoverAncestors(block *types.Block) (common.Hash, error)
} else {
b = bc.GetBlock(hashes[i], numbers[i])
}
- if _, _, err := bc.insertChain(types.Blocks{b}, false, false); err != nil {
+ if _, _, err := bc.insertChain(types.Blocks{b}, false, makeWitness && i == 0); err != nil {
return b.ParentHash(), err
}
}
@@ -2387,7 +2387,7 @@ func (bc *BlockChain) SetCanonical(head *types.Block) (common.Hash, error) {
// Re-execute the reorged chain in case the head state is missing.
if !bc.HasState(head.Root()) {
- if latestValidHash, err := bc.recoverAncestors(head); err != nil {
+ if latestValidHash, err := bc.recoverAncestors(head, false); err != nil {
return latestValidHash, err
}
log.Info("Recovered head state", "number", head.Number(), "hash", head.Hash())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied
For binary representation, an approach on SSZ would benefit latency and throughput. One can follow the same concept of beacon-API and switch to SSZ based on the HTTP SSZ supports optional fields with EIP-7495: SSZ StableContainer. This is currently being implemented across many libraries, with implementation progress documented at https://stabilitynow.box The |
I am implementing this prototype in reth as part of my ethereum protocol fellowship project, it would help in further benchmarking |
8c70658
to
f12bed5
Compare
a90749f
to
ade5cbb
Compare
Please address the one comment I left, otherwise lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if gary's point is addressed. Also, the new idea is to export the witness to the CL, and have the CL direct the validations, is that correct?
If so, I don't think that's ideal. It forces the data to be fed over http, to CL, then (most likely) over http again to some other EL. Beause afaik there's no "pipe" integration with any CL.
However, since the "extra-validator EL" client should be pretty low on ram consumption and negligible on disk, it should be perfectly doable to run both geth + another witness-validating EL on the same machine. And then the network roundtrips can be avoided if geth puts it on disk and the other clients just read it back up.
ade5cbb
to
599be27
Compare
Yes.
HTTP itself is not an issue, as long as you're shuffling data in RAM it's fast. If your EL and CL is split across different machines, then you do have the networking latency/bandwidth to consider, but given that most people will run EL/CL on the same machine, this is moot. For those who run it split, a gigabit network will incur a 30-40ms delay one way, myeah, not ideal, but also not that terrible. The reason I went with engine API is because that's the only available channel for this interoperability (nobody will ship a new thing, that's guaranteed) + it will be needed for Verkle in a similar way (need to solve the same problems) + Verkle will make the witnesses small enough not to matter even so.
Yes, but that requires inventing a new communication pathway between ELs, which nobody will do. That is why I discarded all such attempts. ELs' existing evm runner is also unsuitable, so I can't piggieback on that either. All things considered, the goal was to make it shippable as a first version and iterate vs overdesign and never ship. |
Regarding the order of operations applied to trie (deletions) mentioned by below paragraph: What would be the thoughts on standardizing it as
This way the witness is deterministic and can be shared across clients cc: @karalabe |
This PR integrates witness-enabled block production, witness-creating payload execution and stateless cross-validation into the
engine
API. The purpose of the PR is to enable the following use-cases (for API details, please see next section):Cross validating locally created blocks:
forkchoiceUpdatedWithWitness
instead offorkchoiceUpdated
to trigger witness creation too.getPayload
as before to retrieve the new block and also the above created witness.executeStatelessPayload
against another client to cross-validate the block.Cross validating locally processed blocks:
newPayloadWithWitness
instead ofnewPayload
to trigger witness creation too.executeStatelessPayload
against another client to cross-validate the block.Block production for stateless clients (local or MEV builders):
forkchoiceUpdatedWithWitness
instead offorkchoiceUpdated
to trigger witness creation too.getPayload
as before to retrieve the new block and also the above created witness.Stateless validator validation:
executeStatelessPayload
with the propagated witness to statelessly validate the block.Note, the various
WithWitness
methods could also just be an additional boolean flag on the base methods, but this PR wanted to keep the methods separate until a final consensus is reached on how to integrate in production.The following
engine
API types are introduced:forkchoiceUpdatedWithWitnessV1,2,3
with same params and returns asforkchoiceUpdatedV1,2,3
, but triggering a stateless witness building if block production is requested.getPayloadV2,3
to returnexecutionPayloadEnvelope
with an additionalwitness
field of typebytes
iff created viaforkchoiceUpdatedWithWitnessV2,3
.newPayloadWithWitnessV1,2,3,4
with same params and returns asnewPayloadV1,2,3,4
, but triggering a stateless witness creation during payload execution to allow cross validating it.payloadStatusV1
with awitness
field of typebytes
if returned bynewPayloadWithWitnessV1,2,3,4
.executeStatelessPayloadV1,2,3,4
with same base params asnewPayloadV1,2,3,4
and one more additional param (witness
) of typebytes
. The method returnsstatelessPayloadStatusV1
, which mirrorspayloadStatusV1
but replaceslatestValidHash
withstateRoot
andreceiptRoot
.