Fix race condition due to mutated _getDBs/_putDBs

[federicobond]

This fixes part of the issues described in trufflesuite/ganache#417.

The asyncFirstSeries function called inside Trie#getRaw was iterating over an array that could be mutated by the checkpoint _enterCpMode/_exitCpMode functions, causing the familiar TypeError: Cannot read property 'get' of undefined.

I replaced the mutations with reassignments.

Note that the tests written in trufflesuite/ganache#41 still fail, now both with the same error: TypeError: Cannot read property 'pop' of undefined.

[coveralls]

Coverage remained the same at 81.149% when pulling ffa0c17 on federicobond:fix-typerror into d3baf12 on ethereumjs:master.

[holgerd77]

Sorry, this was left aside a bit. Will have a look in the upcoming days (maybe next week).

[holgerd77]

Hi @federicobond, trying to grab my head around this for review but I really have problems to understand why mutation and reassignment to self behaves differently. Will continue to dig into this but if you are around a note on this would be helpful.

There also was a second PR https://github.com/ethereumjs/merkle-patricia-tree/pull/30/files after this one fixing the same issue as here, but also touching the _putDBs function. I would also look into this if this fix also applies to this function, if you have a quick opinion on this, this also would be helpful.

Thanks!

[holgerd77]

Some context:
https://stackoverflow.com/questions/36095779/reassignment-mutation-reference-types-and-value-types
https://lorenstewart.me/2017/01/22/javascript-array-methods-mutating-vs-non-mutating/

[holgerd77]

Anyhow, since you guys are discussing this for ages (just stumbled on the discussion on the bounty website and this appears to be a widely accepted patch, I will merge.

A late explanation would be nice anyhow.

Also if you see a chance to put this into some test like this from test/index.js would be nice, did you try or is this too un-reproducable due to the race condition nature?

it('should commit another nested checkpoint', function (t) {
    trie.checkpoint()
    var root
    trie.put('test', 'something else', function () {
      root = trie.root
      trie.checkpoint()
      trie.get('test', function (err, val) {
        trie.put('the feels', 'emotion', function () {
          trie.commit(function () {
            console.log(val)
            t.end()
          })
        })
      })
    })
  })

[holgerd77]

Will approve, see discussion.

[holgerd77]

Just released this together with another bug fix as v2.3.1 on npm: https://github.com/ethereumjs/merkle-patricia-tree/releases/tag/v2.3.1

[federicobond]

trying to grab my head around this for review but I really have problems to understand why mutation and reassignment to self behaves differently

In case of mutation, an async iteration that started before may see the array in an inconsistent state. By reassigning we are letting the previous procedure keep a copy of the original list and only affect future requests.

Also if you see a chance to put this into some test like this from test/index.js would be nice, did you try or is this too un-reproducable due to the race condition nature?

I think it may be possible to write a test, but AFAIK depends on the specific event processing order and it's quite tricky to get right. I haven't attempted it though.

[federicobond]

Thank you @holgerd77!