Skip to content

trie, eth/protocols/snap: sanitize the committed node data#29485

Merged
rjl493456442 merged 1 commit intoethereum:masterfrom
rjl493456442:enhance-state-heal
May 16, 2024
Merged

trie, eth/protocols/snap: sanitize the committed node data#29485
rjl493456442 merged 1 commit intoethereum:masterfrom
rjl493456442:enhance-state-heal

Conversation

@rjl493456442
Copy link
Copy Markdown
Member

@rjl493456442 rjl493456442 commented Apr 8, 2024

This pull request adds one more check before committing the trie nodes from the trie.Syncer:

  • for node deletion, the node blob must be zero-length
  • for node update, the node blob must be non-zero-length

The check is cheap enough and shouldn't have any performance impact.


The node blob cached in the trie.Syncer is not deep-copied, rather than a reference of the network packet.

Theoretically it should be safe the hold the network packet reference being originated from here, although i am not 100% confident about it.

If it's possible to empties the referenced blob somehow, than it could be the root cause of #29436

Comment thread trie/sync.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't you rewrite isDelete into:

func (op *nodeOp) isDelete() bool {
	return op.del
}

Alternatively: delete the method

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep, the method should be deleted

@karalabe
Copy link
Copy Markdown
Member

This PR seems simple and good, just maybe not needed (making the surrounding code harder to understand). My 2c, lets leave it open until the root cause for the sync issue is discovered and if it's related to this, we should merge. If it's unrelated, it might be better to close.

Copy link
Copy Markdown
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants