INDY-2262: Implementing NodeRegHandler

[ashcherbakov]

Implemented Items 1, 2 and 4 from INDY-2262 PoA:

• Implement NodeRegHandler to store uncommitted and committed node regs, as well as nod regs for previous view
• Store nodeReg in audit ledger
• Added missing tests to be run by CI
• some random fixes and improvements

[lgtm-com]

This pull request introduces 2 alerts and fixes 1 when merging 8371cad into 2043fa6 - view on LGTM.com

new alerts:

• 1 for Conflicting attributes in base classes
• 1 for Comparison of identical values

fixed alerts:

• 1 for NotImplemented is not an Exception

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging 22e7b4a into 2043fa6 - view on LGTM.com

new alerts:

• 1 for Conflicting attributes in base classes

fixed alerts:

• 1 for NotImplemented is not an Exception

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging b28a8a9 into 2043fa6 - view on LGTM.com

new alerts:

• 1 for Conflicting attributes in base classes

fixed alerts:

• 1 for NotImplemented is not an Exception

[skhoroshavin]

This looks good overall. Things I would consider changing in future PRs are:

• intention behind on_catchup_finished hook looks sensible, however name is a bit misleading IMHO. I'd consider naming it like initialize, reset or sync_from_ledgers (or maybe there is better name)
• we implement same encoding algorithm for different fields in audit ledger again and again, probably it's time to move it into separate set of helper functions?
• now that node reg is stored in audit ledger - can we remove primaries from audit ledger?

[skhoroshavin]

Probably it makes sense to move check last_audit_node_reg < self.ledger.uncommitted_size inside this brach and throw LogicError if it doesn't hold

[skhoroshavin]

It seems like this code relies on uncommitted_node_reg always being initialized. Is it always the case?

[ashcherbakov]

Yes. It's called every time a NODE txn is being applied.

[ashcherbakov]

Thanks for review!

* intention behind `on_catchup_finished` hook looks sensible, however name is a bit misleading IMHO. I'd consider naming it like `initialize`, `reset` or `sync_from_ledgers` (or maybe there is better name)

I agree that the current name is not perfect. However, I believe that our general name convention in BatchHandlers is a callback on some system events (on batch created, on batch reverted, etc.), so on catchup finished looks fine, and it makes clear that this is not an abstract initialization, but the one that needs to be done after catchup is finished.

* we implement same encoding algorithm for different fields in audit ledger again and again, probably it's time to move it into separate set of helper functions?

Fully agree, And there is a TODO comment for this (as well as for other improvements I would like to have). The PR is huge even without these changes, so I suggest to do it in one of the next PRs.

* now that node reg is stored in audit ledger - can we remove primaries from audit ledger?

In theory yes, but I'm not sure that we must do it.
We should defenetly remove FuturePrimariesBatch Handler (this will be done in the next PR)
As for removing Priomaries, I'm not so sure because:

• This is the only easy way to jump to the previous view in audit ledger (and we do use this in NodeRegHandler)
• We may have another algorithm for primaries selection besides round robin
• It's quite cheap to store them as we use deltas