blockchain: fix updateCache

[samlior]

When the database operation is del, this.baseDBOp.value is undefined instead of Buffer.
In the old code, when deleting a value, the value is deleted in the database but not in the cache, causing the next read value still exists (because DBManager always tries to load data from cache first)

BTW, I tried to add a test case for this fix, but to find out that neither DBManager nor DBOp has a test case, maybe this should be improved?

[codecov]

Codecov Report

Merging #1786 (274690c) into master (b5d9fb0) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Flag	Coverage Δ
block	85.57% <ø> (ø)
blockchain	83.82% <100.00%> (+0.53%)	⬆️
client	75.82% <ø> (ø)
common	93.90% <ø> (ø)
devp2p	82.56% <ø> (-0.14%)	⬇️
ethash	90.76% <ø> (ø)
trie	80.02% <ø> (ø)
tx	88.20% <ø> (ø)
util	92.62% <ø> (ø)
vm	81.17% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

[ryanio]

thanks for the PR!

i find this type of conditional chaining kind of awkward, can you instead add to the if statement above?

[samlior]

public updateCache(cacheMap: CacheMap) {
  if (this.cacheString && cacheMap[this.cacheString]) {
    if (this.baseDBOp.type == 'put' && Buffer.isBuffer(this.baseDBOp.value)) {
      cacheMap[this.cacheString].set(this.baseDBOp.key, this.baseDBOp.value)
    } else if (this.baseDBOp.type == 'del') {
      cacheMap[this.cacheString].del(this.baseDBOp.key)
    } else {
      throw new Error('unsupported db operation on cache')
    }
  }
}

It will throw an exception if type is put and value is not Buffer

[holgerd77]

Ah ok, thanks for the explanation! 👍

[ryanio]

gotcha, thanks. i think i still would've preferred if we formatted this as an additional inside if statement

if (this.baseDBOp.type == 'put') {
  if (Buffer.isBuffer(this.baseDBOp.value)) {
    cacheMap[this.cacheString].set(this.baseDBOp.key, this.baseDBOp.value)
  }
}

but that's fine for now. thanks again for the pr :)

good point about missing tests, would be great to have some for these

[holgerd77]

Ok, guess we can maybe update "on occasion" if someone stumbles upon further down the road (e.g. @emersonmacro along his work on #1044).

[holgerd77]

Thanks a lot, this is a really important fix, since this can cause all sorts of underlying weird behavior, which is extremely hard to debug, especially if triggered e.g. in the outer client context! 😄

Will directly add this to the upcoming round of releases here #1774.

Yes, additional tests might be a good idea. We will likely do some substantial refactoring around the Blockchain class during the work towards the next major releases #1717 - e.g. with the encapsulation of all consensus functionality. Will eventually make sense to wait for that before going too deep into test writing. But also not 100% sure, might not affect the tests (test structure) as much as I am thinking on this first round.

[holgerd77]

Ah ok, thanks for the explanation! 👍

[holgerd77]

(small nit: in case you are opening more PRs towards the monorepo (highly welcome! 😀) it would be good if you can open on your side from a dedicated feature branch and not from (your) fork master branch. That makes local testing and the like more robust and less confusing).