[Consensus] Add state objects to CM and CHT for inspection

[fassadlr]

Acquiring and/or inspecting the state of various ConsensusManager and ChainedHeaderTree properties/fields is extremely limited and makes unit tests basically impossible unless reflection is used,

This PR introduces a ConsenusManagerState and ChainedHeaderTreeState class that can easily be added to, without changing the access modifiers of internal or private fields of CM or CHT.

I don't see the point of using locks around these as they will mostly be used in tests for now. Should we want to use CHT properties outside of CHT we can look at how to lock them. @dangershony thoughts on this?

[fassadlr]

@Neurosploit this can then be used in your PR #2620

[Neurosploit]

Great I'll try to merge when this is in. Is that ok?

[dangershony]

We probably actually want to create copies of each collection no? (or the intention is to get the changes as they are reflected on the tree)

Also these methods should be internal so they will not get abused.

[fassadlr]

@dangershony these methods can't be internal as they sit on the IConsensusManager interface ;) We need public access to them.

I will make copies of the collections though 👍

[dangershony]

@Neurosploit if you agree with my comments you can modify this PR directly.

[Neurosploit]

Thanks @dangershony @fassadlr I'll try it out over the weekend, first i'll make these states internal and then create copies, trying out a test merge with a copy of my branch on the way to see if I run into anything and get back to you.

[fassadlr]

@Neurosploit I've made the changes as suggested by @dangershony 👍

[Neurosploit]

@fassadlr No we can do both. We don't need it to be public yet. We can do that once that need arises. There are however some tests that require me to manipulate the CallbacksByBlocksRequestedHash and ExpectedBlockSizes collections. To fix that in a nice way I need to create an internal consensusmanager constructor that allows me to pass in these collections and have the default public constructor call that. In my testconsensusmanager i can then call the internal constructor and keep track of the collection and manipulate it for my needs and then I can work with the copies of the state in the rest of the tests to verify those internals. That makes all my tests pass with this rework.
@dangershony FYI

[fassadlr]

@Neurosploit We will greatly benefit from having this on the interface as there are looooads of tests that can be amended to rather use this instead of getting values via reflection.

[Neurosploit]

Ok, I've already finished my rework in a testbranch so once this is merged i'll merge my PR with master and reapply my rework to my PR. For me it doesn't really matter if the state methods are internal or public both work for me.

[noescape00]

I strongly disagree with exposing private structures of CHT.

CHT is not thread-safe and it's thread safety is being ensured by locks inside CM, no other component should access CHT directly.

Also such change breaks the architecture.

For tests we agreed to use reflection over changing production code. If such change is needed for something not tests-related we can implement it without exposing internal structures of CHT.

[noescape00]

For unit tests or integration tests you can create TestCHT which will inherit from CHT and have those structures exposed- in that case you won't even need reflection.

[fassadlr]

@noescape00 please note that we are not exposing internal properties of CHT or CM. We are merely creating copies of them.

[noescape00]

@noescape00 please note that we are not exposing internal properties of CHT or CM. We are merely creating copies of them.

That's also exposure, just read-only one (except for chained headers- they are references, not copies).
And very expensive one too if it's ever used on production code.

[fassadlr]

@Neurosploit we have decided not to go this route. We don't want to put the State on the interface as it might be misued. @noescape00 will create a PR where we will be using an overridden ChainedHeaderTree class that exposes the private properties of CHT.

[Neurosploit]

Alright just reference it to my issue and I will have a look again. @fassadlr @noescape00.