…che::BatchWrite()
In case the child is FRESH and the parent is non-DIRTY, that is its state has been communicated to the grandparent (if exists), the FRESH status can be copied to the parent.
Note: I may be in over my head on this one; in case this patch is wrong because of my gap of understanding, please excuse me! :)
In case the child is FRESH and the parent is non-DIRTY, that is its state has been communicated to the grandparent (if exists), the FRESH status can be copied to the parent.
IMO commit message and PR title are too long.
I believe this is exactly right.
It only applies in the case where both a grandparent and a parent cache don't have an entry, and a new entry is created in the child cache. When flushing to the parent, it can be marked as fresh.
Why is more important than how in commit messages. When would a fresh entry in a child be flushed into the parent?
@gmaxwell Do you mean to ask where are all the possible flows in the code in which this change would actually effect something? If so, I don't know. It's a general fix to an over-restriction in the coins DB itself - I don't know how small/large its actual optimization impact would be.
Or, perhaps I misunderstood your question.
Right, I understand what this is doing (with a bit of sipa's help at least); but I can't come up with any case in the code where it would actually happen. I think it's the right thing to do, but I'm at a loss as to how to test it or prioritize it in my review.
I can wildly guess this may happen in case of a reorg, in case a previously spent output which was already flushed becomes unspent. I might be wrong as I haven't looked at reorg handling yet. That said, even if this currently doesn't happen in a reorg, future changes to its implementation could trigger this case, since logically this is definitely something that may happen as a result of a reorg.
That's all the extra input I can offer for now, I am sure @sipa and others would be more qualified to answer :)
On Tue, Sep 19, 2017 at 10:16 AM, Gregory Maxwell notifications@github.com wrote:
Right, I understand what this is doing (with a bit of sipa's help at least); but I can't come up with any case in the code where it would actually happen. I think it's the right thing to do, but I'm at a loss as to how to test it or prioritize it in my review.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/11354#issuecomment-330451580, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFJFaHZZypmBMUZZDaavmlpyaRjHhyFks5sj2plgaJpZM4PZ5zD .
I don't think it's currently possible to trigger this logic, because pruned/spent coins aren't loaded from parent caches (GetCoins() can only return true if an unspent coin is found), and we currently erase things from the cache when we flush to a parent (so when a coin is spent and DIRTY, it loses DIRTY-ness at the same time it is erased). @danra I agree that it's possible in the future this could be an optimization (eg in a future where we might write to the parent without clearing the cache), but given that this logic is consensus critical and already super complicated, I think we should just put a comment in that explains this a bit more, and not make any actual code changes -- optimizing consensus code in a way that we don't believe can trigger seems like way too much risk for zero reward.
Closing this, as the consensus is that the risk is too high considering the potential benefit.
