stratisproject/stratisbitcoinfullnode

Bitcoin full node in C#

StratisBitcoinFullNode

This repository is now archived.

The StratisFullNode repository is now maintained with a focus on Stratis Blockchain Technology development.

stratisbitcoinfullnode's Issues

[Builder] Base components refactoring

The builder currently has the node base code under its folder.
The Builder and Base need to be separated.

1. A new folder (suggested name NodeCore/NodeBase) It has the minimal components that are delivered with the node.

• Chain of headers (chain behavior, repository)
• Connection
• Signals
• NodeDeployments
• Logs
• DataFolder
• Settings
• LifeTime

2. Consider changing the name of FullNode (its not always a fullnode) to Node or LocalNode

3. Rename Builder folder to NodeBuilder

Issue Labes Typo improvment -> improvement

https://github.com/stratisproject/StratisBitcoinFullNode/labels/ - improvment -> improvement

LookaheadBlockPuller NextBlockCore race condition

Race condition (R1) and potential race condition (R2) exist in LookaheadBlockPuller:

StratisBitcoinFullNode/Stratis.Bitcoin/BlockPulling/LookaheadBlockPuller.cs

Line 406 in fd06b73

if (!IsDownloading(header.HashBlock))

R1: As soon as IsDownloading returns false here, another thread can change the state (consider BlockPullerBehavior.Node_MessageReceived, just before the newly downloaded block is pushed, the above check for DownloadedBlocks will fail - block not pushed yet - but then IsDownloading here returns false), which will cause this thread to call AskBlocks for a block that was already downloaded.

R2: Another potential race condition here is unclear: Is there also a possibility that this happens when the block is being downloaded? I.e. is this the only thread that can exist in the node and lead to execution of DistributeDownload? If there can be another thread executing DistributeDownload, the map can be updated after IsDownloading returns false, which would lead to a single block being downloaded twice.

R2 is not clear because NextBlockCore is called from NextBlock, which is called from BlockNotification.Notify, which is run in its own async loop, but the NextBlock is also called from ConsensusLoop.ExecuteNextBlock, called from ConsensusLoop.Execute, called from ConsensusFeature.RunLoop, which runs in its own thread. So it seems like there are two different threads that can end with calling NextBlock, but it is unclear whether there is not some mechanism to prevent the concurrency.

The problem in both cases is the same - IsDownloading allows you to get state of shared resource, but once IsDownloading returns, the state is no longer guaranteed and you are working with potentially outdated information. It would be possible to use such an information, if later the code guaranteed that the unwanted behavior (downloading block that has been downloaded already, or is being downloaded) could not occur. This is not the case, however.

Integrate SonarSource

This can be very useful to help with automated code review and code qialuty.

https://github.com/SonarSource/sonar-csharp
https://www.sonarsource.com/why-us/products/codeanalyzers/sonarcsharp.html

Any takers let me know I'll create a task.

Failing integration unit tests - cannot access the file '_DBreezeSchema'

I'm running Windows 7 and running the integration unit tests using the VS2017 test runner. The following unit tests fail:

BlockStoreCanRecoverOnStartup()
WalletCanRecoverOnStartup()

Both fail with the following error: Message: System.IO.IOException : The process cannot access the file '_DBreezeSchema' because it is being used by another process

[BlockStore] Split BlockStoreLoop

Break the BlockStoreLoop.DownloadAndStoreBlocks() is a very big method we should refactor to smaller methods.
Proposal:
Create a new class IBlockStoreLoopSteps with the following methods TryReorg(), CheckExists(), TryPending(), TryDownload() each method returns an out param that decide how to act

A call to each method will look maybe like this

if(this.steps.TryReorg(next, out ret))
{
      if(ret.Delay) 
          break;
      continue;
}

Write tests for the various scenarios

LookaheadBlockPuller.MedianDownloadCount race condition

ConsensusStats.Log calls LookaheadBlockPuller.MedianDownloadCount.get, which checks whether downloadedCounts contains any items and if so, it calls LookaheadBlockPuller.GetMedian, which raises exception if the list is empty. Race condition occurs when LookaheadBlockPuller.CalculateLookahead is called, which calls downloadedCounts.Clear, just after the ConsensusStats.Log thread checked that the downloadedCounts is not empty.

Feature request: Several validation modes

Several mode for validation:

