Return new node on modification #3193

CoderZhi · 2022-03-15T00:15:32Z

Return a new node on any modification. With this change, it is possible to reuse the nodes and minimize the spending in constructing a trie.

#2847

codecov · 2022-03-16T06:31:21Z

Codecov Report

Merging #3193 (8fd2de8) into master (e9732a1) will decrease coverage by 0.17%.
The diff coverage is 78.21%.

@@            Coverage Diff             @@
##           master    #3193      +/-   ##
==========================================
- Coverage   74.95%   74.77%   -0.18%     
==========================================
  Files         269      269              
  Lines       23819    23880      +61     
==========================================
+ Hits        17854    17857       +3     
- Misses       5039     5095      +56     
- Partials      926      928       +2

Impacted Files	Coverage Δ
db/trie/mptrie/node.go	`100.00% <ø> (ø)`
action/protocol/execution/evm/evm.go	`44.41% <15.38%> (-1.02%)`	⬇️
db/trie/mptrie/hashnode.go	`36.36% <41.17%> (-54.55%)`	⬇️
db/trie/mptrie/merklepatriciatrie.go	`78.73% <46.80%> (-12.70%)`	⬇️
db/trie/mptrie/leafiterator.go	`62.06% <50.00%> (ø)`
action/protocol/execution/evm/evmstatedbadapter.go	`65.87% <64.28%> (-0.04%)`	⬇️
db/trie/mptrie/extensionnode.go	`91.91% <94.28%> (-1.07%)`	⬇️
db/trie/mptrie/branchnode.go	`96.40% <97.33%> (-2.21%)`	⬇️
action/protocol/context.go	`68.00% <100.00%> (+0.25%)`	⬆️
api/grpcserver.go	`83.33% <100.00%> (ø)`
... and 6 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Liuhaai · 2022-03-17T05:29:30Z

Could an new pr be created to introduce client interface?

CoderZhi · 2022-03-17T16:20:35Z

Could an new pr be created to introduce client interface?

is it hard to understand client interface?

dustinxie · 2022-03-18T00:11:00Z

db/trie/mptrie/hashnode.go

 	hashVal []byte
 }

-func newHashNode(mpt *merklePatriciaTrie, ha []byte) *hashNode {
-	return &hashNode{mpt: mpt, hashVal: ha}
+func newHashNode(_ client, ha []byte) *hashNode {


why leave client here if it's not used?

dustinxie · 2022-03-18T00:17:06Z

db/trie/mptrie/sortedlist.go

-	li       []uint8
-	isSorted bool // lazy-initilization
+	li     []uint8
+	sorted bool


does not hurt to leave the comment

dustinxie · 2022-03-18T00:29:05Z

db/trie/mptrie/node.go

@@ -17,20 +17,28 @@ var ErrNoData = errors.New("no data in hash node")
 type (
 	keyType []byte

+	client interface {


seems to me merklePatriciaTrie implements this interface, is there more purpose/use of this interface?

if that's the case, name it like trieOps? the name client is not clear in this context

dustinxie · 2022-03-18T00:34:26Z

db/trie/mptrie/node.go

+		Delete(client, keyType, uint8) (node, error)
+		Upsert(client, keyType, uint8, []byte) (node, error)
+		Hash(client) ([]byte, error)
+		Flush(client) error


is it possible not add client to the func's input, if the node struct (leaf, extension, branch) contains the client interface?
feel it's a bit heavy/clumsy with this extra input

dustinxie · 2022-03-18T00:35:33Z

db/trie/mptrie/node.go

+		hash(client, bool) ([]byte, error)
+		proto(client, bool) (proto.Message, error)
+		delete(client) error
+		store(client) error


Liuhaai · 2022-03-17T05:23:15Z

db/trie/mptrie/sortedlist.go

 		}
 	}
 	li := make([]uint8, 0, len(children))
 	for k := range children {
 		li = append(li, k)
 	}
 	return &SortedList{
-		li:       li,
-		isSorted: false,
+		li: li,


add sorted: false

db/trie/mptrie/extensionnode.go

Liuhaai · 2022-03-21T03:50:46Z

db/trie/mptrie/cachenode.go

 	hashVal []byte
 	ser     []byte
 }

-func (cn *cacheNode) Hash() ([]byte, error) {
-	return cn.hash(false)
+func (cn *cacheNode) Hash(c client) ([]byte, error) {


Liuhaai · 2022-03-21T03:54:50Z

db/trie/mptrie/branchnode.go

+			ser:     ser,
+		},
+		children: children,
+		indices:  b.indices,


Is b.indices.Clone() needed?

Liuhaai · 2022-03-21T04:24:49Z

db/trie/mptrie/branchnode.go

 		return nil, err
 	}
+	var indices *SortedList
+	var children map[byte]node


the creation of children could be moved before if condition

children := make(map[byte]node, len(b.children)) for k, v := range b.children { children[k] = v } if child == nil { delete(b.children, key) ....... } else { children[key] = child }

Liuhaai · 2022-03-21T04:37:10Z

db/trie/mptrie/sortedlist.go

 	}
 }

