NodeBuilder: AddRPC() requires UseWallet()

[mikedennis]

If you don't include UseWallet() then the Stratis node crashes on startup. Crash is due to a dependency on WalletManager (which isn't there if you don't include UseWallet())

[mikedennis]

Is this a dependency we require or should we load WalletRPCController conditionally depending upon whether UseWallet() was included?

[dangershony]

For now a quick fix is to allow wallet to be null however a better fix is to separate the controllers in to their features (like with api).

[mikedennis]

This is the actual exception being thrown:

System.InvalidOperationException occurred
  HResult=0x80131509
  Message=Unable to resolve service for type 'Stratis.Bitcoin.Features.Wallet.IWalletManager' while attempting to activate 'Stratis.Bitcoin.Features.Miner.PosMinting'.
  Source=<Cannot evaluate the exception source>
  StackTrace:
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.Service.PopulateCallSites(ServiceProvider provider, ISet`1 callSiteChain, ParameterInfo[] parameters, Boolean throwIfCallSiteNotFound)

The exception is thrown when copying the node services to the RPC services in this line

StratisBitcoinFullNode/Stratis.Bitcoin/Features/RPC/RPCFeature.cs

Line 48 in 9aec20b

var obj = this.fullNode.Services.ServiceProvider.GetService(service.ServiceType);

[dangershony]

Unfortunately I cant seem to reproduce this bug.
If I comment out .UseWallet() in NodeBuilder this tests fails only on the RPC call 'SendCommand' (not the node init)

        [Fact]
        public void CanMineBlocks()
        {
            using(NodeBuilder builder = NodeBuilder.Create())
            {
                var stratisNodeSync = builder.CreateStratisNode();
                builder.StartAll();
                var rpc = stratisNodeSync.CreateRPCClient();
                rpc.SendCommand(NBitcoin.RPC.RPCOperations.generate, 10);
                Assert.Equal(10, rpc.GetBlockCount());
            }
        }

[mikedennis]

If you startup the node like this you will encounter the error:

 var node = new FullNodeBuilder()
                .UseNodeSettings(nodeSettings)
                .UseStratisConsensus()
                .UseBlockStore()
                .UseMempool()
                .AddPowPosMining()
                .AddRPC()
                .Build();

Maybe I should create a unit test for it?

[dangershony]

As discussed the solution is to probably check feature dependencies at Feature level.
Suggested is to add to the FeatureExecutor the following

this.Execute(featureCollection, service => service.ValidateDependencies(featureCollection),  service.Start());

Then each feature can verify its dependencies (by feature)

[mikedennis]

@dangershony

Execute() does both starting and stopping the service and the validation is only relevant to starting. Propose we do this instead:

1. Expose FullNode.Features property in IFullNodeBuilder

2. In FullNodeFeatureExecutor.cs

       public void Start()
        {
            try
            {
                IFeatureCollection featureCollection = this.FullNodeBuilder.Features;
                this.Execute(service => service.ValidateDependencies(this.node.Services.ServiceProvider, featureCollection));
                this.Execute(service => service.Start());
            }
            catch (Exception ex)
            {
                this.logger.LogError("An error occurred starting the application", ex);
                throw;
            }
        }

3. ValidateDependencies() would iterate through its service DI dependencies and check that they can be resolved. If there is any dependencies that can't be resolved through GetService() then they will throw an exception which will bubble up to the FeatureExecutor and is logged there.

        public virtual void ValidateDependencies(IServiceProvider services, IFeatureCollection features)
        {
            Guard.NotNull<IFeatureCollection>(features, nameof(features));
          
            var featureRegistration = features.FeatureRegistrations.FirstOrDefault(i => i.FeatureType == this.GetType());
            DependencyEnumerator dependencyEnumerator = new DependencyEnumerator();
            featureRegistration.ConfigureServicesDelegates.ForEach(i => i.Invoke(dependencyEnumerator));

            foreach (var dependencyType in dependencyEnumerator.dependencies)
            {
                services.GetService(dependencyType);
            }
        }

Thoughts?

[mikedennis]

The above somewhat works but have one problem I haven't been able to figure out. IFullNodeBuilder is not registered in DI unless RPC is added. So I can't get to IFullNodeBuilder from FullNodeFeatureExecutor. Any ideas?

[dangershony]

I don't think we need to iterate over services, just let each feature validate its dependencies.

if(!features.FeatureRegistrations.Any(i => i.FeatureType == typeof(WalletFeature))
    throw FeatureDependencyNotFound()

This could even be an extension

features.FeatureRegistrations.EnsureFeature<WalletFeature>()

[mikedennis]

Yes I guess doing it via trying to resolve all the dependent services is probably a bit of overkill. Dependencies between features is likely more the exception than the rule. I'm sure in general the features are designed to be as standalone as possible.

I will look at implementing your solution.

[mikedennis]

@dangershony

I had problems getting access to the FullNodeBuilder.Features from within the FullNodeExecutor so instead I invoke the validation logic at the end of FullNodeBuilder.Build(). This seems to work but I have a couple of unit tests that now break.

This test below breaks because not all the dependent services are available for the BlockStoreFeature (specifically NBitcoin.ConcurrentChain). Should this be refactored to take in mock feature instead of a real feature? Or is this purpose of the test testing that the default configuration can build a node with BlockStoreFeature?

StratisBitcoinFullNode/Stratis.Bitcoin.Tests/Builder/FullNodeBuilderTest.cs

Lines 137 to 164 in d136fb4

	public void BuildConfiguresFullNodeUsingConfiguration()
	{
	var nodeSettings = new NodeSettings();
	nodeSettings.DataDir = "TestData/FullNodeBuilder/BuildConfiguresFullNodeUsingConfiguration";
	this.fullNodeBuilder.ConfigureServices(e =>
	{
	e.AddSingleton(nodeSettings);
	e.AddSingleton(nodeSettings.LoggerFactory);
	e.AddSingleton(nodeSettings.GetNetwork());
	e.AddSingleton<FullNode>();
	});
	this.fullNodeBuilder.ConfigureFeature(e =>
	{
	e.AddFeature<BlockStoreFeature>();
	});
	this.fullNodeBuilder.ConfigureServiceProvider(e =>
	{
	var settings = e.GetService<NodeSettings>();
	settings.Testnet = true;
	});
	var result = this.fullNodeBuilder.Build();
	Assert.NotNull(result);
	}

[dangershony]

Doesn't this work?

	this.node.Services.Features.ToList()

[mikedennis]

Think I finally have a reasonable solution to this. I'll make a PR if you want to take a look. Thanks for all the help!

[mikedennis]

PR has been merged into master