block validation by rootchain contract #37
Conversation
core/types/block.go
Outdated
| @@ -34,7 +34,7 @@ import ( | |||
| ) | |||
|
|
|||
| var ( | |||
| EmptyRootHash = DeriveSha(Transactions{}) | |||
| EmptyRootHash = DeriveShaFromBMT(Transactions{}) | |||
There was a problem hiding this comment.
EmptyRootHash stands for empty patricia tree, and it is used in ethclient and storage hash. So it should not be touched.
We use additional empty root, EmptyBMTRootHash, for empty binary merkle tree.
pls/rootchain_manager_test.go
Outdated
| rootchainAddress, rootchainContract, err := deployRootChain(genesis) | ||
|
|
||
| chainConfig.RootchainAddress = rootchainAddress | ||
| chainConfig.RootchainURL = "ws://localhost:8546" |
pls/backend.go
Outdated
| @@ -159,6 +159,10 @@ func New(ctx *node.ServiceContext, config *Config) (*Plasma, error) { | |||
| } | |||
| cacheConfig = &core.CacheConfig{Disabled: config.NoPruning, TrieCleanLimit: config.TrieCleanCache, TrieDirtyLimit: config.TrieDirtyCache, TrieTimeLimit: config.TrieTimeout} | |||
| ) | |||
|
|
|||
| pls.chainConfig.RootchainAddress = config.RootChainContract | |||
| pls.chainConfig.RootchainURL = config.RootChainURL | |||
There was a problem hiding this comment.
chainConfig.RootchainAddress and chainConfig.RootchainURL is duplicated with pls.Config.
pls/rootchain_manager_test.go
Outdated
| rootchainAddress, rootchainContract, err := deployRootChain(genesis) | ||
|
|
||
| chainConfig.RootchainAddress = rootchainAddress | ||
| chainConfig.RootchainURL = "ws://localhost:8546" |
core/block_validator.go
Outdated
| // Validate 3 roots from block committed on root chain against the received roots | ||
| // and throw an error if they don't match | ||
| validate := func() error { | ||
| tblock, err := v.rootchain.GetBlock(baseCallOpt, block.Difficulty(), block.Number()) |
There was a problem hiding this comment.
In case of v.rootchain == nil, skip the validation. It is useful when blockchain without RootChain contract is required.
core/block_validator.go
Outdated
| log.Error("failed to make rootchainContract", "err", err) | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
If rootchain is needed only for validator, just add it as argument of newBlockValidator (as well as NewBlockChain).
It makes us keep params.ChainConfig untouched.
core/block_validator.go
Outdated
| return nil | ||
| } | ||
|
|
||
| timer := time.NewTimer(0) |
There was a problem hiding this comment.
we may use time.Ticker rather than time.Timer.
| for { | ||
| select { | ||
| case <-timer.C: | ||
| if v.bc.LastSubmittedNumber.Cmp(block.Number()) < 0 && v.bc.LastSubmittedFork.Cmp(block.Difficulty()) < 0 { |
There was a problem hiding this comment.
LastSubmittedNumber and LastSubmittedFork may be nil if block is not submitted. Read it from RootChain contract and save it when pls is created.
There was a problem hiding this comment.
I think LastSubmittedNumber and LastSubmittedFork are not necessary since we can check whether a specific block is submitted or not by checking the return value of rootchain.Getblock() is empty.
There was a problem hiding this comment.
In that case, Every validate requires one JSONRPC communication.
I think we can also use LastSubmittedNumber and LastSubmittedFork when we submit TX (in the part of gas adjustment)
miner/worker.go
Outdated
| @@ -1091,9 +1091,6 @@ func (w *worker) commitNewWorkForORB(interrupt *int32, noempty bool, timestamp i | |||
| log.Error("Failed to prepare header for mining", "err", err) | |||
| return | |||
| } | |||
| if w.env.IsUserActivated && w.env.NumURBmined.Cmp(big.NewInt(0)) == 0 { | |||
| header.Difficulty.Add(header.Difficulty, big.NewInt(1)) | |||
| } | |||
There was a problem hiding this comment.
Where the first URB with difficulty + 1 is created?
There was a problem hiding this comment.
It caused build error since this version do not support UAF. So I deleted it for testing my code. It will added if you merge #31. So don't worry about this. Additionally, there is no more w.env.NumURBmined field in new version.
There was a problem hiding this comment.
Can you merge and resolve the conflict in current develop branch? This PR is based on older commit, so incurs this kind of issue. If you merge newer one, it will help me to review.
dCanyon
force-pushed
the
p2p-validation
branch
from
488d9c2 to
21ed79a
Compare
| @@ -108,10 +108,10 @@ func (ec *Client) getBlock(ctx context.Context, method string, args ...interface | |||
| if head.UncleHash != types.EmptyUncleHash && len(body.UncleHashes) == 0 { | |||
| return nil, fmt.Errorf("server returned empty uncle list but block header indicates uncles") | |||
| } | |||
| if head.TxHash == types.EmptyRootHash && len(body.Transactions) > 0 { | |||
| if head.TxHash == types.EmptyBMTRootHash && len(body.Transactions) > 0 { | |||
There was a problem hiding this comment.
ethclient is ethereum JSONRPC client and it is used when to connect to root chain. On the other hand, plsclient is used to connect plasma JSONRPC server.
It have to use EmptyRootHash instead of EmptyBMTRootHash because ethereum client use merkle patricia trie.
No description provided.