Restructure proposer and isolate dependencies #137

scravy · 2018-10-01T17:22:50Z

So far this is mostly refactorings, adding adapters, etc. but one can see where this is going.

The bigger picture: This is part of implementing PoSv3 (the Proof-of-Stake protocol we are building on) as in particl in unit-e. Some of the code in here was taken from particl, but as you can see from comparing for example the pos/miner in particl with the proposer/proposer in unit-e they deviated quite a lot so far.

What is what?

staking/ involves functions which are required for supporting PoS. I moved it into it's own folder and namespace as these are part of the libserver compilation target. They need to be available also when building without wallet support (background: For staking/proposing or staking/validation we need a wallet and you can actually compile bitcoin/unit-e without the wallet). This is since they are used for validation, something every node performs. Eventually they might need to be moved to consensus/ or something like that, but for now I am keeping them in their own staking/ space.

Edit: Since validation is not wired yet, staking translation units are actually tied to the libwallet compilation target for now.

proposer/ contains only the components which are actually used for proposing. They are part of the libwallet compilation target. Every node with wallet support is a proposer by default (can be disabled). I am introducing a lot of interfaces here which I designed according to the same design idea as in #153.

esperanza/ – which is where all of this used to live before – is where the finalization gadget lives. As per the same rationale as in #153 I moved things out of there.

Dependency Injection

I have been struggling a lot elsewhere (#88) with bitcoins chronic untestability (when it comes to unit tests, the functional test framework is great). A lot of this is because things are very tightly coupled, singletons, global variables, state, circular dependencies, etc. I ended up starting to isolate components from each other, much like discussed in #153. For example you can see in this pull that the proposer_tests do not require any of the testing setup as the dependencies are completely mocked. This is what I think is a proper unit test – one that tests a unit in isolation.

Thus every component names its dependencies in it's factory function (debatable, but apparently okay with everyone as per #153). It requires a bit of wiring which usually a DI framework could take care of, but I am not going to include something like this in the project. So, manual.

For mocking I pulled in https://github.com/eranpeer/FakeIt – a boost compatible, header only, library; and it only affect the unit tests. See proposer_tests for a small usage example (I'm not using it much yet). -> #171

scravy · 2018-10-21T21:19:21Z

src/injector.h

+  COMPONENT(Proposer, proposer::Proposer, proposer::Proposer::MakeProposer,
+            proposer::Settings, proposer::MultiWallet, proposer::Network,
+            proposer::ChainState, proposer::BlockProposer)
+};


This is how a complete Injector looks like. I am only using this one in libwallet and for the proposer and its dependencies only. In the future I'd like to integrate other components with it.

Looks great to be honest.

scravy · 2018-10-21T21:21:22Z

src/Makefile.test.include

@@ -44,9 +44,9 @@ UNITE_TESTS =\
  test/checkqueue_tests.cpp \
  test/coins_tests.cpp \
  test/compress_tests.cpp \
-  test/counting_semaphore_tests.cpp \


