Improving Altcoin support

[NicolasDorier]

So @Kukks came with an idea which seem a viable path for increasing altcoin support in NBitcoin.

Current situation

Current altcoin support is possible only if the transaction, block, blockheader, and way of calculating the hashes are similar to bitcoin. (like Litecoin, Viacoin, or BCash) This is done by building a Network through the NetworkBuilder class. One can specify a way to calculate the PoWHash algorithm as well so it is possible to check PoW on altcoins not using SHA256 as their PoW algorithm.

We want to increase altcoin support for those who does not have the exactly same transaction format or block format. (Stratis for example)

Proposed change

In order to not break any existing code, @Kukks proposed a solution which might work and involve minimal work.

For block header for example, the Network class would have a BlockHeaderFactory instance, responsible for parsing a Block Header from bytes. Non-Bitcoin block header would be subclass of block header, overriding GetHash, GetPowHash and ReadWrite. A block header instance would then know exactly, without any outside information:

• How to calculate its hash,
• How to calculate its PoW Hash,
• How to serialize itself,

Consequence

It means that in order to correctly deserialize a blockheader we would need to have a Network/BlockHeaderFactory instance in the scope. And luckily, this should be already the case. (Node class is an example)

Place deserializing a block header and not having Network/BlockHeaderFactory in the scope should assume Bitcoin format and marked as issues in github for later refactoring.

Should be made obsolete

The GetPoWHash of BlockHeader currently takes a Network parameter. This is not needed anymore as the BlockHeader would get the logic for GetPowHash calculation inside itself. We can just keep the current GetPowHash(Network) but mark it [Obsolete]. Then having the real GetPowHash() without Network parameter.

~BitcoinStream is deserializing Block/BlockHeader/Transaction without Network information. A constructor of BitcoinStream should be added with Network as parameter, and the current constructor should be marked [Obsolete].~ (Not needed because BlockHeader/Block/Transaction knows how to deserialize themselves...)

Note

We would use the following plan for BlockHeader/BlockHeaderFactory, Block/BlockFactory, Transaction/TransactionFactory.

I hope we can get one separate PR for each of them, starting by BlockHeader/BlockHeaderFactory.

I hope we can do that as mechanically as possible, marking the place where we deserialize without Network in the scope, then making different for solving each of those cases.

Once again, the most important thing is to not break people who only care about Bitcoin, so "blank sheet" designs are not possible.

@Kukks @knocte @dangershony @bokobza @Aprogiena @nopara73

[nopara73]

Concept ACK. I think it's a great solution. Altcoiners may need to go for the extra mile to understand it, but it only minimally messes with the Bitcoin user space (IMO GetPowHash and BitcoinStream are not often used), which is the most important.

[Aprogiena]

We did have a similar discussion in Stratis, but I wasn't involved in that that much. I think @dangershony can talk more about how we solved that. We certainly started the transition, not sure if we finished it at this point. And not sure which solution we went for at the end.

[Kukks]

You polished up the idea really well @NicolasDorier 👍 Perhaps we can leave the GetPowHash there marked as Obsolete in case it breaks for other devs? @Aprogiena @dangershony directed me to this PR

[NicolasDorier]

yes, I think except one or two cleanup in the Consensus class that almost nobody use it should be fine.

[dangershony]

A constructor of BitcoinStream should be added with Network as parameter, and the current constructor should be marked [Obsolete].

This sounds very good, I already started such a PR but got distracted.

A BlockHeaderFactory also sounds like a good start and later we might add more factories for Block and Transaction

Currently in our code we use static flags to determine serialization but this has to go away, we have a flag in Network that specifies if this is a PoS network so this covers cases for serialization (for example we with block we need to add a block signature and in transaction the PoS timestamp)

But ideally for the stratis platform we need to be able to also use different Block/Header/Transaction types as in the future some of our altcoins might add more data to a trx (or not use a UTXO style trx all) but we cross this bridge when we get there.

[NicolasDorier]

@dangershony actually I am thinking: BitcoinStream does not have to know the Network. The reason is that under the model of subclassing BlockHeader/Transaction/Block, those instances knows how to deserialize themselves through ReadWrite so BitcoinStream does not need to know.

[NicolasDorier]

So actually I think the plan might be quite easy: Really the only thing those factory need is a .CreateBlockHeader() method. We then only have to deprecate the use of new BlockHeader() and make them use Network.BlockHeaderFactory.CreateNewBlockHeader().

Deserialization is always handled by a subsequent header.ReadWrite() so there is no need of additional method in the factories.

I am quite excited, this might be very minimal change.

[Kukks]

Will give this a go this Monday. :)

[dangershony]

@NicolasDorier you are right but having the Network in BitcoinStream may still be very useful for all kinds of scenarios, I would still try to get it in there if we can. what you suggested will work without breaking anything.

[NicolasDorier]

@dangershony this is too difficult I think. Methods like GetSerializedSize, GetVirtualSize does not expect you pass the Network to them, and should not be expecting this either.

[NicolasDorier]

@Kukks try to keep GetPowHash if you can, else dump it. It might break stuff but this is not a very used piece of code and was not well designed.

[Kukks]

@NicolasDorier

I can mark the GetPoWHash in Network as Obsolete to raise compile warnings on it. I can't see how leaving the GetPoWHash setter would work though.

Are there any usecases of anyone using the GetPoWHash other than in the BlockHeader?

[NicolasDorier]

Nah, it should be in the block header. That's why maybe removing it is better.

[Kukks]

@NicolasDorier

The GetPoWHash of BlockHeader currently takes a Network parameter. This is not needed anymore as the BlockHeader would get the logic for GetPowHash calculation inside itself.
We can just keep the current GetPowHash(Network) but mark it [Obsolete].
Then having the real GetPowHash() without Network parameter.

I couldn't find a signature for GetPowHash(Network) in BlockHeader or anywhere else in the project? I will be getting rid of the GetPowHash in Network as it seems messy to try and handle that config when the block header provided from a factory should be handling it.

[NicolasDorier]

@Kukks yes GetPowHash() should be in header, it was not until now.

[Kukks]

@NicolasDorier I see there has been a number of changes pushed. It seems all the types needed to be covered for STRAT (@dangershony ) are covered by the ConsensusFactory.

[Kukks]

Maybe the Altcoins integrated should be in their own nuget and not directly in NBitcoin?

[NicolasDorier]

Yes. I worked quite a lot today on this. Now NBitcoin has NBitcoin.TestFramework and NBitcoin.Altcoins in this repo. Currently implemented is Litecoin, BCH, Feathercoin. I would like to try with an alt having different shape. @dangershony

I updated NBXplorer and BTCPay to use this new version.

There is a guide to integrate and do some basic testing on the altcoin.

[NicolasDorier]

Closing this. Tested with coin with different PoW (Dash), with different block format (Doge) and with different address format (BCash).

It is working smoothly!

Thanks for the idea @Kukks this refactoring is a big success for shitcoin integration.