+func (sl *SortedList) sort() {


let's keep private func below public func

Liuhaai · 2022-03-21T04:46:41Z

Other things seems good for me apart from two things: 1. how is the performance of new func (b *branchNode) updateChild() compared with the old one in production; 2. I am not very confident about the change in sync mode. If a full sync could be done to verify the change, that'd be better.

huof6829 · 2022-08-25T14:46:51Z

db/trie/mptrie/merklepatriciatrie.go

+		mpt.resetRoot(emptyRoot, mpt.emptyRootHash)
+		return nil


return mpt.resetRoot because error unhandled

huof6829 · 2022-08-25T15:22:56Z

db/trie/mptrie/branchnode.go

 }

-func (b *branchNode) updateChild(key byte, child node, hashnode bool) (node, error) {
-	if err := b.delete(); err != nil {
+func (b *branchNode) updateChild(cli client, key byte, child node, hashnode bool) (node, error) {


hashnode is not used

huof6829 · 2022-08-25T15:24:05Z

db/trie/mptrie/extensionnode.go

 }

-func (e *extensionNode) updateChild(newChild node, hashnode bool) (node, error) {
-	err := e.delete()
+func (e *extensionNode) updateChild(cli client, newChild node, hashnode bool) (node, error) {


hashnode is not used

huof6829 · 2022-08-25T15:24:28Z

db/trie/mptrie/extensionnode.go

 }

-func (e *extensionNode) updatePath(path []byte, hashnode bool) (node, error) {
-	if err := e.delete(); err != nil {
+func (e *extensionNode) updatePath(cli client, path []byte, hashnode bool) (node, error) {


hashnode is not used

raullenchai · 2022-09-07T02:43:55Z

PTAL @dustinxie @Liuhaai @huof6829

Liuhaai · 2022-09-09T17:17:39Z

Except from hashnode bool mentioned by @huof6829, other things look good for me.

sonarcloud · 2022-09-21T03:58:11Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
6.5% Duplication

CoderZhi requested a review from a team as a code owner March 15, 2022 00:15

CoderZhi force-pushed the new_node_on_modification branch 3 times, most recently from 0c93b9e to 260b0d9 Compare March 16, 2022 04:35

return new node on modification

3e5b098

CoderZhi force-pushed the new_node_on_modification branch from 260b0d9 to 3e5b098 Compare March 16, 2022 04:38

add unit test

f423880

dustinxie reviewed Mar 18, 2022

View reviewed changes

CoderZhi added 2 commits March 18, 2022 16:13

remove unused parameters

9810c7a

Merge branch 'master' into new_node_on_modification

53ebbd2

Liuhaai reviewed Mar 21, 2022

View reviewed changes

address comments

0376fa9

Liuhaai mentioned this pull request Jun 17, 2022

[db] Add unit test for branchNode #3457

Closed

Merge branch 'master' into new_node_on_modification

ea31122

huof6829 reviewed Aug 25, 2022

View reviewed changes

Merge branch 'master' into new_node_on_modification

bf9f140

huof6829 added 3 commits September 16, 2022 10:29

Merge branch 'master' into new_node_on_modification

5127258

delete useless argument hashnode bool

ee3adb0

check return error in setRootHash

52392e8

huof6829 approved these changes Sep 16, 2022

View reviewed changes

Merge branch 'master' into new_node_on_modification

9c8a487

dustinxie approved these changes Sep 21, 2022

View reviewed changes

Merge branch 'master' into new_node_on_modification

8fd2de8

huof6829 merged commit c53ad41 into master Sep 21, 2022

dustinxie deleted the new_node_on_modification branch December 17, 2022 04:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return new node on modification #3193

Return new node on modification #3193

CoderZhi commented Mar 15, 2022 •

edited by raullenchai

Loading

codecov bot commented Mar 16, 2022 •

edited

Loading

Liuhaai commented Mar 17, 2022

CoderZhi commented Mar 17, 2022

dustinxie Mar 18, 2022

dustinxie Mar 18, 2022

dustinxie Mar 18, 2022 •

edited

Loading

dustinxie Mar 18, 2022

dustinxie Mar 18, 2022

Liuhaai Mar 17, 2022

Liuhaai Mar 21, 2022

Liuhaai Mar 21, 2022

Liuhaai Mar 21, 2022

Liuhaai Mar 21, 2022

Liuhaai commented Mar 21, 2022 •

edited

Loading

huof6829 Aug 25, 2022

huof6829 Aug 25, 2022

huof6829 Aug 25, 2022

huof6829 Aug 25, 2022

raullenchai commented Sep 7, 2022

Liuhaai commented Sep 9, 2022

sonarcloud bot commented Sep 21, 2022

Return new node on modification #3193

Return new node on modification #3193

Conversation

CoderZhi commented Mar 15, 2022 • edited by raullenchai Loading

codecov bot commented Mar 16, 2022 • edited Loading

Codecov Report

Liuhaai commented Mar 17, 2022

CoderZhi commented Mar 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustinxie Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Liuhaai commented Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raullenchai commented Sep 7, 2022

Liuhaai commented Sep 9, 2022

sonarcloud bot commented Sep 21, 2022

CoderZhi commented Mar 15, 2022 •

edited by raullenchai

Loading

codecov bot commented Mar 16, 2022 •

edited

Loading

dustinxie Mar 18, 2022 •

edited

Loading

Liuhaai commented Mar 21, 2022 •

edited

Loading