• Managed Validation (current one, consensus reimplemented in C#)
• LibConsensus Validation (partially managed, script executed by libconsensus library of Bitcoin Core)
• No Validation (No check on blocks are done, safe only if connected to a whitelisted node)

The last would allows to be behind a bitcoin core proxy, while still being able to take advantage of a full UTXO and easier to develop features of Stratis full node.

Migrate logging to NLog

There is a lot of complex code in the project and sometimes it is hard to analyze problems. This can be very much solved with detailed logging system. I have many years of experience with NLog and it offers some great features that fit the bill that we are unsure how to do in Serilog that we currently use.

Thus I'd like to suggest migration to NLog for logging, which will allow me to install detailed logs in very sensitive and complex code - like puller, coin views, memory pool, block store, etc.

One of the objections for block puller is that there are no tests to cover the complex code of this part of the code. However, it is very difficult to creates tests after the code is written and deployed for a significant amount of time. Even if we create a large number of tests for block puller, we will certainly not cover all possibilities and code paths that can occur. This is where logging is so powerful.

All we need is to install detailed logs in these complex parts and when there is something weird, we just need to analyze the log. This assumes that developers will be testing their code mostly with detailed logging enabled even for the parts that are currently not working with. We can achieve this by using configuration file for NLog that will not make it into the production release and the logging of the release would be normally directed by command line options and node config file.

Fix failing unit tests

There are a few unit tests that are failing intermittently.
We need to fix them in order to get a clearer view of the sanity of our build and the code in general.

Windows:
RunWithDelayRunsTaskUntilCancelled
NotifyWithoutSyncFromRunsWithoutBroadcastingBlocks
StartLogsStartAndStop

Linux:
GetTimeOffsetReturnsCurrentUtcTimeOffset
StartWithDelayUsesIntervalForDelayRunsTaskUntilCancelled

OS X:
GetTimeOffsetReturnsCurrentUtcTimeOffset
RunWithDelayRunsTaskUntilCancelled
SpendableAmountConfirmedOnlyGivenBeingConfirmedAndSpentUnconfirmedReturnsAmount

In the attached file there is a bit more explanation on the failures.
unittests.txt

Discussion: Consensus Validation Rules

An idea I think can be useful for the consensus code is to break many of the validation logic inside the consensus validator class in to ValidationMacros (or similar) to have smaller snippets of code.

Then on startup a node will load all its consensus validation macros to a container and when a block is received each block will pass in all the macros in order to be considered valid.
A Context will be passed in to every Macro for Macros that need previously calculated data

The benefits:

• Readability - a macro will be named based on its logic (i.e CoinBaseMaturityMacro)
• Testability - it will be much easier to write tests for each macro
• ReUsability - POW/POS/PO..X consensus rules will be able to reuse macros
• ChainSpecific - currently POS has hard coded checks for protocol version, that logic is Stratis specific
• Optimization - some macros will be able to run in parallel

I don't think there will be any performance degradation on iterating over several tens of macros.
I am also not sure Macro is the right name.

Wallet history ignores the change address

As Agree the wallet will only show history of change if its spent (not incoming)
@bokobza

Reorgs causing memory leaks in block puller

LookaheadBlockPuller.PushBlock is called when the block is being delivered in BPB.Node_MessageReceived. Now consider the start of the method:

            uint256 hash = block.Header.GetHash();
            ChainedBlock header = this.Chain.GetBlock(hash);
            while (this.currentSize + length >= this.MaxBufferedSize && header.Height != this.location.Height + 1)

If we reorged to different chain before the block came, can't we get null header here? So do we rely here on the exception to be consumed by OnMessageReceived try/catch block around handler.DynamicInvoke(this, m);? This is probably OK, although not sure if intentional. But what if reorg happened later, but before this new block is consumed. It seems like it will be added to downloadedBlocks and never consumed. Looks like memory leak and if blocks are full, this means 1MB of memory gone forever, and eventually, if the node is run for a long time, it could sum up.

Fix compiler warnings.

There are couple of warnings to be seen during the build process and it should be quite easy to get rid of them:

4>------ Rebuild All started: Project: Stratis.Bitcoin, Configuration: Debug Any CPU ------
4>Features\MemoryPool\MempoolValidator.cs(554,4,554,6): warning CS0162: Unreachable code detected
4>Features\MemoryPool\MempoolOrphans.cs(192,11,192,22): warning CS0219: The variable 'nFetchFlags' is assigned but its value is never used
4>Features\Consensus\ConsensusStats.cs(15,31,15,36): warning CS0169: The field 'ConsensusStats.stack' is never used
4>Features\RPC\RPCRouteHandler.cs(20,38,20,54): warning CS0169: The field 'RPCRouteHandler.contollerMapping' is never used
4>Features\Miner\PosMinting.cs(48,25,48,38): warning CS0169: The field 'PosMinting.hashPrevBlock' is never used

5>------ Rebuild All started: Project: Stratis.Bitcoin.IntegrationTests, Configuration: Debug Any CPU ------
5>UtilTests.cs(39,6,39,48): warning CS4014: Because this call is not awaited, execution of the current method continues before the call is completed. Consider applying the 'await' operator to the result of the call.
5>MemoryPoolTests.cs(946,14,946,16): warning CS0649: Field 'TestMemPoolEntryHelper.lp' is never assigned to, and will always have its default value null
5>BlockStoreTests.cs(20,15,20,35): warning xUnit1013: Public method 'BlockRepositoryBench' on test class 'BlockStoreTests' should be marked as a Fact.
5>MinerTests.cs(567,15,567,42): warning xUnit1013: Public method 'MinerCreateNewBlockValidity' on test class 'MinerTests' should be marked as a Fact.

[Refactor] Split DownloadBlockStep

The logic contained in DownloadBlockStep in the BlockStore needs to be split into two logical areas for better unit testing.

Create a FindBlocks and DownloadBlocks "sub" step and unit test each separately.

Consider changing to use .NET configuration extensions

.NET Core already has built in support for INI files (which is essentially what bitcoin.conf is), in Microsoft.Extensions.Configuration. The following code will load and parse the existing bitcoin.conf without any custom code:

public static void Main(string[] args)
{
    // command line args use format: --testnet=1
    var configBuilder = new ConfigurationBuilder()
        .AddIniFile("bitcoin.conf", true)
        .AddCommandLine(args);
    var config = configBuilder.Build();
    Console.WriteLine("testnet = '{0}'", config["testnet"]);
    Console.WriteLine("server = '{0}'", config["server"]);
    Console.ReadLine();
}

Note that there is also a standard command line args parser, which can be configured to override the INI file, although the format is slightly different than the original bitcoind, e.g. you must use the format "--testnet=1" or "--testnet 1" (it also supports single character aliases, e.g. "-c=1" or "-t 1".

One of the benefits of the .NET Core configuration system is it support sections and hierarchical content.

e.g. in the INI file:

# comments
testnet=1
; also valid for comments
[rpc]
server=1
rpcuser=test

Which in the code can be accessed as config.GetSection("rpc")["server"], or as config["rpc:server"].

To override from the command line, use arg with format "--rcp:server=1" or "--rpc:server 1"

There is also Microsoft.Extensions.Options, which will map configurations into data classes for you, and can be used with dependency injection, e.g.

public class RpcOptions {
    public string Server { get; set; }
    public string RpcUser { get; set; }
}

public class Rpc {
    public Rpc(IOptions<RpcOptions> options) {
        // services can be configured to create and inject options from the config        
    }

Note that fully embracing .NET configuration will change the command line format, although you could also write your own parser.

There does also still need to be bootstrap handling at startup to determine the location of any configuration file, e.g. could be from a combination of --datadir, --conf, --testnet, including whether --testnet is overridden in the config file.

Miner Refactor - On BlockAssembler

Instead of an assembler that uses inheritance we could create a BlockBuilder
Then to construct a block and find a valid POW/POS block we could have extensions
Perhaps similar to this:

var block = new BlockBuilder()
       .UseNetwork(this.network)
       .CreateBlock()
       .AddTransactions(IMempool)
       .Mine()
       .Validate()
       .Build()

var block = new BlockBuilder()
       .UseNetwork(this.network)
       .CreateBlock()
       .AddTransactions(IMempool)
       .Stake()
       .Validate()
       .Build()

Increase status update rate for Bench[0]

After running the SBFN in linux's terminal, I would like to suggest an enhancement to increase the refresh rate of the Bench[0] output to the terminal to 2 seconds or in real-time.

Store.Height stuck during IBD

With some runs of full node now Store.Height being stuck at a certain value (random) while Headers.Height and Consensus.Height are increasing.

Might be related to #266

ConnectionManager.ConnectedNodes needs protection

Allowing ConnectionManager Behavior to directly modify ConnectionManager.ConnectedNodes has a really bad smell to it to me.

I don't think anything should be modifying the list of nodes without the knowledge of ConnectionManager, and hooking the Added/Removed event within ConnectionManager seems dirty too.

https://github.com/stratisproject/StratisBitcoinFullNode/blob/master/Stratis.Bitcoin/Connection/ConnectionManagerBehavior.cs#L66

			if(node.State == NodeState.HandShaked)
			{
				this.ConnectionManager.ConnectedNodes.Add(node);
				Logs.ConnectionManager.LogInformation("Node " + node.RemoteSocketEndpoint + " connected (" + (this.Inbound ? "inbound" : "outbound") + "), agent " + node.PeerVersion.UserAgent + ", height " + node.PeerVersion.StartHeight);
				node.SendMessageAsync(new SendHeadersPayload());
			}

Recommend making ConnectionManager.ConnectedNodes return IEnumerable, or creating a new interface in NBitcoin/NStratis which permits a readonly collection.

Cleaning old rewind data

Rewind data need to be cleaned. At block 330000, my hard drive took 20GB of space when the UTXO was only 2GB.

2 observations:

• We do not need rewind data if we are synching old blocks. (what about not saving them at all?)
• We do not need more than 1 days worth of block as it is unlikely there is such big reorg.

If we need to rewind past the last rewind data, the UTXO is completely wiped out, and reperform a full indexing. (we can save time here by disabling checks on block that were validated previously)

Running node with invalid parameters/settings cause crash

dotnet run -datadir=xyz

if xyz does not exist, the application crashes. This does not sound very nice towards the user.

In general I think the user should never see exception and app crash even if it is their fault. Similar thing happens when you use -port and the port is already used by another application. The app crashes, and the user sees a lot of information they are probably not interested in, instead of "this port is in use" or "this directory does not exist" etc.

Daemon crashes if first ran as root, then as regular user on Linux

You may get the following exception if you first dotnet run the daemon as root, but then later run it as a regular user:
**Unhandled Exception: System.NullReferenceException: Object reference not set to an instance of an object.
at Stratis.Bitcoin.Consensus.DBreezeCoinView.**b__3_0() in /home/auxon0/sources/StratisBitcoinFullNode/Stratis.Bitcoin/Consensus/CoinViews/DBreezeCoinView.cs:line 32
at Stratis.Bitcoin.DBreezeSingleThreadSession.<>c__DisplayClass10_0.b__0() in /home/auxon0/sources/StratisBitcoinFullNode/Stratis.Bitcoin/DBreezeSingleThreadSession.cs:line 127
at System.Threading.Tasks.Task.Execute()
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
at Stratis.Bitcoin.Consensus.DBreezeCoinView..ctor(Network network, String folder) in /home/auxon0/sources/StratisBitcoinFullNode/Stratis.Bitcoin/Consensus/CoinViews/DBreezeCoinView.cs:line 25
at Stratis.Bitcoin.Builder.FullNodeBuilderExtensions.<>c__DisplayClass2_0.b__0(IServiceCollection service) in /home/auxon0/sources/StratisBitcoinFullNode/Stratis.Bitcoin/Builder/FullNodeBuilderExtensions.cs:line 48
at Stratis.Bitcoin.Builder.FullNodeBuilder.BuildServices() in /home/auxon0/sources/StratisBitcoinFullNode/Stratis.Bitcoin/Builder/FullNodeBuilder.cs:line 153
at Stratis.Bitcoin.Builder.FullNodeBuilder.Build() in /home/auxon0/sources/StratisBitcoinFullNode/Stratis.Bitcoin/Builder/FullNodeBuilder.cs:line 126
at Stratis.BitcoinD.Program.Main(String[] args) in /home/auxon0/sources/StratisBitcoinFullNode/Stratis.BitcoinD/Program.cs:line 24

To workaround this issue, a user may simply use "sudo dotnet run" to launch the daemon.

A better error message explaining that access denied is recommended for a fix, and a documentation update explaining the issue and workaround.

BlockPuller bug: Node_MessageReceived

Node_MessageReceived threw this exception:

Locals:

Console:

Please let me know if there's any more information I can provide to help troubleshoot this.

Potential race condition in BlockPuller AskBlocks->GetNodeBehaviors->DistributeDownload

I'm not sure about this one, but will add it anyway. So if you can argue why this can not happen, please do.

BlockPuller.AskBlocks calls BlockPuller.GetNodeBehaviors, which selects candidate nodes to assign work to. It seems to me that it can happen that another thread will call Node.Cleanup when the peer node disconnects. In that case the Nodes collection that BlockPuller.GetNodeBehaviors works with might be unupdated until BlockPullerBehavior.DetachCore is called, so BlockPuller.AskBlocks can get a list of nodes including the one that just disconnected.

Then let's say BlockPuller.AskBlocks calls BlockPuller.DistributeDownload and just before it modifies BlockPuller.map, the BlockPullerBehavior.DetachCore can be called in the other thread, which would release all the work assigned to this freshly disconnected node. Then BlockPuller.DistributeDownload would continue and modify the map.

This would end with a work assigned to a no longer existing node, which would lead to puller stalling once the assigned block would be requested.

Mempool bug:

This bug found a few times (after running for about 24 hours or if mempool is really full)

fail: Mempool[0]
      System.ArgumentException: An item with the same key has already been added. Key: e492cffb55da94e2e4f5ecb29cb47a47365a1572867f8db523e7e323c315d52f
         at System.ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException(Object key)
         at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
         at Stratis.Bitcoin.Consensus.UnspentOutputSet.SetCoins(FetchCoinsResponse coins) in C:\Users\Dan\Documents\GitHub\StratisBitcoinFullNode\Stratis.Bitcoin\Consensus\UnspentOutputSet.cs:line 52
         at Stratis.Bitcoin.MemoryPool.MempoolCoinView.<LoadView>d__10.MoveNext() in C:\Users\Dan\Documents\GitHub\StratisBitcoinFullNode\Stratis.Bitcoin\MemoryPool\MemPoolCoinView.cs:line 41
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Stratis.Bitcoin.MemoryPool.MempoolValidator.<AcceptToMemoryPoolWorker>d__27.MoveNext() in C:\Users\Dan\Documents\GitHub\StratisBitcoinFullNode\Stratis.Bitcoin\MemoryPool\MempoolValidator.cs:line 107
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Stratis.Bitcoin.MemoryPool.MempoolValidator.<AcceptToMemoryPoolWithTime>d__25.MoveNext() in C:\Users\Dan\Documents\GitHub\StratisBitcoinFullNode\Stratis.Bitcoin\MemoryPool\MempoolValidator.cs:line 71
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Stratis.Bitcoin.MemoryPool.MempoolBehavior.<ProcessTxPayloadAsync>d__24.MoveNext() in C:\Users\Dan\Documents\GitHub\StratisBitcoinFullNode\Stratis.Bitcoin\MemoryPool\MempoolBehavior.cs:line 184
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Stratis.Bitcoin.MemoryPool.MempoolBehavior.<AttachedNode_MessageReceived>d__20.MoveNext() in C:\Users\Dan\Documents\GitHub\StratisBitcoinFullNode\Stratis.Bitcoin\MemoryPool\MempoolBehavior.cs:line 70

Unhandled Exception: System.ArgumentException: An item with the same key has already been added. Key: e492cffb55da94e2e4f5ecb29cb47a47365a1572867f8db523e7e323c315d52f
   at System.ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException(Object key)
   at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
   at Stratis.Bitcoin.Consensus.UnspentOutputSet.SetCoins(FetchCoinsResponse coins) in C:\Users\Dan\Documents\GitHub\StratisBitcoinFullNode\Stratis.Bitcoin\Consensus\UnspentOutputSet.cs:line 52
   at Stratis.Bitcoin.MemoryPool.MempoolCoinView.<LoadView>d__10.MoveNext() in C:\Users\Dan\Documents\GitHub\StratisBitcoinFullNode\Stratis.Bitcoin\MemoryPool\MemPoolCoinView.cs:line 41
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Stratis.Bitcoin.MemoryPool.MempoolValidator.<AcceptToMemoryPoolWorker>d__27.MoveNext() in C:\Users\Dan\Documents\GitHub\StratisBitcoinFullNode\Stratis.Bitcoin\MemoryPool\MempoolValidator.cs:line 107
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Stratis.Bitcoin.MemoryPool.MempoolValidator.<AcceptToMemoryPoolWithTime>d__25.MoveNext() in C:\Users\Dan\Documents\GitHub\StratisBitcoinFullNode\Stratis.Bitcoin\MemoryPool\MempoolValidator.cs:line 71
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Stratis.Bitcoin.MemoryPool.MempoolBehavior.<ProcessTxPayloadAsync>d__24.MoveNext() in C:\Users\Dan\Documents\GitHub\StratisBitcoinFullNode\Stratis.Bitcoin\MemoryPool\MempoolBehavior.cs:line 184
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Stratis.Bitcoin.MemoryPool.MempoolBehavior.<AttachedNode_MessageReceived>d__20.MoveNext() in C:\Users\Dan\Documents\GitHub\StratisBitcoinFullNode\Stratis.Bitcoin\MemoryPool\MempoolBehavior.cs:line 86
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.QueueUserWorkItemCallbackDefaultContext.System.Threading.IThreadPoolWorkItem.ExecuteWorkItem()
   at System.Threading.ThreadPoolWorkQueue.Dispatch()

Persist mempool to disk: Flatfile?

Bitcoin Core uses flat file for memory pool persistence.

Should we do the same, or should we put it in DBreeze?

If flat, should we make it a core compatible file, or use our own format?

Rework how Stratis.Bitcoin.Signals are used

Consider removing ISignals.Blocks and ISignals.Transactions from the interface and make them private in Signals. This will require Subscribe method to be implemented.

Reason: All code is currently using something like this:
this.signals.Blocks.Broadcast(block.Block);

instead of

this.signals.Signal(block.Block);

and both [Blocks|Transactions].Broadcast implement guards and then .Signal() methods also implement guards. This is probably not used as intended.

Feature: Add the assumevalid flag

bitcoin/bitcoin@e440ac7

DNS entries not picking the DNS server

When a test net node starts first time (no address.dat file) the node doesnt connect to the dns seed node
This might be related to the AddressBehaviour/AddressManager in NStratis

BlockPullerBehavior.Node_MessageReceived is calling blocking PushBlock

This is probably not intentional as it seems that we could still ask for more blocks to arrive and they will but we will block in PushBlock until the consumer consumes them because there are limits on how many bytes we want to use by the puller. However, we will still use more and more RAM as new blocks arrive, they will just not go to the puller. So the limits in the puller won't achieve their original goal - limit RAM consumption.

If we really want to limit amount of RAM consumed, we would need to stop asking, rather than blocking processing (of already received) blocks.

So this issue should be resolved by not asking for more if we are full and probably completely removing the blocking logic in PushBlock.

Longest chain timeout

When a peer that connected with a longer chain got disconnected before our node downloaded the blocks we need to disconnect and reset the longest chain to one of the connected peers

Code style policy changes

The purpose if this issue is to open discussion about changing coding style - https://github.com/stratisproject/StratisBitcoinFullNode/blob/master/Documentation/coding-style.md

Currently we only ask contributors to follow the coding style, yet we do not have any rules on whether or not a code can be accepted if it violates the coding style.

The current code base uses different code styles and a lot of code violates many rules of the current style.

I think we should define both - the rules we want (as the current rule set is not sufficient) as well as the policy about how this is or is not enforced - i.e. which rules are required and which are only recommended.

This issue is for StratisBitcoinFullNode project only at this moment, and once we converge to something here, other projects are welcome to follow the outcome as well.

Besides the rules and the policies on their enforcement, we should also define the decision process of which rules will be applied should contributors disagree on some (i.e. who is the boss if any, are there any vetos, or is this just majority voting?).

PROPOSING RULE (NEW OR CHANGE): If you want to propose a new rule, or change the text of the current rule, please start your comment starting with RULE PROPOSAL and use three sections: DEFINITION - in this first part just write the rule as you would like to see it; RATIONALE - try to explain why you think this is a good idea to have; POLICY - explain what kind of policy you would like to have on this rule (i.e. would you like it to have only as a general guideline/recommendation, or strictly enforced - i.e. not accepting new code violating it?).

NodeBuilder: AddRPC() requires UseWallet()

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())

LookaheadBlockPuller - QualityScore function is too penalizing

Added a bunch of logs in my local source code, it revealed that the block puller wanted to get a block, the work was assigned to a peer that had 75 quality score. The node provided the block in about 2 seconds, but during this time it lost 40 % of its quality score because of "stalling". Then by finishing the task, it gained +1, so it went 75 -> 45 -> 46. The peer ended with much worse score than it started, while it did a pretty good job.

Basically what happens with the current score function is that any peer that is assigned work that is not currently needed can gain score, while to be assigned with the task to download currently needed block (say it was assigned to a peer that disconnected and then reassigned to other peer) always ends with dramatic lowering of the score. In case the peer is assigned the current and couple of following blocks, it will almost surely be thrown away (stalling -> score down -> score zero -> all tasks released from the node).

It seems like this downloading strategy does not really reflect how good nodes are.

CustomThreadPoolScheduler finished and WaitFinished are broken but unused - remove them

StratisBitcoinFullNode/Stratis.Bitcoin/Utilities/CustomThreadPoolScheduler.cs

It seems that this event is used in a broken way. First, it does not signal by default, which probably means
that unless a first task is scheduled, it can't signal. Second, it is set in do method if after executing the task there are no more tasks to execute or being executed at the moment. However, assume a case in which no one waits on this event. This means that after first task completes, no one resets the event and even if there later are many tasks being executed or in the queue, this event would still be set until someone would consume it, which would immediately cause the consumer to think that there is no more work to be done.

Moreover, there seem to be no synchronization over the shared resource among multiple threads, so race conditions are possible.

As no one is using WaitFinished anyway, I suggest to remove both, this event and the method using it
from the code.

Command line argument -listen in Stratis node behaves like -bind in Bitcoin Core

Stratis node uses -listen for what Bitcoin Core uses -bind. Bitcoin Core also defines -listen, but uses it for different thing.

This is probably not an intention and the parameters should be compatible.

Discussion: How to move NStraits code to the fullnode

Ideally we would want to remove the dependency on NStratis.

• Either we go for a big-bang and refactor
• Cheery pick classes and move over only what we need.

At the moment we decided to go big-bang after the coming release

Prune mode

I was doing some test with the fullnode and now its taking 13 gig + on my hd. So, i decided to set prune=2000 to lower it down but, it didnt delete anything like with bitcoin core wallet. Should it do that? Here is the release note for the file pruning with bitcoin :

https://github.com/bitcoin/bitcoin/blob/v0.11.0/doc/release-notes.md#block-file-pruning

Like Dan pointed, prune mode is on when the value is different from 0 :
nodeArgs.Prune = config.GetOrDefault("prune", 0) != 0;

BlockPuller: Collection Modified bug

Happened right after startup. Will investigate further.

info: Stratis.Bitcoin.FullNode[0]
      PeriodicLog starting
info: Stratis.Bitcoin.Connection[0]
      Node [::ffff:217.119.149.253]:8333 connected (outbound), agent /Satoshi:0.13.2/, height 463506
crit: Stratis.Bitcoin.Consensus[0]
      Consensus loop unhandled exception (Tip:463504)
System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
   at System.ThrowHelper.ThrowInvalidOperationException(ExceptionResource resource)
   at System.Collections.Generic.List`1.Enumerator.MoveNextRare()
   at Stratis.Bitcoin.BlockPulling.BlockPuller.AssignPendingVectors() in D:\Dev\StratisBitcoinFullNode\Stratis.Bitcoin\BlockPulling\BlockPuller.cs:line 268
   at Stratis.Bitcoin.BlockPulling.BlockPuller.OnStalling(ChainedBlock chainedBlock) in D:\Dev\StratisBitcoinFullNode\Stratis.Bitcoin\BlockPulling\BlockPuller.cs:line 306
   at Stratis.Bitcoin.BlockPulling.LookaheadBlockPuller.NextBlockCore(CancellationToken cancellationToken) in D:\Dev\StratisBitcoinFullNode\Stratis.Bitcoin\BlockPulling\LookaheadBlockPuller.cs:line 299
   at Stratis.Bitcoin.BlockPulling.LookaheadBlockPuller.NextBlock(CancellationToken cancellationToken) in D:\Dev\StratisBitcoinFullNode\Stratis.Bitcoin\BlockPulling\LookaheadBlockPuller.cs:line 125
   at Stratis.Bitcoin.Consensus.ConsensusLoop.ExecuteNextBlock(CancellationToken cancellationToken) in D:\Dev\StratisBitcoinFullNode\Stratis.Bitcoin\Consensus\ConsensusLoop.cs:line 145
   at Stratis.Bitcoin.Consensus.ConsensusLoop.<Execute>d__21.MoveNext() in D:\Dev\StratisBitcoinFullNode\Stratis.Bitcoin\Consensus\ConsensusLoop.cs:line 121
   at Stratis.Bitcoin.Consensus.ConsensusFeature.RunLoop() in D:\Dev\StratisBitcoinFullNode\Stratis.Bitcoin\Consensus\ConsensusFeature.cs:line 106

Unhandled Exception: System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
   at System.ThrowHelper.ThrowInvalidOperationException(ExceptionResource resource)
   at System.Collections.Generic.List`1.Enumerator.MoveNextRare()
   at Stratis.Bitcoin.BlockPulling.BlockPuller.AssignPendingVectors() in D:\Dev\StratisBitcoinFullNode\Stratis.Bitcoin\BlockPulling\BlockPuller.cs:line 268
   at Stratis.Bitcoin.BlockPulling.BlockPuller.OnStalling(ChainedBlock chainedBlock) in D:\Dev\StratisBitcoinFullNode\Stratis.Bitcoin\BlockPulling\BlockPuller.cs:line 306
   at Stratis.Bitcoin.BlockPulling.LookaheadBlockPuller.NextBlockCore(CancellationToken cancellationToken) in D:\Dev\StratisBitcoinFullNode\Stratis.Bitcoin\BlockPulling\LookaheadBlockPuller.cs:line 299
   at Stratis.Bitcoin.BlockPulling.LookaheadBlockPuller.NextBlock(CancellationToken cancellationToken) in D:\Dev\StratisBitcoinFullNode\Stratis.Bitcoin\BlockPulling\LookaheadBlockPuller.cs:line 125
   at Stratis.Bitcoin.Consensus.ConsensusLoop.ExecuteNextBlock(CancellationToken cancellationToken) in D:\Dev\StratisBitcoinFullNode\Stratis.Bitcoin\Consensus\ConsensusLoop.cs:line 145
   at Stratis.Bitcoin.Consensus.ConsensusLoop.<Execute>d__21.MoveNext() in D:\Dev\StratisBitcoinFullNode\Stratis.Bitcoin\Consensus\ConsensusLoop.cs:line 121
   at Stratis.Bitcoin.Consensus.ConsensusFeature.RunLoop() in D:\Dev\StratisBitcoinFullNode\Stratis.Bitcoin\Consensus\ConsensusFeature.cs:line 165
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)

Program Version Identifier

This came up while implementing GetInfo.

Bitcoin Core "getinfo" RPC command returns a program version and a network version, as so:

{
    "version" : 89900,
    "protocolversion" : 70002,
...

Or more modern:

{
  "version": 1010000,
  "protocolversion": 70012,
...

Nbitcoin and NStratis use a custom string in handshake:
"/NBitcoin:" + version.Major + "." + version.MajorRevision + "." + version.Build + "/"

RPC getinfo result requires a uint for version number.
I see two options, either return a "compatibility version number" from getinfo, which would cause major maintainability issues I think, or convert the assembly version number to such a version uint.

I have suggested creating the following extension method for System.Version which will convert the assembly version into a uint for use in getinfo, and elsewhere as needed.

return (uint)(version.Major * 1000000u + version.Minor * 10000u + version.Build * 100u + version.Revision);
This will result in a getinfo like this:

{
"version": 1000104,
"protocolversion": 70012,

I haven't dug to see where else this uint version number might be used, comments on that are desired.

Project cannot load in VS2015 when VS2017 is installed

In order for developers to open and build this solution in VS2015 when VS2017 is installed, a global.json file must be added to the project which specifies the core version to use.

According to:
http://stackoverflow.com/questions/40674393/visual-studio-2017-install-breaks-visual-studio-2015-asp-net-core-projects

Is there any plan to upgrade this project to VS2017 in the near future?

Stratis Full Node

Hi guys

I have some azure credits and wanted to run a full Stratis node now that its playing with the big boys. What do I need to do? Just compile the master and run?

Thanks,

What is the state of the mempool development?

Which branch is the more advanced? master or MemPool?

Synching super slow if prune = 0

BlockStoreLoop.DownloadBlocks threw an unhandled exception

I freshly merged master and I randomly got this:

BlockStoreLoop.DownloadBlocks threw an unhandled exception
Exception: System.NullReferenceException: Object reference not set to an instance of an object. 
  at Stratis.Bitcoin.Features.BlockStore.LoopSteps.DownloadBlockStep.d__6.MoveNext() in StratisBitcoinFullNode\Stratis.Bitcoin\Features\BlockStore\LoopSteps\DownloadBlockStep.cs:line 106 
--- End of stack trace from previous location where exception was thrown --- 
  at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() 
  at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) 
  at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult() 
  at Stratis.Bitcoin.Features.BlockStore.LoopSteps.DownloadBlockStep.d__4.MoveNext() in StratisBitcoinFullNode\Stratis.Bitcoin\Features\BlockStore\LoopSteps\DownloadBlockStep.cs:line 66 
--- End of stack trace from previous location where exception was thrown --- 
  at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() 
  at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) 
  at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult() 
  at Stratis.Bitcoin.Features.BlockStore.LoopSteps.BlockStoreStepChain.d__2.MoveNext() in StratisBitcoinFullNode\Stratis.Bitcoin\Features\BlockStore\LoopSteps\BlockStoreLoopStep.cs:line 24 
--- End of stack trace from previous location where exception was thrown --- 
  at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() 
  at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) 
  at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult() 
  at Stratis.Bitcoin.Features.BlockStore.BlockStoreLoop.d__35.MoveNext() in StratisBitcoinFullNode\Stratis.Bitcoin\Features\BlockStore\BlockStoreLoop.cs:line 178 
--- End of stack trace from previous location where exception was thrown --- 
  at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() 
  at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) 
  at System.Runtime.CompilerServices.TaskAwaiter.GetResult() 
  at Stratis.Bitcoin.Features.BlockStore.BlockStoreLoop.<b__34_0>d.MoveNext() in StratisBitcoinFullNode\Stratis.Bitcoin\Features\BlockStore\BlockStoreLoop.cs:line 151 
