fast node verification #4

kyrie-yl · 2022-01-20T09:21:24Z

No description provided.

cmd/utils/flags.go

core/blockchain.go

core/verify_manager.go

core/verify_modes.go

core/verify_peers.go

core/verify_task.go

core/verify_manager.go

Signed-off-by: kyrie-yl <[email protected]>

unclezoro · 2022-01-25T11:52:52Z

cmd/geth/main.go

@@ -71,6 +71,8 @@ var (
 		utils.NoUSBFlag,
 		utils.DirectBroadcastFlag,
 		utils.DisableSnapProtocolFlag,
+		utils.DisableDiffProtocolFlag,


I mean it is not proper to introduce such low level flag. Especially it is related with other flags.
For example: when TriesVerifyModeFlag need trust protocol while EnableTrustProtocolFlag is false, it is become complicated.

Consider we should disable diff protocol in verify node, and enable trust protocol, we didn't have better solution temporary.

unclezoro · 2022-01-25T11:55:52Z

cmd/utils/flags.go

+	defaultVerifyMode = ethconfig.Defaults.TriesVerifyMode
+	TriesVerifyModeFlag = TextMarshalerFlag{
+		Name: "tries-verify-mode",
+		Usage: `tries verify mode: "local", "full", "light", "insecure"`,


please explain in details what the each mode means in the usage so that the user can understand

core/verify_modes.go

unclezoro · 2022-01-25T12:05:38Z

core/verify_modes.go

+}
+
+func (mode *VerifyMode) NeedRemoteVerify() bool {
+	if *mode == FullVerify || *mode == LightVerify {


return *mode == FullVerify || *mode == LightVerify directly?

unclezoro · 2022-01-25T12:25:52Z

core/blockchain.go

@@ -239,6 +239,7 @@ type BlockChain struct {
 	engine    consensus.Engine
 	validator Validator // Block and state validator interface
 	processor Processor // Block transaction processor interface
+	verifyManager *VerifyManager


Shall we use interface here, like validator, processor ?

verifyManager is not a good name, we need distinguish it from validator here.

eth/backend.go

unclezoro · 2022-01-25T12:36:24Z

core/blockchain.go

@@ -462,6 +463,16 @@ func NewBlockChain(db ethdb.Database, cacheConfig *CacheConfig, chainConfig *par
 	return bc, nil
 }

+func (bc *BlockChain) StartVerify(peers VerifyPeers, allowUntrustedVerify bool) {


Can we not pass allowUntrustedVerify when StartVerify, but pass it when new the verifyManager.

unclezoro · 2022-02-10T13:44:13Z

core/trie_verify.go

+	header := vm.bc.CurrentHeader()
+	// Start verify task from H to H-11 if need.
+	vm.NewBlockVerifyTask(header)
+	prune := time.NewTicker(time.Second)


prune --> pruneTicker

1 second is too short for pruneTicker, I will suggest 5 seconds

unclezoro · 2022-02-10T13:47:30Z

core/trie_verify.go

+			}
+		case <-prune.C:
+			for hash, task := range vm.tasks {
+				if vm.bc.CurrentHeader().Number.Uint64()-task.blockHeader.Number.Uint64() > 15 {


15 is magic number here.

The subtraction here may overflow.

unclezoro · 2022-02-10T13:50:52Z

core/trie_verify.go

+
+func (vm *VerifyManager) Stop() {
+	// stop all the tasks
+	for _, task := range vm.tasks {


race condition on vm.tasks, it may double close task.terminalCh and cause panic

unclezoro · 2022-02-10T14:01:33Z

core/trie_verify.go

+func (vt *VerifyTask) Start(verifyCh chan common.Hash) {
+	vt.startAt = time.Now()
+
+	vt.selectPeersToVerify(defaultPeerNumber)


Please try better name thanselectPeersToVerify. 主语是select，就是挑选，实际上会把请求发出去。

unclezoro · 2022-02-10T14:03:36Z

core/trie_verify.go

+			validPeers = append(validPeers, p)
+		}
+	}
+	// if


clean the code.

unclezoro · 2022-02-10T14:05:15Z

core/trie_verify.go

+	}
+
+	// if n < len(validPeers), select n peers from validPeers randomly.
+	rand.Seed(time.Now().UnixNano())


Please log(warn) if there is no node to request.

unclezoro · 2022-02-10T14:07:14Z

core/trie_verify.go

+func (vt *VerifyTask) compareRootHashAndWrite(msg VerifyMessage, verifyCh chan common.Hash) {
+	if msg.verifyResult.Root == vt.blockHeader.Root {
+		blockhash := msg.verifyResult.BlockHash
+		rawdb.MarkTrustBlock(vt.db, blockhash)


we have done MarkTrustBlock twice, I think we can delete here, but let VerifyManager to do it.

unclezoro · 2022-02-10T14:12:52Z

core/types.go

+	// VerifyBlock verify the given blcok.
+	VerifyBlock(header *types.Header)
+	// ValidateBlockVerify validate the given block has been verified.
+	ValidateBlockVerify(block *types.Block) error


ValidateBlockVerify is too confusing...

unclezoro · 2022-02-10T14:15:03Z

core/types.go

@@ -32,6 +32,17 @@ type Validator interface {
 	// ValidateState validates the given statedb and optionally the receipts and
 	// gas used.
 	ValidateState(block *types.Block, state *state.StateDB, receipts types.Receipts, usedGas uint64) error
+
+	// StartRemoteVerify start the remote verify routine for given peers.
+	StartRemoteVerify(peers VerifyPeers)


I think there is some way to hide StartRemoteVerify and StopRemoteVerify, we should expose as less interface as possible.

unclezoro · 2022-02-10T14:16:26Z

core/types.go

+	// StopRemoteVerify stop the remote verify routine.
+	StopRemoteVerify()
+	// VerifyBlock verify the given blcok.
+	VerifyBlock(header *types.Header)


We can merge VerifyBlock and ValidateBody into one.

keefel · 2022-02-12T09:39:18Z

core/block_validator.go

 	validator := &BlockValidator{
 		config: config,
 		engine: engine,
 		bc:     blockchain,
 	}
+	if mode.NeedRemoteVerify() {
+		remoteValidator := NewVerifyManager(blockchain, peers, *mode == InsecureVerify)


Should assign validator's remoteValidator, not just a local variable.

keefel · 2022-02-12T09:41:41Z

core/block_validator.go

+		if !v.remoteValidator.AncestorVerified(v.bc.GetHeaderByNumber(header.Number.Uint64())) {
+			return fmt.Errorf("block's ancessor %x has not been verified", block.Hash())
+		}
+	}


Wrap these code as a new validateFunc is better? Can run concurrently with above another two validateFuncs

Please run concurrently like another two validateFuncs.

keefel · 2022-02-12T10:07:50Z

core/trie_verify.go

+				if diffLayer, err = vm.bc.GenerateDiffLayer(hash); err != nil {
+					log.Error("failed to get diff layer", "block", hash, "number", header.Number, "error", err)
+					return
+				}


There is a case if the block is an empty block, the error is nil and difflayer is nil too. So, add another branch to test if difflayer == nil, if so, return directly.

keefel · 2022-02-12T10:16:23Z

core/trie_verify.go

+	if header.TxHash == types.EmptyRootHash {
+		parent := vm.bc.GetHeaderByHash(header.ParentHash)
+		if header.Root == parent.Root {
+			return true


No need use if branch, actually if you use if branch, when not equal, you should use else branch to return false here, so recommend to return header.Root == parent.Root directly.

I think when header.Root == parent.Root, we should check whether the parent has been verified, if not, this empty block is not verified, if the parent has been verified already, this empty block is verified. So maybe here should be
if header.Root != parent.Root { return false }

unclezoro · 2022-02-14T02:53:53Z

core/trie_verify.go

+		allowUntrusted: allowUntrusted,
+
+		newTaskCh: make(chan *types.Header),
+		verifyCh:  make(chan common.Hash),


I will suggest verifyCh have some buffer, like 11, so than the main process will not been blocked.

So do newTaskCh, please allocate some buffer.

unclezoro · 2022-02-14T02:58:07Z

cmd/utils/flags.go

-		Usage: `Disable the tries state root verification, the state consistency is no longer 100% guaranteed, diffsync is not allowed if enabled. Do not enable it unless you know exactly what the consequence it will cause.`,
+	defaultVerifyMode = ethconfig.Defaults.TriesVerifyMode
+	TriesVerifyModeFlag = TextMarshalerFlag{
+		Name: "tries-verify-mode",


Better description is needed to let users understand the meaning of each mode

unclezoro · 2022-02-14T03:05:51Z

cmd/utils/flags.go

+		}
+		// If a node sets verify mode but not local, it's a fast node whose difflayer is not integral.
+		// So fast node should disable diff protocol.
+		if cfg.TriesVerifyMode != core.LocalVerify {


After discuss with nash, we can delete flag DisableDiffProtocol, even fast node can provide difflayer for diff sync.

unclezoro · 2022-02-14T03:07:20Z

core/rawdb/schema.go

@@ -93,6 +93,9 @@ var (
 	// difflayer database
 	diffLayerPrefix = []byte("d") // diffLayerPrefix + hash  -> diffLayer

+	// trust block database
+	trustBlockPrefix = []byte("trust-block-") // trustBlockPrefix + hash -> verify result


Please delete useless code

unclezoro · 2022-02-14T03:07:30Z

core/rawdb/schema.go

@@ -165,6 +168,11 @@ func headerHashKey(number uint64) []byte {
 	return append(append(headerPrefix, encodeBlockNumber(number)...), headerHashSuffix...)
 }

+// trustBlockHashKey = trustBlockPrefix + hash
+func trustBlockHashKey(hash common.Hash) []byte {


Please delete useless code

unclezoro · 2022-02-14T03:07:54Z

core/trie_verify.go

+	tryAllPeersTime = 15 * time.Second
+)
+
+type verifyManager struct {


Please rename verifyManager to remoteVerifyManager

unclezoro · 2022-02-14T03:11:41Z

core/trie_verify.go

@@ -0,0 +1,345 @@
+package core


The file name trie_verify can rename to remote_state_verifier

unclezoro · 2022-02-14T03:12:52Z

core/trie_verify.go

+	return nil
+}
+
+func (mode *VerifyMode) NeedRemoteVerify() bool {


Please change func (mode *VerifyMode) NeedRemoteVerify() bool to func (mode VerifyMode) NeedRemoteVerify() bool, so that you can change

func NewBlockValidator(config *params.ChainConfig, blockchain *BlockChain, engine consensus.Engine, mode *VerifyMode, peers verifyPeers) *BlockValidator

to

func NewBlockValidator(config *params.ChainConfig, blockchain *BlockChain, engine consensus.Engine, mode VerifyMode, peers verifyPeers) *BlockValidator {

unclezoro · 2022-02-14T03:20:50Z

core/block_validator.go

@@ -149,6 +160,16 @@ func (v *BlockValidator) ValidateState(block *types.Block, statedb *state.StateD
 	return err
 }

+func (v *BlockValidator) VerifyBlockTrie(header *types.Header) {


Let us try subscribe the header info rather than called in main process.

unclezoro · 2022-02-14T03:24:14Z

core/trie_verify.go

+	tasks          map[common.Hash]*verifyTask
+	peers          verifyPeers
+	verifiedCache  *lru.Cache
+	allowUntrusted bool


can we rename it to allowInsecure, since the peer is still trusted, but the result is not insecure

unclezoro · 2022-02-14T03:42:21Z

Please add promethus metrics

Signed-off-by: kyrie-yl <[email protected]>

[R4R] add mev module

kyrie-yl force-pushed the fastnode-v1 branch from 2e0530d to 95b1650 Compare January 20, 2022 09:32