Skip to content
This repository has been archived by the owner on Jan 19, 2021. It is now read-only.

Safeguard against bug ganache-cli #417 #29

Closed
wants to merge 1 commit into from

Conversation

rosario
Copy link

@rosario rosario commented Dec 27, 2017

  1. The function Trie.prototype.getRaw can only call db.get when db is defined

  2. The function Trie.prototype.put will call _updateNode when stack is defined

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 81.057% when pulling eba916c on rosario:master into d3baf12 on ethereumjs:master.

@benjamincburns
Copy link

Thanks for submitting this @rosario. Maybe I'm misunderstanding the change, but it looks like this masks the race condition rather than fixing its underlying cause. I think we want to fail fast, and fail loudly here, and fix the logic which causes the callback to findPath in put to be called without its stack argument, and which causes dbGet to be called when db is undefined.

@@ -99,7 +99,7 @@ Trie.prototype.put = function (key, value, cb) {
if (self.root.toString('hex') !== ethUtil.SHA3_RLP.toString('hex')) {
// first try to find the give key or its nearst node
self.findPath(key, function (err, foundValue, keyRemainder, stack) {
if (err) {
if (err || !stack) {
return cb(err)

Choose a reason for hiding this comment

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

If stack is undefined, it's likely that err is also undefined or null. It seems this should still call cb with an error in this case.

Copy link
Author

Choose a reason for hiding this comment

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

If I understood correctly there might be no error, but the stack would be undefined. Then updateNode would fail because there is no stack.

@rosario
Copy link
Author

rosario commented Jan 2, 2018

Very good point @benjamincburns. True, it's not fixing the race condition, but it would safeguard from calling functions on undefined values. I see now why it's ok to fail on those conditions

@holgerd77
Copy link
Member

Just merged #28 and also read through the various discussions on this on various places but got lost at some point.

Are you guys still consider this as a PR to be considered?

@holgerd77
Copy link
Member

Any comments on this (see post above)? Otherwise I would close.

@holgerd77 holgerd77 closed this May 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants