tracking issue - invalid rangeproof handling (good header, bad block)

[antiochp]

The following PRs were merged to current/5.0.x as part of the emergency hardfork on 20210318 -

• [5.0.x] Removing verifier caches #3598 "Removing verifier caches"
• [5.0.x] Rewind bad block #3600 "Rewind bad block"
• [5.0.x] rewind bad headers, ban on explicit bad headers #3603 "Rewind bad headers"

Additionally the following PR was merged to master -

• Removing verifier caches #3597 "Removing verifier caches"

Some of this work was "tactical" and we do not need this on master.
Some of this work was "strategic" and we do want it on master.

This tracking issue is a central place to document what needs to be on master and what we can omit.
There are also some additional changes/improvements that we want to consider, off the back of the hardfork and fix.

This is a work in progress and details will be updated as we go.
This is not time-critical but the proposed upcoming 5.1.0 release is a good candidate for cleaning this up if possible.

---

This is rough first draft of where we stand currently -

• Currently the rangeproof verification cache is disabled on both master and 5.0.x branches.
  • We need to decide if we want to reintroduce this fully and what the cache key impl needs to be.
  • tromp had a question about utxhohashset (fast sync state?) validation and where/how caching is involved
• "rewind bad block" should not be required on master as it was simply a hot fix to get past the "bad fork"
• We do want to propagate the InvalidBlockProof error (to ban peers correctly)
  • the whole error/bool/ban logic needs a rethink and cleanup as its hard to reason about currently
• Related [5.0.x] rewind bad headers, ban on explicit bad headers #3603 (on 5.0.x branch) "rewind headers and ban"
  • "rewind headers" also a hot fix and not required on master
• but we do need to handle the "good header, bad block" scenario robustly somehow and this needs impl on master
  • one option is explicit list of "bad headers" as we have in that PR but ideally this is dynamic

---

Tasks broken out as separate issues -

• tracking issue - enable rangeproof verifier cache #3606 "enable verifier cache (fix cache key impl)"
  • took a different approach and removed it entirely Remove verifier cache entirely #3628
• tracking issue - good header, bad block #3607 "good header, bad block"
• handle is_bad_data correctly for various block errors #3627 error/bool/ban logic cleanup

[trevyn]

Question: I understand that there is a lot of work that will go into fully resolving this in the way that we would like. Given that the network has settled a bit, what is the minimal set that would allow an "acceptable" release from master now? Is the already-applied removal of verifier caches enough, or do we also strictly need improved banning behavior and "good header, bad block" detection?

[tromp]

This time we only needed to rewind less than a day back, but it would be good to prepare for a possible future need to rewind beyond the 1 week horizon. Perhaps that can rely on the existence of block archive nodes. Since this entails quite a bit more work, it should probably be delayed for a much later release. But I just wanted to leave a note here...

[antiochp]

Closing this issue - all PRs are merged and prepped for an initial 5.1.0.