[LEDGER] merge ledgerstorage with kvledger pkg

[cendhu]

Type of change

• Improvement (improvement to code)

Description

The ledgerstorage package encapsulates both pvtdata storage and block storage. We needed this package as

• we had to have 2-phase commit between pvtdata storage and block storage.
• we had a recovery logic that was needed after a commit failure or peer crash between pvtdata commit and blockstore commit.
• we treated pvtdata store as a pvt blockstore and blockstore as a public block store.

After introducing the support for peer rollback, we removed the 2-phase commit between the pvtdata storage and block storage as we had to allow the pvtdata storage to have block height higher than the blockstore. Hence, the recovery logic was also removed. Thus, the motivation for having ledgerstorage package became feeble and hence, we remove it.

Further, core/ledger, core/ledger/ledgerstore, core/ledger/ledgermgmt create confusion and provide disinformation. At least in this PR, we remove core/ledger/ledgerstore.

Additional details

As it is not justified to add production code to avoid data race while using a debug log, we remove the debug log.

Related issues

FAB-17670

[cendhu]

@manish-sethi I am getting two data races

one between

• a debug message at lockbased_txmgr (where we invoke multiple listeners after a block commit (and)
• the purge pvtdata goroutine at pvtdatastore

another between

• a debug message at lockbased_txmgr (where we invoke multiple listeners after a block commit (and)
• collElgListener at pvtdatastore

When I remove the debug message, obiviously, I don't get any data races.

While the second data race at least make sense, the first one looks surpising to me. As there are too many moving parts, I need your help to identify the root cause. Let's pair for this when you get time.

[yacovm]

Senthil, this happens because the logger tries to print all elements of collElgNotifier which has a map full of pvtdatastorage.store objects and the purgerLock inside is unlocked in that other goroutine which involves an atomic operation increment. Since the logger prints the inner field using reflection, no locking/atomic operations are being used and a data race occurs.

The following piece of code trivially stops the logger from using reflection.

diff --git a/core/ledger/kvledger/coll_elg_notifier.go b/core/ledger/kvledger/coll_elg_notifier.go
index 8ce001e31..ed13e58e1 100644
--- a/core/ledger/kvledger/coll_elg_notifier.go
+++ b/core/ledger/kvledger/coll_elg_notifier.go
@@ -20,6 +20,10 @@ type collElgNotifier struct {
        listeners                     map[string]collElgListener
 }
 
+func (n *collElgNotifier) String() string {
+       return "foo"
+}
+
 func (n *collElgNotifier) Initialize(ledgerID string, qe ledger.SimpleQueryExecutor) error {
        // Noop
        return nil

[cendhu]

Thank you, @yacovm You just taught me how to debug data races especially when it occurs in debug messages. I didn't have this thought process earlier. I appreciate the help.

[cendhu]

As it is not justified to add production code to avoid data race while using a debug log, we remove the debug log.

[yacovm]

As it is not justified to add production code to avoid data race while using a debug log, we remove the debug log.

Wait, what?? why is it not justified? Adding a stringer is perfectly fine!

[cendhu]

My opinion is that it is not justifiable. Similarly, I am not supportive of writing interfaces in the production code so that mock can created.

If @manish-sethi also feels that it is justifiable to change the production code for a debug log, I will do it. Or he might find some other bug in my code :-)

[manish-sethi]

As it is not justified to add production code to avoid data race while using a debug log, we remove the debug log.

Wait, what?? why is it not justified? Adding a stringer is perfectly fine!

Yes, I agree that adding a stringer is good in this case.
Ideally, we should have added a function Name() or ID() in the listener interface (or take a name input while registering the listener). So, it is forced. But, as an easy fix, Stringer seems fine.

[ryjones]

please squash to one commit.

[cendhu]

@yacovm @manish-sethi as both of you feel that it is completely fine, I keep my philosophy aside.

I will go with adding Name() to the interface as it looks clean.

[cendhu]

@ryjones sure. Will do.

[cendhu]

Added 20 lines of production code and 64 lines to mock code to retain a debug log 😦

[manish-sethi]

It mimics peer crash -> this confused me for a moment. Though, it is correct from the point of view of the data not getting written to statedb but, nothing of that sort of thing is getting tested here. The tests that are moved here still have the flavor of testing of commits between the two store only. After some time, we may forget the scope of these tests and I guess that it would make sense to either have them in a separate test file or renaming the tests to makes the scope clear.

[cendhu]

I removed the mimics peer crash.

TestPvtStoreAheadOfBlockStore() ensures that the isPvtStoreAheadOfBlockStore is set correctly. It can be set due to two scenarios: The block is written to the pvtdatastore and the peer fails before committing the block to blockstore. When the peer restarts, it should have isPvtStoreAheadOfBlockStore as true. Another scenario is related to rollback. Long ago we decided to have rollback related tests in the kvledger/tests. I am not understanding the problem with this test. If I miss something, please let me know.

[manish-sethi]

What I meant in my comment is the following....
Earlier, these tests were in a separate package and the scope of that package was clear, i.e., maintaining consistency between ledger storage and pvt data store (exactly, what you have described above). So, a meaning of a crash mimic is clear once you are in that package.
Now, we chose to move that code here in the kvledger package. The kvldegder package deals with much more coordination between many components. mimic crash could mean that "is this test mimicking a crash after writing to both blockstore and pvtdata store, but short of committing to statedb?".
That's what I meant that "TestPvtStoreAheadOfBlockStore" name does not convey what is the state of other stores and hence I made the comment that - "The tests that are moved here still have the flavor of testing of commits between the two store only. After some time, we may forget the scope of these tests and I guess that it would make sense to either have them in a separate test file or renaming the tests to makes the scope clear." But this is not something that should hold this PR from merging. This can be fixed later in a separate PR as well.