Implement the CSV-Package soft-fork via BIP0009 VersionBits #775

Roasbeef · 2016-09-27T19:19:27Z

This commit fully implements the CSV-package soft-fork. The CSV-package soft-fork bundles the deployment of the following BIPs: 68, 112, and 113. The activation logic of the soft-fork is governed by BIP 9, VersionBits.

This PR currently is built upon a combined branch includes #713, #628, #712, and #755. Therefore, only the last 3 commits should be examined during code-review. As the aforementioned pending PR's re merged into master, this PR will be updated accordingly to increasingly reflect the final diff.

A summary of the proposed changes follows:

A new ConsensusDeployment has been added to the chain parameters for mainnet, testnet, simnet, and regtestnet. This new deployment defines the BIP0009 parameters (bit #, start time, end time) for the CSV-package soft-fork. A new constant, DeploymentCSV has been added which uniquely identifies the new deployment within the Deployments array for each of the target networks.
During non-contextual, and contextual block validation, we now examine the ThresholdState for the CSV deployment, activating the observance of the new consensus rules once the soft-fork switches to the ThresholdActive state.
- If the soft-fork is active, then MTP is used for transaction finality checks, the CSV op-code is recognized and validated during Script execution, and a transaction can only be included within a block if the sequence locks for all its inputs have been met.
Finally, when selecting transactions for inclusion into the current block template, the version bits state is again checked in order to guard the observance of the new consensus rules. If the soft-fork is active, then transactions can only be included into the to-be-constructed block iff they don't violate any of the new consensus rules.

Roasbeef · 2016-09-27T19:23:12Z

I plan to cherry-pick the integration tests I wrote for BIPs 112 and 113 which were extracted from the final version of the PR's in favor of reducing the scope of them by making them mempool only. I'm also working on an integration test for BIP 68 (CSV) will also be added to this PR once complete.

@davecgh you mentioned earlier that you'd prefer the integration tests for soft-fork be segmented into a distinct package as they have stricter requirements on the pre-conditions of the test-chain. Once the new package is added to the BIP 9 PR, I'll move my integration tests to the new package.

davecgh · 2016-10-27T14:34:22Z

This can be rebased since #628, #712, and #713 have all been merged. Also, #755 is ready for master, but it is waiting on this one so they can be deployed at the same time.

davecgh · 2016-10-27T18:28:39Z

blockchain/validate.go

+		// Obtain the latest state of the deployed CSV soft-fork in
+		// order to properly guard the new validation behavior based on
+		// the current BIP 9 version bits state.
+		csvState, err := b.ThresholdState(chaincfg.DeploymentCSV)


I believe this will end up dead locking because the chain state lock is already held and the exported ThresholdState takes the lock. Given the usefulness of being able to check the state based on a deployment as the exported version does, I suspect we might need to refactor it slightly such that there is a thresholdHoldForDeployment or something similar.

I've carried out your suggestion and have created a new internal version of ThresholdState which assumes that the caller already has the chainLock acquired. This function is now used in this fragment in order to prevent the deadlock condition you pointed out.

davecgh · 2016-10-27T18:34:56Z

blockchain/validate.go

+		var blockTime time.Time
+		if csvState == ThresholdActive {
+			blockTime = medianTime
+		} else {


nit: Perhaps it's just a matter of taste, but I find the set and override more readable since it's shorter:

blockTime := header.Timestamp if csvState == ThresholdActive { blockTime = medianTime }

Agreed. I've modified this fragment to eliminate the else cause by setting the blockTime value to header.Timestamp initially. As a result, the code is a bit shorter and easier to read as you noted.

davecgh · 2016-10-27T18:38:33Z

blockchain/validate.go

+		// view from the database. This view is required in order to
+		// properly evalulate the sequence locks (relative lock-time)
+		// for each input referenced within the block.
+		utxoView := NewUtxoViewpoint()


Since we don't have a utxo cache this will end up double loading the utxos. Also, this assumes the block is extended the main chain which may or may not be true. We'll need to figure out a better place to do these checks and/or get the view earlier and pass it in to the function as well as down the call chain.

davecgh · 2016-10-27T18:39:39Z

mining.go

+	// Once the CSV soft-fork is fully active, we'll switch to using the
+	// current median time past of the past block's timestamps for all
+	// lock-time based checks.
+	var blockTime time.Time


Same comment here regarding setting and overriding.

davecgh · 2016-10-28T23:00:09Z

blockchain/thresholdstate.go

+// tip of the current best chain.
+//
+// This function MUST be called with the chain state lock held (for writes).
+func (b *BlockChain) thresholdHoldForDeployment(deploymentID uint32) (ThresholdState, error) {


A couple of points here. First one is a nit. Everything else calls it threshold as one word, so this should probably be thresholdForDeployment (lowercase h).

Second point is the function should take the *blockNode to operate on:

func (b *BlockChain) thresholdholdForDeployment(node *blockNode, deploymentID uint32) (ThresholdState, error) { ... return b.thresholdState(node, checker, cache) }

davecgh · 2016-10-28T23:01:04Z

blockchain/thresholdstate.go

+// This function is safe for concurrent access.
+func (b *BlockChain) ThresholdState(deploymentID uint32) (ThresholdState, error) {
+	b.chainLock.Lock()
+	state, err := b.thresholdHoldForDeployment(deploymentID)


With the below mentioned changes this would become:

b.thresholdForDeployment(b.bestNode, deploymentID)

davecgh · 2016-10-28T23:05:03Z

blockchain/validate.go

@@ -1133,6 +1217,16 @@ func (b *BlockChain) checkConnectBlock(node *blockNode, block *btcutil.Block, vi
 		scriptFlags |= txscript.ScriptVerifyCheckLockTimeVerify
 	}

+	// Enforce CHECKSEQUENCEVERIFY during all block validation checks once
+	// the soft-fork deployment is fully active.
+	csvState, err := b.thresholdHoldForDeployment(chaincfg.DeploymentCSV)


And this would need to be:

csvState, err := b.thresholdholdForDeployment(prevNode, chaincfg.DeploymentCSV)

so it is from the point of view of the block being checked instead of the main chain.

davecgh · 2016-10-29T01:40:48Z

blockchain/chain.go

+	// Obtain the latest BIP9 version bits state for the CSV-package
+	// soft-fork deployment. The adherence of sequence locks depends on the
+	// current soft-fork state.
+	csvState, err := b.ThresholdState(chaincfg.DeploymentCSV)


This will deadlock since it takes the chain state lock and it's already held in this function.

Also, calcSequenceLock needs to accept a *blockNode and be updated to use the passed node and use it here as b.thresholdForDeployment(node, chaincfg.DeploymentCSV) as well as in place of other b.bestNode references since the function needs to work form arbitrary points in the chain during reorgs.

Then it would be called with b.bestNode from the exported CalcSequenceLock and prevNode (need to verify if it's prevNode or node) in checkConnectBlock in the validation code below to ensure it is calculated according to the correct block.

I believe this should do it:

--- a/blockchain/chain.go +++ b/blockchain/chain.go @@ -776,18 +776,14 @@ func (b *BlockChain) CalcSequenceLock(tx *btcutil.Tx, utxoView *UtxoViewpoint, b.chainLock.Lock() defer b.chainLock.Unlock() - return b.calcSequenceLock(tx, utxoView, mempool) + return b.calcSequenceLock(b.bestNode, tx, utxoView, mempool) } // calcSequenceLock computes the relative lock-times for the passed // transaction. See the exported version, CalcSequenceLock for further details. // // This function MUST be called with the chain state lock held (for writes). -func (b *BlockChain) calcSequenceLock(tx *btcutil.Tx, utxoView *UtxoViewpoint, - mempool bool) (*SequenceLock, error) { - - mTx := tx.MsgTx() - +func (b *BlockChain) calcSequenceLock(node *blockNode, tx *btcutil.Tx, utxoView *UtxoViewpoint, mempool bool) (*SequenceLock, error) { // A value of -1 for each relative lock type represents a relative time // lock value that will allow a transaction to be included in a block // at any given height or time. This value is returned as the relative @@ -798,7 +794,7 @@ func (b *BlockChain) calcSequenceLock(tx *btcutil.Tx, utxoView *UtxoViewpoint, // Obtain the latest BIP9 version bits state for the CSV-package // soft-fork deployment. The adherence of sequence locks depends on the // current soft-fork state. - csvState, err := b.ThresholdState(chaincfg.DeploymentCSV) + csvState, err := b.thresholdForDeployment(node, chaincfg.DeploymentCSV) if err != nil { return nil, err } @@ -809,6 +805,7 @@ func (b *BlockChain) calcSequenceLock(tx *btcutil.Tx, utxoView *UtxoViewpoint, // sequence locks don't apply to coinbase transactions Therefore, we // return sequence lock values of -1 indicating that this transaction // can be included within a block at any given height or time. + mTx := tx.MsgTx() sequenceLockActive := mTx.Version >= 2 && (mempool || csvSoftforkActive) if !sequenceLockActive || IsCoinBase(tx) { return sequenceLock, nil @@ -853,8 +850,8 @@ func (b *BlockChain) calcSequenceLock(tx *btcutil.Tx, utxoView *UtxoViewpoint, // compute the past median time for the block prior to // the one which included this referenced output. // TODO: caching should be added to keep this speedy - inputDepth := uint32(b.bestNode.height-inputHeight) + 1 - blockNode, err := b.relativeNode(b.bestNode, inputDepth) + inputDepth := uint32(node.height-inputHeight) + 1 + blockNode, err := b.relativeNode(node, inputDepth) if err != nil { return sequenceLock, err } diff --git a/blockchain/validate.go b/blockchain/validate.go index c8fbb58..413e0cb 100644 --- a/blockchain/validate.go +++ b/blockchain/validate.go @@ -1209,7 +1209,8 @@ func (b *BlockChain) checkConnectBlock(node *blockNode, block *btcutil.Block, vi // A transaction can only be included within a block // once the sequence locks of *all* its inputs are // active. - sequenceLock, err := b.calcSequenceLock(tx, view, false) + sequenceLock, err := b.calcSequenceLock(prevNode, tx, + view, false) if err != nil { return err } ~

davecgh · 2016-10-29T04:14:48Z

blockchain/thresholdstate.go

+// blockNode passed in as the first argument to this method.
+//
+// This function MUST be called with the chain state lock held (for writes).
+func (b *BlockChain) thresholdForDeployment(blockNode *blockNode,


I don't want to bikeshed too much since it's important we get this PR finished and landed, but I wonder if this might be better named deploymentState since I noticed that thresholdForDeployment is quite long and most of the callers end up using some variant of the name state in the variable anyways. Definitely not something that has to be changed, but if you're in there making modifications anyways to fix the actual code issues pointed out regarding reorgs, and you think it's a good idea, go for it.

Roasbeef · 2016-11-08T05:53:38Z

I've successfully fully synced both testnet and mainnet with this PR without any issues. The version bits state guarded consensus enforcements properly flipped over perfectly without any issues 🎉

davecgh · 2016-11-08T08:52:52Z

Thanks for following up with test results. I'm going to do some testing myself as well. I did want to point out that the RPC server help for the new getblockchaininfo command is missing as reported by Travis:

rpcserverhelp_test.go:17: RPC handler defined for method 'getblockchaininfo' without also specifying result types
rpcserverhelp_test.go:44: Failed to generate help for method 'getblockchaininfo': no result types specified for method getblockchaininfo

stevenroose · 2016-11-08T23:35:05Z

rpcserver.go

+
+	// Finally, query the BIP0009 verion bits state fo all currently
+	// defined BIP0009 soft-fork deployments.
+	for deployment, depolymentDetails := range activeNetParams.Deployments {


query the BIP0009 verion bits state

version*

for deployment, depolymentDetails

deploymentsDetails*

stevenroose · 2016-11-08T23:56:49Z

blockchain/thresholdstate.go

+// since these values are serialized and must be stable for long-term storage.
+const (
+	// ThresholdDefined is the first state for each deployment and is the
+	// state for the genesis block has by defintion for all deployments.


defintion

definition*

stevenroose · 2016-11-08T23:57:18Z

blockchain/thresholdstate.go

+	c.entries[hash] = state
+}
+
+// MarkFlushed marks all of the current udpates as flushed to the database.


udpates

updates*

stevenroose · 2016-11-08T23:58:02Z

blockchain/thresholdstate.go

+
+	// Condition returns whether or not the rule change activation condition
+	// has been met.  This typically involves checking whether or not the
+	// bit assocaited with the condition is set, but can be more complex as


assocaited

associated*

davecgh · 2016-11-10T06:47:47Z

blockchain/thresholdstate.go

+	return state, err
+}
+
+// thresholdForDeployment returns the current rule change threshold for a


s/thresholdForDeployment/deploymentState/

davecgh · 2016-12-01T03:58:01Z

The following diff should take care of the test failure:

diff --git a/rpctest/rpc_harness.go b/rpctest/rpc_harness.go
index d09b472..dc3853e 100644
--- a/rpctest/rpc_harness.go
+++ b/rpctest/rpc_harness.go
@@ -273,7 +273,9 @@ func (h *Harness) TearDown() error {
                return err
        }

+       harnessStateMtx.Lock()
        delete(testInstances, h.testNodeDir)
+       harnessStateMtx.Unlock()

        return nil
 }

davecgh · 2016-12-01T04:47:33Z

Looks like TearDownAll grabs the mutex and then calls harness.TearDown which grabs it again. So, I believe there needs to be an unexported func (h *Harness) tearDown() error { which has to be called with the harnessStateMtx held that is called from TearDownAll. Then Teardown would just become:

// TearDown stops the running rpc test instance. All created processes are
// killed, and temporary directories removed.
//
// NOTE: This method and SetUp should always be called from the same goroutine
// as they are not concurrent safe.
func (h *Harness) TearDown() error {
	harnessStateMtx.Lock()
	defer harnessStateMtx.Unlock()

	return h.teardown()
}

davecgh · 2016-12-01T04:02:44Z

rpcserverhelp.go

+	"softforkdescription-reject":  "The current activation status of the softfork",
+	"softforkdescription-version": "The block version that signals enforcement of this softfork",
+	"softforkdescription-id":      "The string identifier for the soft fork",
+	"-status":                     "A bool which indicates if the soft fork is active",


"softforkdescription-status"

davecgh · 2016-12-01T20:10:49Z

rpcserver.go

+		switch deployment {
+		case chaincfg.DeploymentCSV:
+			forkName = "csv"
+		}


I think there should be a default case here which results in an internalRPCError so tests will end up failing if a new deployment is added without also covering its fork name.

Alternatively, the name could be added to the chainparams.ConsensusDeployment struct, but I do prefer the RPC specific strings to be consolidated here in the RPC server code.

Good point, I've opted to go with the first option.

davecgh · 2016-12-01T20:11:32Z

rpcserver.go

+		// identified by its deployment ID.
+		deploymentStatus, err := s.chain.ThresholdState(uint32(deployment))
+		if err != nil {
+			return nil, err


This should be converted to a btcjson.RPCError because this is in an RPC handler. All error returns should be of that type.

Perhaps something like the following:

context := "Failed to obtain deployment status" return nil, internalRPCError(err.Error(), context)

Nice catch, fixed.

davecgh · 2016-12-01T20:19:06Z

rpcserver.go

+		// The string method for deployment status has a "Threshold"
+		// prefix for all the states, so we trim off this section in
+		// order to display it properly.
+		statusString := strings.TrimPrefix(deploymentStatus.String(),


I'm a little bit concerned about this because it means changing the internal variable names would end up returning a different status string.

I suspect there should be a conversion function which returns the appropriate status string given a ThresholdState and uses a switch with a default case that returns an error. That way any additional threshold states would be properly caught.

Then this would become something like:

statusString, err := softForkStatus(deploymentStatus) if err != nil { context := "Failed to obtain soft fork status description" return nil, internalRPCError(err.Error(), context) }

Good point, switching to a function like softForkStatus is more robust and will catch errors much sooner. I've modified this fragment to use such a function instead.

davecgh · 2016-12-01T21:19:33Z

chaincfg/params.go

@@ -270,6 +275,11 @@ var MainNetParams = Params{
 			StartTime:  1199145601, // January 1, 2008 UTC
 			ExpireTime: 1230767999, // December 31, 2008 UTC
 		},
+		DeploymentCSV: {
+			BitNumber:  0,
+			StartTime:  1462060800, // March 1st, 2016


This is May 1st, 2016.

fmt.Println(time.Unix(1462060800, 0).UTC())

Output:
2016-05-01 00:00:00 +0000 UTC

Yep, I noticed that a bit ago and fixed it. Looks like this comment was made from an outdated diff.

davecgh · 2016-12-01T21:33:36Z

blockchain/chain.go

+	// Obtain the latest BIP9 version bits state for the CSV-package
+	// soft-fork deployment. The adherence of sequence locks depends on the
+	// current soft-fork state.
+	csvState, err := b.deploymentState(node, chaincfg.DeploymentCSV)


What do you think about changing this to avoid the extra work when mempool is set? It seems wasteful to calculate the information when it isn't needed because the soft fork is guaranteed active when mempool is set. Perhaps something like:

// Comments... csvSoftforkActive := mempool if !csvSoftforkActive { csvState, err := b.deploymentState(node, chaincfg.DeploymentCSV) if err !=nil { return nil, err } csvSoftforkActive = csvState == ThresholdActive } ... sequenceLockActive := mTx.Version >= 2 && csvSoftforkActive ...

Nice, I like the suggestion. This'll also indirectly speed up mempool acceptance/processing since it doesn't need to unconditionally query the the version bit state.

davecgh · 2016-12-01T22:02:21Z

blockchain/validate.go

+
+		// Additionally, if the CSV soft-fork package is now active,
+		// then we also enforce the relative sequence number based
+		// lock-times within the inputs of all transacitons in this


s/transacitons/transactions/

davecgh · 2016-12-01T23:36:59Z

integration/csv_fork_test.go

+
+	for _, txn := range block.Transactions {
+		txHash := txn.TxHash()
+		if (&txHash).IsEqual(txid) {


It is more efficient to just compare the hash directly like so:

if txn.TxHash() == *txHash {

I really need to make a PR to remove the IsEqual function from chainhash.Hash. It is a holdover from the very early days when we didn't really have as good of an understanding on the GC implications of doing it this way.

davecgh · 2016-12-01T23:41:27Z

integration/csv_fork_test.go

+		}
+	}
+
+	t.Fatalf("txid %v was not found in block %v", txid, blockHash)


It would probably be useful to include the runtime.Caller(1) information in the same vein as the BIP0009 assertVersionBit function. That really helps in the case of failure since otherwise you can't tell which invocation actually cause the failure.

Fixed, also added similar caller/line context to the isSoftForkActive function.

davecgh · 2016-12-01T23:54:29Z

integration/csv_fork_test.go

+	if err := r.SetUp(true, 10); err != nil {
+		t.Fatalf("unable to setup test chain: %v", err)
+	}
+


This needs to do teardown.

defer r.TearDown()

davecgh · 2016-12-02T00:05:37Z

integration/csv_fork_test.go

+	}
+
+	// As the transaction is p2sh it should be accepted into the mempool
+	// and fond within the next generated block.


s/fond/found/

davecgh · 2016-12-02T00:08:52Z

integration/csv_fork_test.go

+
+	// Create a trivially spendable output with a CSV lock-time of 10
+	// relative blocks.
+	redeemScript, testUTXO, tx, err := createCSVOutput(r, t, outputAmt, 10,


Since 10 is used here and below, should the constant for relativeBlockLock be moved up and used instead?

davecgh · 2016-12-02T01:39:29Z

integration/csv_fork_test.go

+// Overview:
+//  - Pre soft-fork:
+//    - Transactions with non-final lock-times from the PoV of MTP should be
+//    rejected from the mempool.


Please use hanging indents for consistency.

+// - Transactions with non-final lock-times from the PoV of MTP should be +// rejected from the mempool.

davecgh · 2016-12-02T01:41:55Z

integration/csv_fork_test.go

+func TestBIP0113Activation(t *testing.T) {
+	t.Parallel()
+
+	// Initialize the primary mining node with only the genesis block.


This comment is not accurate. It says only the genesis block, yet it is creating 10 mature outputs (which really means 100 blocks for the coinbase maturity and an additional 10). Along the same lines, why 10? All that is needed is 1 from what I can tell.

Fixed, it only now generates 101 blocks in order to obtain a single mature output.

davecgh · 2016-12-02T02:44:16Z

integration/csv_fork_test.go

+		t.Fatalf("transaction accepted, but should be non-final")
+	} else if !strings.Contains(err.Error(), "not finalized") {
+		t.Fatalf("transaction should be rejected due to being "+
+			"non-final, instead :%v", err)


There is an extra space before the colon here.

davecgh · 2016-12-02T02:49:01Z

integration/csv_fork_test.go

+	// Next, mine enough blocks to ensure that the soft-fork becomes
+	// activated. Assert that the block version of the second-to-last block
+	// in the final range is active.
+	numBlocks := (r.ActiveNet.MinerConfirmationWindow * 2) + 1


This does not look accurate to me. The call to SetUp above already has 10 blocks generated (plus the coinbase maturity, so 110) and an additional two generated block for a total of 112.

Since simnet has MinerConfirmationWindow of 100, the CSV activation should happen at block 300, which is 300 - 112 = 188 blocks from the current height. Which would be (r.ActiveNet.MinerConfirmationWindow * 2) - 12.

I think it would also be worthwhile to make sure the block just before the activation threshold, (r.ActiveNet.MinerConfirmationWindow * 2) - 13, does not have CSV set either. I have a strong suspicion there is an off by one in the code (probably due to an incorrect use of prevNode instead of the current node).

I tested this very scenario and it claims CSV is active at height 299 even though I don't believe it should be active until 300.

It also might be ideal to assert the block chain height is what the test thinks it's supposed to be prior to making this call to ensure it is hitting the actual desired block heights.

Hmm, yes I think you're correct, CSV should be active at 300, but not at height 299.

As we discussed offline, I'll separate out the getblockchaininfo portion of this PR into a new one, so we can use it to write some additional integration tests for bip9 so we can drill down into the issue an locate where the off-by-one error is.

davecgh · 2016-12-02T05:07:19Z

integration/csv_fork_test.go

+	if err != nil {
+		return nil, nil, nil, err
+	}
+	txid, err := r.Node.SendRawTransaction(fundTx, true)


Please make all of these txid vars txHash instead. We have been very careful to avoid mischaracterizing the hash as an ID in general. Improperly treating the transaction hash as a unique ID is one of the main reason malleability led to so many issues in various software. I do realize a big reason for segwit is stable hashes so they can be (ab)used as an ID, but I'd really prefer to avoid the notion altogether in the code since non-segwit transactions still exist and it applies there even less.

Good point, fixed to instead read txHash in the latest diff.

davecgh · 2016-12-03T05:46:15Z

blockchain/thresholdstate.go

+// blockNode passed in as the first argument to this method.
+//
+// This function MUST be called with the chain state lock held (for writes).
+func (b *BlockChain) deploymentState(blockNode *blockNode,


The argument here really should be named prevNode because thresholdState accepts a prevNode. That is to say it returns the threshold state for the block after the provided block node (hence the name prevNode).

I would also suggest the following diff to make the comments call this out more clearly.

// deploymentState returns the current rule change threshold for a given // deploymentID. The threshold is evaluated from the point of view of the -// blockNode passed in as the first argument to this method. +// 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(blockNode *blockNode, - deploymentID uint32) (ThresholdState, error) { - +func (b *BlockChain) deploymentState(prevNode *blockNode, deploymentID uint32) (ThresholdState, error) {

Indeed, makes sense to ensure that all the documentation around these functions is sufficiently explicit in order to avoid future off-by-one errors like the ones we discovered in earlier PR's and this one itself.

Fixed in a forthcoming push.

davecgh · 2016-12-03T05:46:58Z

blockchain/validate.go

+		// timestamps for all lock-time based checks.
+		blockTime := header.Timestamp
+		if csvState == ThresholdActive {
+			medianTime, err := b.calcPastMedianTime(prevNode)


I believe this is incorrect. The calcPastMedianTime takes the block node you want to the calulate the median time for as opposed to its parent. However, I don't think it's available in this function at the current time. It'll probably need to be modified to pass in the current block node and get the previous one for the places that need it such as checkBlockHeaderContext and deploymentState.

This also likely means the block node creation for the block being validated will need to move up before the call to checkBlockContext in accept.go.

We really need some tests that check for off-by-one conditions in the sequence locks (both in blocks and timestamps).

As I noted on IRC, I believe that this is actually correct. The MTP used for a block to decide transaction finality is the median of the past 11 blocks. So this would mean using prevNode everywhere during transaction finality tests as a part of block validation.

This meshes with the mempool behavior at it uses the current MTP (of the chain tip) to decide if the transaction can be accepted into the mempool based on its eligibility for the next block. The latest set of commits includes a boundary condition tests for the adherence of MTP.

davecgh · 2016-12-03T05:51:51Z

blockchain/validate.go

@@ -1126,6 +1147,46 @@ func (b *BlockChain) checkConnectBlock(node *blockNode, block *btcutil.Block, vi
 		scriptFlags |= txscript.ScriptVerifyCheckLockTimeVerify
 	}

+	// Enforce CHECKSEQUENCEVERIFY during all block validation checks once
+	// the soft-fork deployment is fully active.
+	csvState, err := b.deploymentState(node, chaincfg.DeploymentCSV)


I believe this should be the previous node since deploymentState calculates the state for the block after the passed node.

davecgh · 2016-12-03T05:54:23Z

blockchain/chain.go

+		// Obtain the latest BIP9 version bits state for the
+		// CSV-package soft-fork deployment. The adherence of sequence
+		// locks depends on the current soft-fork state.
+		csvState, err := b.deploymentState(node, chaincfg.DeploymentCSV)


I believe this should be the previous node since deploymentState calculates the state for the block after the passed node.

davecgh · 2016-12-03T07:58:10Z

integration/csv_fork_test.go

+		t.Fatalf("unable to query for chain info: %v", err)
+	}
+
+	fmt.Println("height: ", chainInfo.Blocks)


Leftover debug.

davecgh · 2016-12-03T07:58:14Z

integration/csv_fork_test.go

+	fmt.Println("height: ", chainInfo.Blocks)
+
+	csvForkState, ok := chainInfo.Bip9SoftForks[bip9ForkName]
+	fmt.Println("status: ", csvForkState)


Leftover debug.

davecgh · 2016-12-07T03:40:30Z

integration/csv_fork_test.go

 	_, height, err := r.Node.GetBestBlock()
 	if err != nil {
 		t.Fatalf("unable to get best block: %v", err)
 	}
-	if height != 103 {
-		t.Fatalf("height mismatch: expected %v got %v", 102, height)
+	if height != 107 {


You might want to make use of the assertChainHeight function I added in #873 since it replaces all of the GetBestBlock and height check and resulting t.Fatalf accordingly. It's available to all of the integration tests.

assertChainHeight(r, t, 107)

Modified the tests to use the newly available testing helper funcs.

davecgh · 2016-12-07T03:58:17Z

integration/csv_fork_test.go

+	if err != nil {
+		t.Fatalf("unable to get best block: %v", err)
+	}
+	if height != 107 {


Might want to make use of the assertChainHeight function added in #873 since it replaces all of the GetBestBlock and height check and resulting t.Fatalf accordingly. It's available to all of the integration tests.

assertChainHeight(r, t, 107)

davecgh · 2016-12-07T04:00:28Z

integration/csv_fork_test.go

+	}
+	defer r.TearDown()
+
+	if isSoftForkActive(r, t, "csv") {


Might want to make use of the assertSoftForkStatus function added in #873 since it allows any status to be asserted and automatically handles the t.Fatalf accordingly. It's available to all of the integration tests.

const ( csvKey = "csv" ) ... assertSoftForkStatus(r, t, csvKey, blockchain.ThresholdActive)

Modified the tests to use assertSoftForkStatus when applicable.

davecgh · 2016-12-07T07:03:39Z

blockchain/chain_test.go

-	// compatible with Bitcoin Core.
-	nextMedianTime := blocks[2].MsgBlock().Header.Timestamp.Unix()
+	// The median time calculated from the PoV of the best block in our
+	// test chain. For unconfirmed inputs, this value will be used since th


davecgh · 2016-12-07T07:05:40Z

blockchain/chain_test.go


 	// Create a new database and chain instance to run tests against.
-	chain, teardownFunc, err := chainSetup("haveblock", &chaincfg.MainNetParams)
+	chain, teardownFunc, err := chainSetup("haveblock", netParams)


I believe the haveblock here can conflict with the other test with that name if they happen to run at the same time. Probably should be something like calcseqlock.

Haven't seen any signs of negative interactions yet myself, but changing to calcseqlock just to be safe :)

davecgh · 2016-12-07T07:08:37Z

blockchain/chain_test.go

+			mtp:   mtp,
+		})
+
+		prevBlock = block
 	}

 	// Create with all the utxos within the create created above.


s/create/blocks/

davecgh · 2016-12-07T07:17:33Z

blockchain/chain.go

+		// Obtain the latest BIP9 version bits state for the
+		// CSV-package soft-fork deployment. The adherence of sequence
+		// locks depends on the current soft-fork state.
+		csvState, err := b.deploymentState(node.parent, chaincfg.DeploymentCSV)


I don't believe it's safe to use node.parent here. The calcSequenceLock function is called by the exported CalcSequenceLock with b.bestNode. When the chain is very first loaded, the only thing it has is the best node (no parents are loaded yet). In that case, this will be incorrect since it would be called with nil.

The getPrevNodeFromNode func needs to be used to ensure it's loaded if needed.

Nice catch, fixed to use getPrevFromNode to ensure the parent node is accessed/loaded safely.

davecgh · 2016-12-07T16:47:59Z

rpctest/rpc_harness.go

+// TearDown method for more details.
+//
+// NOTE: This MUST be called with the harnessStateMtx held.
+func (h *Harness) tearDown() error {


This will end up conflicting when you rebase. I had to do something very similar with the additional BIP0009 integration tests in order to prevent Travis from failing. Sorry for the overlap!

No worries, I removed my commit fixing the race condition as a part of my latest rebase.

Roasbeef · 2017-03-09T23:00:46Z

Rebased over the recent changes to the indexing within the blockchain package.

afk11 · 2017-04-12T10:48:16Z

Hey all, me again. CSV has been active on mainnet for over a year now, what would be the holdup on this PR?

davecgh · 2017-02-09T01:10:11Z

blockchain/thresholdstate.go

+	return state, err
+}
+
+// depolymentState returns the current rule change threshold for a given


depolymentState -> deploymentState

davecgh · 2017-05-10T03:37:48Z

Other than that final comment typo and squashing the review commits down into the respective commits, this is ready to go. I've extensively tested it and reviewed it multiple times. Fantastic work!

Roasbeef · 2017-05-10T06:26:38Z

Thanks @davecgh!

I've squashed down the review commits. Very excited to see this get in!

This commit adds BIP-9 deployment parameters for all registered networks for the CSV soft-fork package. The mainnet and testnet parameters have been set in accordance to the finalized BIPs. For simnet, and the regression net, the activation date is back-dated in order to allow signaling for the soft-fork at any time. Additionally the expiration time for simnet and regrets has been set to math.MaxInt64, meaning they’ll never expire.

This commit publicly exports the CreateBlock function as it can be very useful for generating blocks for tests. Additionally, the behavior of the function has been modified slightly to build off of the genesis block for the specified chain if the `prevBlock` paramter is nil.

This commit modifies the existing block validation logic to examine the current version bits state of the CSV soft-fork, enforcing the new validation rules (BIPs 68, 112, and 113) accordingly based on the current `ThesholdState`.

davecgh

OK

dajohi mentioned this pull request Oct 19, 2016

txscript: Implement CheckSequenceVerify (BIP0112) #628

Merged

davecgh mentioned this pull request Oct 19, 2016

blockchain: Remove isMajorityVersion code. #785

Merged

dajohi added this to the 0.13.0 milestone Oct 23, 2016

davecgh mentioned this pull request Oct 26, 2016

rpctest: Make subpackage of integration. #815

Merged

davecgh requested changes Oct 27, 2016

View reviewed changes

davecgh requested changes Oct 28, 2016

View reviewed changes

Roasbeef force-pushed the csv-soft-fork branch 3 times, most recently from 60a43c2 to 906ae4c Compare October 29, 2016 00:42

davecgh requested changes Oct 29, 2016

View reviewed changes

davecgh reviewed Oct 29, 2016

View reviewed changes

Roasbeef force-pushed the csv-soft-fork branch 2 times, most recently from 49fb6c8 to ad240da Compare November 3, 2016 01:42

Roasbeef mentioned this pull request Nov 7, 2016

add support for getblockchaininfo btcsuite/btcrpcclient#99

Merged

stevenroose reviewed Nov 8, 2016

View reviewed changes

davecgh reviewed Nov 10, 2016

View reviewed changes

Roasbeef force-pushed the csv-soft-fork branch from c8b0e59 to fb25069 Compare November 13, 2016 04:18

Roasbeef force-pushed the csv-soft-fork branch from fb25069 to fb20237 Compare December 1, 2016 01:08

davecgh requested changes Dec 1, 2016

View reviewed changes

Roasbeef force-pushed the csv-soft-fork branch from 94ccbaf to f122cfb Compare December 2, 2016 04:06

davecgh requested changes Dec 2, 2016

View reviewed changes

davecgh requested changes Dec 3, 2016

View reviewed changes

Roasbeef force-pushed the csv-soft-fork branch from c9b26c6 to 1943e65 Compare December 7, 2016 02:19

davecgh requested changes Dec 7, 2016

View reviewed changes

Roasbeef force-pushed the csv-soft-fork branch from bccfd4d to 0674f94 Compare December 12, 2016 02:48

Roasbeef mentioned this pull request Dec 17, 2016

Implement BIPs 141, 142, 143, 145 and 147 (Segregated Witness) #656

Merged

12 tasks

Roasbeef force-pushed the csv-soft-fork branch 2 times, most recently from 11efbf1 to 4403f7a Compare March 9, 2017 23:00

Roasbeef force-pushed the csv-soft-fork branch from 4403f7a to 53676ae Compare March 9, 2017 23:07

davecgh reviewed May 10, 2017

View reviewed changes

Roasbeef force-pushed the csv-soft-fork branch 3 times, most recently from b804547 to f6982cc Compare May 10, 2017 06:01

Roasbeef added 6 commits May 10, 2017 15:36

blockchain: update calcSequenceLocks test to activate CSV first

d767af1

rpcserver: add csv case for getblockchaininfo

7d0b808

integration: add integration tests for the CSV-package soft-fork

8972841

Roasbeef force-pushed the csv-soft-fork branch from f6982cc to 8972841 Compare May 10, 2017 22:37

davecgh approved these changes May 10, 2017

View reviewed changes

davecgh merged commit 8972841 into btcsuite:master May 10, 2017

Implement the CSV-Package soft-fork via BIP0009 VersionBits #775

Implement the CSV-Package soft-fork via BIP0009 VersionBits #775

Conversation

Roasbeef commented Sep 27, 2016 • edited

Roasbeef commented Sep 27, 2016

davecgh commented Oct 27, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh Oct 28, 2016 • edited

Choose a reason for hiding this comment

davecgh Oct 29, 2016 • edited

Choose a reason for hiding this comment

davecgh Oct 29, 2016 • edited

Choose a reason for hiding this comment

Roasbeef commented Nov 8, 2016

davecgh commented Nov 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh commented Dec 1, 2016

davecgh commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh Dec 1, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef Dec 2, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef Dec 7, 2016 • edited

Choose a reason for hiding this comment

davecgh Dec 3, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh Dec 3, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Sep 27, 2016 •

edited

davecgh commented Oct 27, 2016 •

edited

davecgh Oct 28, 2016 •

edited

davecgh Oct 29, 2016 •

edited

davecgh Oct 29, 2016 •

edited

davecgh commented Nov 8, 2016 •

edited

davecgh Dec 1, 2016 •

edited

Roasbeef Dec 2, 2016 •

edited

Roasbeef Dec 7, 2016 •

edited

davecgh Dec 3, 2016 •

edited

davecgh Dec 3, 2016 •

edited