[LEDGER] Move UpdatesBytesBuilder to txmgr pkg

[cendhu]

Type of change

• Improvement (improvement to code)

Description

Currently, we create the bytes representation of UpdateBatch to compute the hash of UpdateBatch which would be used to compute the block commit hash along with the validation results. The utility for converting an UpdateBatch to deterministic bytes is present in the privacyenabledstate pkg, i.e., DeterministicBytesForPubAndHashUpdates(). We have made the following changes to this utility.

1. This utility is used only by the txmgr and hence, we move this utility function to txmgr pkg from privacyenabledstate pkg to keep the utility closer to the consumer.
2. We have removed the UpdateBytesBuilder struct and unexport functions as they are not needed any longer.
3. We have defined proto messages such as KVWriteProto and KVWriteBatchProto to construct bytes from the UpdateBatch. As we shouldn't append the keyword proto to messages, we renamed KVWriteProto to KVWrite and KVWriteBatchProto to Updates.
4. The function name such as buildForKeys(), buildForColls() are not very explicit in what they do. When the buildForColls() calls buildForKeys(), it adds even more confusion as the first-level function, i.e., deterministicBytesForPubAndHashUpdates() is also calling buildForKeys(). Hence, we have used the following function names instead:
   • genKVsFromNsUpdates()
   • genKVsFromCollsUpdates()
   • genKVs()

Related issues

FAB-17830

[manish-sethi]

LGTM except one comment on the final commit

The function names such as getKVsFromNsUpdates give a false impression of what theses functions are doing. You use typically getXXX when the result is already available in the desired form (e.g., accessing an internal field).

The important characteristics of these functions is that they are building/generating the proto messages. May be you may want to consider generateKVProtosFromNsUpdates or anything similar. For shortening the name, perhaps genKVsFromNsUpdates can be used.

[cendhu]

I agree @manish-sethi. I also felt that get reads like reading it from the DB as we have used this prefix majoritily for DB access. I thought of construct but the name became long. I think the name suggested by you makes sense. I have changed the function names to genKVsXXX.

[manish-sethi]

LGTM from code change point of view. Though, the code changes are mainly related to refactoring, still, it touches a critical code for commit-hash computation. Unfortunately, we lack a test that verifies that the behavior is still the same as before. May be this is a good time to add a test in kvledger/tests - wherein, the stored ledger in the testdata folder can be used as a reference and one of the reset/rollback tests can be enhanced to verify the newly computed commit-hash - wdyt?

[cendhu]

We have UT to ensure that the behavior of deterministicBytesForPubAndHashUpdates() is unchanged. Further, TestAddCommitHash() in kv_ledger_test.go also adds more robustness.

I understood the importance of ledger-level integration tests using testdata to ensure we are not changing the way the hash is computed. I would prefer to push a separate PR. If it must be done in this PR itself, I am fine with that too.

[manish-sethi]

Yes, there is a safety check at the UT level. I am fine for the higher level test in a separate PR. TestAddCommitHash() does not serve that purpose though.

[cendhu]

Thanks for merging this @manish-sethi Next, I will work on the commit hash test and push a PR -- https://jira.hyperledger.org/browse/FAB-17832