--- End of stack trace from previous location where exception was thrown --- 
  at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() 
  at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) 
  at System.Runtime.CompilerServices.ConfiguredTaskAwaitable.ConfiguredTaskAwaiter.GetResult() 
  at Stratis.Bitcoin.Utilities.AsyncLoop.<>c__DisplayClass8_0.<b__0>d.MoveNext() in StratisBitcoinFullNode\Stratis.Bitcoin\Utilities\AsyncLoop.cs:line 119

Possibly related to #268

RPC behavior and folder structure

If it is intended that the RPC responses be compliant with Bitcoin Core, serializable Model based formatters for the output structure can be created.

Project structure:
Currently a folder Controllers is in the Project root. As these controllers will only be used by the RPC feature, can we move the Controllers into the RPC folder? This would allow us to group RPC response formatters under the RPC folder as well.

Blockpuller DistributeDownload bug

aprogiena [3:43 PM]
ok then

[3:44]
so BlockPuller.cs in Stratis.Bitcoin.BlockPulling

[3:44]
DistributeDownload method

[3:44]
int minHeight parameter

[3:44]
it is called from AskBlocks as
DistributeDownload(vectors, nodes, downloadRequests.Min(d => d.Height));

[3:45]
in downloadRequests i believe we have list of blocks that we want to download and we do have d.Height for each of them - i.e. their height in blockchain

