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.

Sign up for GitHub

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

Jump to bottom

feat: Add testnet4 chain support #2274

Closed
wants to merge 0 commits into from
Closed

feat: Add testnet4 chain support #2274

wants to merge 0 commits into from
+0 −0

Conversation

bullet-tooth
Copy link

This PR adds support of the Bitcoin testnet version 4 chain (testnet3 is in a way of deprecation).

Reference: https://bips.dev/94/

Testnet4 genesis block: https://mempool.space/testnet4/block/00000000da84f2bafbbc53dee25a72ae507ff4914b867c565be350b0da8bf043

Also, I have intention to add testnet4 support to btcwallet after this PR

@Roasbeef
Copy link
Member

Looks like we have another here: #2275

Neither that one, nor this one, have any of the new difficulty adjustment algorithm changes.

@jcvernaleo
Copy link
Member

I know that in #2275 they were working on the difficultly bits. @marcopeereboom I thought you had started on that part, is it in your PR?

@Roasbeef Roasbeef mentioned this pull request Nov 13, 2024
Open
@bullet-tooth
Copy link
Author

@Roasbeef @jcvernaleo thanks for your inputs.
As I can see the #2275 is still in draft so if you could provide a little context about this "difficulty adjustment algorithm changes" I could implement it as well in this PR.

@Roasbeef
Copy link
Member

@bullet-tooth see the BIP: https://bips.dev/94/

@bullet-tooth bullet-tooth marked this pull request as draft November 23, 2024 00:09
@bullet-tooth bullet-tooth marked this pull request as ready for review November 23, 2024 14:59
@coveralls
Copy link

coveralls commented Nov 25, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 12318119030

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 15 of 41 (36.59%) changed or added relevant lines in 8 files are covered.
  • 14 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.04%) to 57.245%
Changes Missing Coverage Covered Lines Changed/Added Lines %
peer/peer.go 0 1 0.0%
rpcserver.go 0 2 0.0%
blockchain/difficulty.go 6 10 60.0%
config.go 0 4 0.0%
btcd.go 0 5 0.0%
blockchain/validate.go 5 15 33.33%
Files with Coverage Reduction New Missed Lines %
rpcclient/infrastructure.go 2 40.17%
txscript/taproot.go 2 95.98%
peer/peer.go 10 74.1%
Totals Coverage Status
Change from base Build 12253655920: -0.04%
Covered Lines: 29910
Relevant Lines: 52249
💛 - Coveralls

@bullet-tooth
Copy link
Author

Hey @Roasbeef, do you have any estimates about reviewing/merging this PR?

@guggero
Copy link
Collaborator

guggero commented Dec 10, 2024

If you clean up the PR into a review friendly commit structure, I'll take a look.
Here are some docs to go by: https://github.com/lightningnetwork/lnd/blob/master/docs/code_contribution_guidelines.md
and https://github.com/lightningnetwork/lnd/blob/master/docs/code_formatting_rules.md.
Those aren't strictly enforced in this project but definitely nice to have if you want review from the lnd team.

@bullet-tooth bullet-tooth force-pushed the feat/add-testnet4-support branch from c71544f to 730a170 Compare December 13, 2024 15:08
@bullet-tooth
Copy link
Author

Hello, @guggero. Do you have any estimates when you will be able to review the PR.
Also, such as it already has some conflicts I will resolve them in the near future.

@guggero guggero self-requested a review January 14, 2025 15:51
guggero
guggero requested changes Jan 16, 2025
View reviewed changes
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Thanks for the update.
Still requires quite a bit of formal cleanup before more review time should be put in.

With "review friendly commit structure" I didn't mean squashing everything into a single commit.
Suggestion for commit order:

  • Any comment/typo fixes.
  • Any changes to non-Go files (e.g. .gitignore)
  • Add new parameter to chaincfg including unit tests.
  • Add EnforceBip94 flag to parameters.
  • Add changes to blockchain package.
  • Add changes to cmd package.

blockchain/difficulty.go Outdated Show resolved Hide resolved
blockchain/error.go Outdated Show resolved Hide resolved
blockchain/validate.go Outdated Show resolved Hide resolved
blockchain/validate.go Outdated
Comment on lines 725 to 713
if c.ChainParams().EnforceBIP94 {
// Check timestamp for the first block of each difficulty adjustment
// interval, except the genesis block.
if blockHeight%c.BlocksPerRetarget() == 0 {
prevBlockTimestamp := time.Unix(prevNode.Timestamp(), 0)
if header.Timestamp.Before(prevBlockTimestamp.Add(-maxTimeWarp)) {
str := "block's timestamp %v is too early on diff adjustment block %v"
str = fmt.Sprintf(str, header.Timestamp, prevBlockTimestamp)
return ruleError(ErrTimewarpAttack, str)
}
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: line length. Also, sounds like this whole block could be extracted into a standalone function. Something like checkBip94().

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure such an approach will improve the code quality because we need to pass parameters and handle errors anyway. So I decided not to introduce a new function

btcd.go Outdated Show resolved Hide resolved
cmd/addblock/config.go Outdated Show resolved Hide resolved
cmd/btcctl/config.go Outdated Show resolved Hide resolved
cmd/findcheckpoint/config.go Outdated Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
peer/peer.go Outdated Show resolved Hide resolved
@humb1t
Copy link

humb1t commented Feb 10, 2025

Great work @bullet-tooth - can't wait to see this PR being merged.

@bullet-tooth bullet-tooth force-pushed the feat/add-testnet4-support branch from 730a170 to 09ba026 Compare February 10, 2025 15:56
@bullet-tooth bullet-tooth deleted the feat/add-testnet4-support branch February 10, 2025 17:10
@bullet-tooth bullet-tooth mentioned this pull request Feb 10, 2025
Open
@bullet-tooth
Copy link
Author

@guggero I created a new PR where resolved all the mentioned comments and made several commits history. Because incidentally dropped commits from this one and the PR was closed automatically. Sorry for the inconvenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guggero guggero guggero requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bullet-tooth @Roasbeef @jcvernaleo @coveralls @guggero @humb1t