Unclear update of the code that loads checkpoints about concord-bft

[andrewjstone]

CC @yuliasherman

[yuliasherman]

We have already an open bug for this issue: SkvbcPersistenceTest fails intermittently (in CI & locally) #294. Looks like we have some incorrect behavior that requires a deep investigation.

[guyg7]

Okay. I will look at #294 .
But, for now, I suggest to rollback to the correct version of this line.

[yuliasherman]

Did you observe the second assert being thrown? Because we did not somehow.

[guyg7]

This may be another bug....
Let me look at #294 first

[guyg7]

Okay. I believe that #294 arises because two Checkpoint messages are written in the same transaction (to the same sequence number), and only the first message is actually written to the disk/database, the second message is not written.
From what I can see, it is because of a bug in DBMetadataStorage: in DBMetadataStorage, the write operations are collected by using unordered_map::insert. Notice that unordered_map::insert does not add element <k,v> if the map already contains key k.

[guyg7]

I guess it would be simple to fix DBMetadataStorage :-)

[yuliasherman]

Hm. I remember I've already fixed this bug in the past. The entry in the map needs to be deleted and then a new one inserted. But now I don't see the fix. Thanks for your attention!