search

MetacoSA / NBitcoin

Comprehensive Bitcoin library for the .NET framework.
MIT License
1.87k stars 846 forks source link

Add TestNet4 for Bitcoin #1216

Open turbolay opened 3 months ago

turbolay commented 3 months ago

Fixes #1216

This PR doesn't replace TestNet by TestNet4, it simply adds to Network and Bitcoin the field TestNet4. Most interesting file to look at is Network.cs

I also replicated some of the tests we have for TestNet to TestNet4

I used @jlopp 's value for network magic.

I however don't know what to use as vSeeds:

https://github.com/MetacoSA/NBitcoin/blob/828b7d9ffbc6255b06c40cdf7e46d68d6bd04ca9/NBitcoin/Network.cs#L2288-L2295

mathis1337 commented 3 months ago

These are the only two I am aware of for the vSeeds right now:

vSeeds.emplace_back("seed.testnet4.bitcoin.sprovoost.nl."); // Sjors Provoost
vSeeds.emplace_back("seed.testnet4.wiz.biz."); // Jason Maurice

You can verify this here: https://github.com/fjahr/bitcoin/blob/2024-04-testnet-4-fix/src/kernel/chainparams.cpp#L350 and the official PR: https://github.com/bitcoin/bitcoin/pull/29775/files#diff-e20339c384d6f19b519ea2de7f4ba4fed92a36d66a80f0339b09927c3fa38d6dR350

mathis1337 commented 3 months ago

I added a PR to yours, so you can merge in if you want. https://github.com/turbolay/NBitcoin/pull/1

turbolay commented 3 months ago

Thanks!! Not sure why I missed that. We will review this PR next week with @lontivero and ping NDorier when we will be sure that it's mergeable

lontivero commented 3 months ago

ACK. It connects to the p2p network and interacts with it flawlessly. What else should I test? @NicolasDorier ?

NicolasDorier commented 3 months ago

Do not add ChainName.Testnet4. This is something specific to Bitcoin, while the ChainName type is not. You can use a static variable Testnet4ChainName in the Bitcoin type, and handle the case in the GetNetwork.

turbolay commented 2 months ago

@lontivero Can you check at last commit please? 9b4a9aa I also made this approach for the PR, not sure which is best as they both look bad.

lontivero commented 2 months ago

I don't have context. Why is not okay to have a ChainName for testnet4?

farukterzioglu commented 1 month ago

I don't have context. Why is not okay to have a ChainName for testnet4?

@lontivero testnet4 is only for Bitcoin. Other blockchains (Litecoin, Dash etc.) don't have that.

@turbolay do you have any blocker to have progress with this, is it ready to be reviewed? Relevant change is already merged to Bitcoin Core.

turbolay commented 1 month ago

@farukterzioglu I think it's OK, even if the solution doesn't look like the best

Please also considerate this alternative, that makes other kind of compromises: https://github.com/turbolay/NBitcoin/commit/abc2aec594d02a15e2e254fec8858002e0c95ab7#diff-12355e5baf374196ed9c5a0708f16632568b4bfc9139c8daff9aca066fd9f8e1

Feel free to request changes, I would apply fast, or take over the PR.

farukterzioglu commented 1 month ago

@NicolasDorier, above lontivero confirmed it can connect to the new testnet. And it is refactored as you suggested above. Does the rest look good? Would you have additional comment?

Kukks commented 2 weeks ago

Can you rebase? We moved things around to get everything cleaner

turbolay commented 2 weeks ago

Can you rebase? We moved things around to get everything cleaner

Sorry I am busy lately, thank you for the refactor, it looks good indeed. I will rebase tonight.