[3:46]
so we have a bunch of blocks we want to download and we have a bunch of nodes we can download from and this DistributeDownload method is about to distribute these download tasks among those nodes

[3:46]
now we have a (already updated) comment in DistributeDownload:
// Be careful not to ask the block from a node that does not have it

[3:46]
enforced as
if (behavior.ChainBehavior?.PendingTip?.Height >= minHeight)
selectnodes.Add(behavior);

[3:47]
i.e. for each of the node we check whether we know how long their vision of the blockchain is and only if it is long enough (>= minHeight) we add the node among candidates to receive download task (selectnodes)

[3:48]
if so far so good, here comes my confusion and i hope i did not mess that up otherwise i'll look like an idiot 🙂

dangershony [3:49 PM]
This is all correct yes (hard to look like an idiot this is not so strait forward) (edited)

aprogiena [3:49 PM]
consider we have peer nodes A, B, C, D with their vision of the reality (length of the chains they have) as follows:
A = 15
B = 16
C = 16
D = 11

[3:50]
now assume we have last block no. 9 and so we want blocks 10, 11, 12, 13, 14, 15, 16 (edited)

[3:51]
this would mean that
DistributeDownload(vectors, nodes, downloadRequests.Min(d => d.Height));
would translate as
DistributeDownload(vectors, nodes, 10);

[3:51]
that would make the filtering at the start of DistributeDownload not filter out any of the nodes

[3:52]
so even node D would remain in the selection

[3:52]
what i'm confused about is that after that filtering, there is no minHeight or d.Height used at all

[3:52]
which looks like D could be asked for blocks 12, 13, 14, 15, 16

[3:52]
even if it does not have them

[3:53]
which contradicts the comment // Be careful not to ask the block from a node that does not have it

dangershony [3:53 PM]
Well spotted bug.....!!!

[3:53]
It might explain why download stalls sometimes

aprogiena [3:53 PM]
i.e. i believe there should be Max() not Min()

[3:54]
i.e. DistributeDownload(vectors, nodes, 16);

[3:54]
or completely done in different manner

dangershony [3:54 PM]
It should be both I guess

zeptin [3:54 PM]
hmm, i'm not familiar with that part of the code, but in that case D would be a candidate for at least downloading 10 and 11, right? you might be unnecessarily restricting the pool of available nodes by being too pessimistic

aprogiena [3:54 PM]
or checking for node height even after that filtering (i.e. in index selection)

[3:54]
or whatever

[3:55]
@zeptin exactly, optimally you would like not to disqualify D from giving you 10 and 11

[3:55]
but if we just naively replaced Min with Max, that would do it

