Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TestNet4 for Bitcoin #1216

Merged
merged 1 commit into from
Oct 30, 2024
Merged

Add TestNet4 for Bitcoin #1216

merged 1 commit into from
Oct 30, 2024

Conversation

turbolay
Copy link
Contributor

@turbolay turbolay commented Jun 20, 2024

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:

NBitcoin/NBitcoin/Network.cs

Lines 2288 to 2295 in 828b7d9

#if !NOSOCKET
vFixedSeeds.Clear();
vSeeds.Clear();
vSeeds.Add(new DNSSeedData("bitcoin.jonasschnelli.ch", "testnet-seed.bitcoin.jonasschnelli.ch"));
vSeeds.Add(new DNSSeedData("tbtc.petertodd.org", "seed.tbtc.petertodd.org"));
vSeeds.Add(new DNSSeedData("bitcoin.sprovoost.nl", "seed.testnet.bitcoin.sprovoost.nl"));
vSeeds.Add(new DNSSeedData("bluematt.me", "testnet-seed.bluematt.me"));
#endif

@turbolay turbolay marked this pull request as draft June 20, 2024 10:59
@turbolay turbolay mentioned this pull request Jun 20, 2024
@mathis1337
Copy link

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
Copy link

I added a PR to yours, so you can merge in if you want.
turbolay#1

@turbolay turbolay marked this pull request as ready for review June 22, 2024 08:18
@turbolay
Copy link
Contributor Author

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
Copy link
Contributor

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

@NicolasDorier
Copy link
Collaborator

NicolasDorier commented Jun 27, 2024

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.

public Network Regtest => Network.RegTest;

public string CryptoCode => "BTC";

public static readonly ChainName TestNet4Name = new ChainName("TestNet4");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is why I took so long to push: I'm not sure if it's correct to have this public. I tried another approach and move TestNet4 to Bitcoin class but ended up with a similar (worse probably) problem, see

@turbolay
Copy link
Contributor Author

@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
Copy link
Contributor

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

@farukterzioglu
Copy link
Collaborator

farukterzioglu commented Aug 26, 2024

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
Copy link
Contributor Author

turbolay commented Aug 27, 2024

@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: turbolay@abc2aec#diff-12355e5baf374196ed9c5a0708f16632568b4bfc9139c8daff9aca066fd9f8e1

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

@farukterzioglu
Copy link
Collaborator

@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
Copy link
Contributor

Kukks commented Sep 17, 2024

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

@turbolay
Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is a horrible converter with so many hardcoded stuff. It should use Network natively..

NBitcoin/Network.cs Outdated Show resolved Hide resolved
NBitcoin/Network.cs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants