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

feat: Add testnet4 chain support (recreated) #2318

Closed

Conversation

bullet-tooth
Copy link

This PR is a recreation of the previous PR because I incidentally dropped a brunch and the PR was closed automatically.

Description

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

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 updates. Definitely looks better now.

Have a couple of fixes that are required, otherwise things crash quite immediately (please test things before requesting review in the future).

After those fixes I was able to sync up to block height 70513

@saubyk saubyk added this to the v0.25 milestone Feb 25, 2025
@saubyk
Copy link
Collaborator

saubyk commented Feb 25, 2025

hey @bullet-tooth are you planning to work on this pr at a high priority? We are interested in moving this along quickly. Thanks.

@bullet-tooth
Copy link
Author

hey @bullet-tooth are you planning to work on this pr at a high priority? We are interested in moving this along quickly. Thanks.

Done.

Also, verified locally, it could sync up to the current height (71627).

@coveralls
Copy link

coveralls commented Feb 26, 2025

Pull Request Test Coverage Report for Build 13721174389

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

  • 24 of 68 (35.29%) changed or added relevant lines in 10 files are covered.
  • 15 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.05%) to 55.277%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rpcclient/infrastructure.go 0 2 0.0%
rpcserver.go 0 2 0.0%
cmd/addblock/config.go 0 4 0.0%
cmd/findcheckpoint/config.go 0 4 0.0%
config.go 0 4 0.0%
database/cmd/dbtool/globalconfig.go 0 4 0.0%
blockchain/difficulty.go 2 7 28.57%
blockchain/validate.go 21 30 70.0%
cmd/btcctl/config.go 0 10 0.0%
Files with Coverage Reduction New Missed Lines %
peer/peer.go 15 73.49%
Totals Coverage Status
Change from base Build 13719035689: -0.05%
Covered Lines: 29953
Relevant Lines: 54187

💛 - Coveralls

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.

Would be nice if you could squash/fixup the last 4 commits into those that actually introduced the code (to make it easy for the next reviewer, see https://thoughtbot.com/blog/git-interactive-rebase-squash-amend-rewriting-history).
Other than that looks good to me, thanks!

Need a second reviewer though.

),
},
DeploymentTaproot: {
BitNumber: 2,
Copy link
Member

Choose a reason for hiding this comment

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

Was a different bit used for testnet4? If I sync up, I don't see the taproot soft fork active:

⛰   ./btcctl --testnet4 getblockchaininfo
{
  "chain": "testnet4",
  "blocks": 72925,
  "headers": 72925,
  "bestblockhash": "00000000d9dbd8ee8e69cc8b3dbb4beeeaf1d31a854ce2e61ee2100a807e70ef",
  "difficulty": 1,
  "mediantime": 1741301713,
  "pruned": false,
  "bip9_softforks": {
    "csv": {
      "status": "active",
      "bit": 29,
      "startTime": 0,
      "start_time": -62135596800,
      "timeout": -62135596800,
      "since": 0,
      "min_activation_height": 0
    },
    "dummy": {
      "status": "failed",
      "bit": 28,
      "startTime": 0,
      "start_time": 1199145601,
      "timeout": 1230767999,
      "since": 0,
      "min_activation_height": 0
    },
    "dummy-min-activation": {
      "status": "started",
      "bit": 22,
      "startTime": 0,
      "start_time": -62135596800,
      "timeout": -62135596800,
      "since": 0,
      "min_activation_height": 100000
    },
    "segwit": {
      "status": "active",
      "bit": 29,
      "startTime": 0,
      "start_time": -62135596800,
      "timeout": -62135596800,
      "since": 0,
      "min_activation_height": 0
    },
    "taproot": {
      "status": "started",
      "bit": 2,
      "startTime": 0,
      "start_time": -62135596800,
      "timeout": -62135596800,
      "since": 0,
      "min_activation_height": 0
    }
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so looking at the bitcoind PR, they just have hard coded heights for the activation for all past soft forks, including taproot: https://github.com/bitcoin/bitcoin/pull/29775/files#diff-e20339c384d6f19b519ea2de7f4ba4fed92a36d66a80f0339b09927c3fa38d6dR311-R316

We'll need to implement something similar, otherwise btcd will think that those forks aren't active at all.

Copy link
Author

Choose a reason for hiding this comment

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

What do you think about the following approach,

to add AlwaysActive bool field into ConsensusDeployment struct and set it to true. So the testnet4 deployments configuration will look like:

		DeploymentCSV: {
			BitNumber:    0,
			AlwaysActive: true,
		},
		DeploymentSegwit: {
			BitNumber:    1,
			AlwaysActive: true,
		},
		DeploymentTaproot: {
			BitNumber:    2,
			AlwaysActive: true,
		},

Or, will it be better to have hard coded heights for the activation similar to bitcoind?

Copy link
Member

Choose a reason for hiding this comment

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

Or, will it be better to have hard coded heights for the activation similar to bitcoind?

Perhaps let's go with the hard coded activation heights? I think the code diff for both versions would be somewhat similar.

The target area for this change would be the bip 9/8 activation logic:

// ThresholdState returns the current rule change threshold state of the given
// deployment ID for the block AFTER the end of the current best chain.
//
// This function is safe for concurrent access.
func (b *BlockChain) ThresholdState(deploymentID uint32) (ThresholdState, error) {
b.chainLock.Lock()
state, err := b.deploymentState(b.bestChain.Tip(), deploymentID)
b.chainLock.Unlock()
return state, err
}
// IsDeploymentActive returns true if the target deploymentID is active, and
// false otherwise.
//
// This function is safe for concurrent access.
func (b *BlockChain) IsDeploymentActive(deploymentID uint32) (bool, error) {
b.chainLock.Lock()
state, err := b.deploymentState(b.bestChain.Tip(), deploymentID)
b.chainLock.Unlock()
if err != nil {
return false, err
}
return state == ThresholdActive, nil
}
// deploymentState returns the current rule change threshold for a given
// deploymentID. The threshold is evaluated from the point of view of the block
// node passed in as the first argument to this method.
//
// It is important to note that, as the variable name indicates, this function
// expects the block node prior to the block for which the deployment state is
// desired. In other words, the returned deployment state is for the block
// AFTER the passed node.
//
// This function MUST be called with the chain state lock held (for writes).
func (b *BlockChain) deploymentState(prevNode *blockNode, deploymentID uint32) (ThresholdState, error) {
if deploymentID > uint32(len(b.chainParams.Deployments)) {
return ThresholdFailed, DeploymentError(deploymentID)
}
deployment := &b.chainParams.Deployments[deploymentID]
checker := deploymentChecker{deployment: deployment, chain: b}
cache := &b.deploymentCaches[deploymentID]
return b.thresholdState(prevNode, checker, cache)
}
// initThresholdCaches initializes the threshold state caches for each warning
// bit and defined deployment and provides warnings if the chain is current per
// the warnUnknownRuleActivations function.
func (b *BlockChain) initThresholdCaches() error {
// Initialize the warning and deployment caches by calculating the
// threshold state for each of them. This will ensure the caches are
// populated and any states that needed to be recalculated due to
// definition changes is done now.
prevNode := b.bestChain.Tip().parent
for bit := uint32(0); bit < vbNumBits; bit++ {
checker := bitConditionChecker{bit: bit, chain: b}
cache := &b.warningCaches[bit]
_, err := b.thresholdState(prevNode, checker, cache)
if err != nil {
return err
}
}
for id := 0; id < len(b.chainParams.Deployments); id++ {
deployment := &b.chainParams.Deployments[id]
cache := &b.deploymentCaches[id]
checker := deploymentChecker{deployment: deployment, chain: b}
_, err := b.thresholdState(prevNode, checker, cache)
if err != nil {
return err
}
}
// No warnings about unknown rules until the chain is current.
if b.isCurrent() {
bestNode := b.bestChain.Tip()
// Warn if any unknown new rules are either about to activate or
// have already been activated.
if err := b.warnUnknownRuleActivations(bestNode); err != nil {
return err
}
}
return nil
}

We can update the deploymentChecker to read that activation height, and just go directly to active once that height has been passed. The thresholdState method already has access to the current block height, as it can derive that from the prevNode.

I think we should do this in another PR so the diff is isolated, and we can add unit + itests for the change.

@@ -170,7 +170,8 @@ type config struct {
SigNet bool `long:"signet" description:"Use the signet test network"`
SigNetChallenge string `long:"signetchallenge" description:"Connect to a custom signet network defined by this challenge instead of using the global default signet test network -- Can be specified multiple times"`
SigNetSeedNode []string `long:"signetseednode" description:"Specify a seed node for the signet network instead of using the global default signet network seed nodes"`
TestNet3 bool `long:"testnet" description:"Use the test network"`
TestNet3 bool `long:"testnet" description:"Use the test network (version 3)"`
TestNet4 bool `long:"testnet4" description:"Use the test network (version 4)"`
Copy link
Member

Choose a reason for hiding this comment

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

@Roasbeef
Copy link
Member

Roasbeef commented Mar 6, 2025

I added a commit to add some property based tests for assertNoTimewrap. Please take care to ensure those commits aren't dropped during a rebase.

bullet-tooth and others added 8 commits March 7, 2025 12:55
Add robust property-based tests for the assertNoTimeWarp function using
the rapid testing library. The tests verify the following scenarios:

- Basic property tests:
  - Only retarget blocks (block height divisible by blocksPerRetarget) are checked
  - Valid timestamps (within maxTimeWarp of previous block) pass validation
  - Invalid timestamps (too early) fail with appropriate ErrTimewarpAttack
  - Correct boundary behavior (exactly at maxTimeWarp limit)

- Invariant tests:
  - Function never panics with valid inputs
  - Non-retarget blocks always return nil regardless of timestamps

- Security tests:
  - All retarget blocks are protected from timewarp attacks
  - Non-retarget blocks are not affected by the timewarp check
@bullet-tooth bullet-tooth force-pushed the feat/add-testnet4-support branch from ca68f11 to 1732501 Compare March 7, 2025 12:57
@Roasbeef
Copy link
Member

I added two commits here that implement the BIP 9 changes, can you add them to this branch? https://github.com/btcsuite/btcd/compare/master...Roasbeef:btcd:bip-9-always-active?expand=1

@Roasbeef
Copy link
Member

@bullet-tooth thanks for starting this PR! If you don't mind, I'll take it over now to get it to a final merge state.

@Roasbeef Roasbeef closed this Mar 11, 2025
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.

5 participants