I moved the counting semaphore (and the tests along with it) to libwallet to prevent linking issues (it's not used outside that compilation target). If it's used anywhere else we can move it then.

scravy

@kostyantyn @Ruteri @AM5800 @frolosofsky @Nizametdinov @Gnappuraz @cornelius

So, I considered the feedback about moving, pointers, etc. and decided to get rid of them. In my original description I said that I would not pull in a dependency injection library/framework – and I didn't. I looked at a few, but they are either too big, not available for C++, or I find them too intrusive (either way I am not keen on adding dependencies anyway). So I rolled up my sleeves and created a tiny injector utility myself.

Look at the dependency_injector_tests.cpp to see how it's used.

Compared to traditional DI libraries this one is less sophisticated. In the language of Google Guava, it supports only "eager singletons" (which is all I care for anyway). Also it might strike you as a little odd that the dependencies have to be supplied in the injector too (i.e. you define a component as COMPONENT(Name, Type, FactoryFunction, Dep1, Dep2, ...)). Ideally it would be something like NAME = bind(Type, OtherType/Provider/FactoryFunction) and bind would then reflect OtherType for dependencies. This... is hard to do in C++11, so I went for the option I went for (and I am reasonably content with that, it's still all statically checked etc.).

Having this component delivers you from the burden of:

thinking about initialization order (it computes the dependency graph and initialized in the right order accordingly)
dealing with the lifecycle of components (the injector creates and deletes components; the components are destructed in reverse order of initialization)

The dependencies are passed to components as type Dependency<Type> (as opposed to std::shared_ptr<Type>). A Dependency is really just a pointer (since lifecycle is managed by the injector).

To have the design of the injector not intrude your class design too much there is no requirement on augmenting any class with an INJECT macro or extend any library supplied type (or anything the like). All which is required is a static factory function that returns an std::unique_ptr<Type>. The injector knows how to deal with these functions, will release() the unique_ptr and take ownership.

Let me know what you think!

Ruteri

utACK

Ruteri · 2018-10-22T08:32:43Z

src/injector.h

+  COMPONENT(Proposer, proposer::Proposer, proposer::Proposer::MakeProposer,
+            proposer::Settings, proposer::MultiWallet, proposer::Network,
+            proposer::ChainState, proposer::BlockProposer)
+};


Looks great to be honest.

Ruteri

utACK
DI looks great.

Nizametdinov

utACK d62b54e. I find Injector awesome and that's cool that we're moving toward a testable architecture

Nizametdinov · 2018-10-22T09:00:58Z

src/injector.h

+  COMPONENT(ChainState, proposer::ChainState, proposer::ChainState::MakeChain)
+
+  COMPONENT(MultiWallet, proposer::MultiWallet,
+            proposer::MultiWallet::MakeMultiWallet);


Nit: redundant semicolon

Nizametdinov

utACK d62b54e. I find Injector awesome and that's cool that we are moving toward a testable architecture

scravy · 2018-10-22T11:03:33Z

@Nizametdinov I will take care of this superfluous trailing semicolon in a following PR if you don't mind ;-)

scravy · 2018-10-22T11:03:46Z

@Nizametdinov I will take care of this superfluous trailing semicolon in a following PR if you don't mind ;-)

Extracted from #137 This includes the header-only [fakeit](https://github.com/eranpeer/FakeIt) library into the unit test target (so no dependencies added to `united` etc.). `fakeit` can mock interfaces (anything which `std::is_polymorphic` yields true for ;-)). Here are usage examples extracted from their README: ```cpp class SomeInterface { virtual int foo(int) = 0; virtual int bar(string) = 0; }; ``` ```cpp // Instantiate a mock object. Mock<SomeInterface> mock; // Setup mock behavior. When(Method(mock,foo)).Return(1); // Method mock.foo will return 1 once. // Fetch the mock instance. SomeInterface &i = mock.get(); // Will print "1". cout << i.foo(0); ``` Verify method invocation ```cpp Mock<SomeInterface> mock; When(Method(mock,foo)).Return(0); SomeInterface &i = mock.get(); // Production code i.foo(1); // Verify method mock.foo was invoked. Verify(Method(mock,foo)); // Verify method mock.foo was invoked with specific arguments. Verify(Method(mock,foo).Using(1)); ```

scravy added the wip Work in progress which is not supposed to be merged yet label Oct 1, 2018

scravy self-assigned this Oct 1, 2018

scravy added this to the Port PoS v3 milestone Oct 1, 2018

scravy force-pushed the proposer-logic branch 4 times, most recently from 9362bb5 to 51d058c Compare October 4, 2018 15:25

scravy added the proposing label Oct 4, 2018

scravy force-pushed the proposer-logic branch 14 times, most recently from 450abb7 to 2c18172 Compare October 12, 2018 17:58

scravy force-pushed the proposer-logic branch 8 times, most recently from f18e4fd to 75ae52b Compare October 15, 2018 15:13

scravy force-pushed the proposer-logic branch 8 times, most recently from 9072357 to 33b231b Compare October 21, 2018 11:00

Restructure proposer, isolate components

4d2368e

scravy force-pushed the proposer-logic branch from 33b231b to 4d2368e Compare October 21, 2018 13:58

add injector test

30549ac

scravy force-pushed the proposer-logic branch from 4c62f37 to b168ad6 Compare October 21, 2018 20:52

inject proposer

64710c4

scravy force-pushed the proposer-logic branch from b168ad6 to 64710c4 Compare October 21, 2018 21:03

Initialize proposer using Injector

d62b54e

scravy commented Oct 21, 2018

View reviewed changes

scravy changed the title ~~WIP: Proposer logic~~ Restructure proposer and isolate dependencies Oct 21, 2018

scravy removed the wip Work in progress which is not supposed to be merged yet label Oct 21, 2018

Ruteri approved these changes Oct 22, 2018

View reviewed changes

Nizametdinov approved these changes Oct 22, 2018

View reviewed changes

scravy merged commit d02f452 into master Oct 22, 2018

scravy deleted the proposer-logic branch October 25, 2018 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure proposer and isolate dependencies #137

Restructure proposer and isolate dependencies #137

scravy commented Oct 1, 2018 •

edited

Loading

scravy Oct 21, 2018

Ruteri Oct 22, 2018

scravy Oct 21, 2018

scravy left a comment

Ruteri left a comment

Ruteri Oct 22, 2018

Ruteri left a comment

Nizametdinov left a comment

Nizametdinov Oct 22, 2018

Nizametdinov left a comment

scravy commented Oct 22, 2018

scravy commented Oct 22, 2018

Restructure proposer and isolate dependencies #137

Restructure proposer and isolate dependencies #137

Conversation

scravy commented Oct 1, 2018 • edited Loading

scravy Oct 21, 2018

Choose a reason for hiding this comment

Ruteri Oct 22, 2018

Choose a reason for hiding this comment

scravy Oct 21, 2018

Choose a reason for hiding this comment

scravy left a comment

Choose a reason for hiding this comment

Ruteri left a comment

Choose a reason for hiding this comment

Ruteri Oct 22, 2018

Choose a reason for hiding this comment

Ruteri left a comment

Choose a reason for hiding this comment

Nizametdinov left a comment

Choose a reason for hiding this comment

Nizametdinov Oct 22, 2018

Choose a reason for hiding this comment

Nizametdinov left a comment

Choose a reason for hiding this comment

scravy commented Oct 22, 2018

scravy commented Oct 22, 2018

scravy commented Oct 1, 2018 •

edited

Loading