[3:55]
so i don't thinkg just Min->Max is the right solution

[3:55]
although it is the easiest one

zeptin [3:56 PM]
i assume that A through D are nodes that have already been contacted that we know the definite height for

aprogiena [3:56 PM]
actually they don't need to be (maybe) because there is a check
if (behavior.ChainBehavior?.PendingTip?.Height >= minHeight)

dangershony [3:56 PM]
Consider this
A = 15
B = 16
C = 16
D = 11
E = 4
F = 9 (us)

aprogiena [3:56 PM]
so i assume those guys with ? can be null if you don't know their state of reality (edited)

dangershony [3:57 PM]
So we need to filter out E and D so you need min and max height

[3:58]
This is your to fix @Aprogiena

zeptin [3:58 PM]
i assume kicking slow nodes happens later once you've actually connected to them and started downloading

[3:58]
because it is not necessarily true that the nodes with the longest chain are the fastest peers

aprogiena [3:58 PM]
@dangershony yeah, you would like to filter out those that can't really help you at all, and among others you want to download assign tasks with respect to their chain length

[3:59]
@dangershony well 😕 tx 😄

[3:59]
@zeptin there is quality score that is respected when distributing those tasks; this is kind of experience with the node

[4:00]
so nodes with greater quality score are asked more than others

dangershony [4:00 PM]
You found it you fix it 😂 and you also get 2 honour points 🙏

Question: Writing Tests

I am not sure if this is better here or in slack. Please feel free to move if required.

I am looking to write some tests as a way to learn more about the codebase.

I think starting with tests around "configuration" would be simple enough to let me focus on tests while still learning a little...

So, before I write a line of code....here is the question and I don't want to start a war. :) There seems to be a mix of var and explicit type.

I see that using var is a warning in the editorconfig....but it seems to be used pretty liberally....any change we could remove the var warning from editorconfig? or maybe even swap it for require var?

#ThisIsNotJava 🙏

Need a license file

What license is StratisBitcoinFullNode under?

NBitcoin uses the MIT License, but that is very permissive, so Stratis could use anything.

See as it is publicly available on GitHub, there are already a bunch of forks, but it would be good to clarify what license is intended.

Personally, these days I prefer copyleft licenses, which force derivatives to remain open. With a Microsoft background, I tend to use Ms-RL, although something like the GPL or LGPL also work.