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

Open
wants to merge 25 commits into
from

Projects

None yet

5 participants

@Roasbeef
Member
Roasbeef commented Apr 8, 2016 edited

This PR makes several modifications across btcd, and many of the sub-packages in order to add compatibility for the upcoming Segregated Witness soft-fork. As the set of changes is rather expansive, this PR has been split up into several commits to in order to facilitate thorough code review of each change.

This PR currently implements the changes dictated by the following BIPS:

BIP 145 has not yet been fully implemented. It is amongst the pending TODO list items remaining for this PR before the WIP tag is removed.

NOTE: This is dependent on the following PR in btcutil: btcsuite/btcutil#75

As of the writing of this PR description, with this patch set btcd is able to successful sync up, and correctly validate all blocks and witness commitments on segnet4. This PR is still being extensively tested in order to ensure all new consensus critical decision related to transactions, blocks, and script validation are functioning properly.

The changes across all packages are briefly summarized below:

  • blockchain:
    • New functions have been added to calculate block, and transaction "cost".
    • The previous BuildMerkleTreeStore function is now able to calculate the witness merkle root. This allowed the creation of new functions to extract, and validate the witness commitment.
    • The new "cost" metrics are now used when validating blocks, and transactions.
    • The txscript.ScriptVerifyWitness is now used by default when validating transaction scripts. Once BIP9 support is in place, this will instead be gated by the appropriate versionbits stage threshold.
    • Script validation performed by txValidator now also uses the new HashCache in order to more efficiently validate inputs spending witness program outputs.
    • When checking BIP34 for blocks below height 16, we now attempt to extract just a small integer. This behavior never came when BIP34 was rolled out, as it was deployed around height 100k or so.
  • chaincfg:
    • The parameters for segnet4 have been added, along with the new address bytes for the new standard output types.
  • database:
    • Blocks, and transactions are always stored using the witness encoding if they have witness data. Additionally, all blocks are attempted to be decoded using the witness encoding.
  • peer:
    • Detecting if a peer supports segwit has been added.
    • A new queue function QueueMessageWithEncoding` has been added in order to allow callers to specify the encoding type to use when serializing messages. This was required in order to allow callers to selectively send witness data to peers.
    • The MaxProtocolVersion has been increased to 70012 in order to signal to other peers we supports segwit.
  • txscript:
    • Validation logic for p2wsh, p2wkh, and nested p2sh outputs have been added. In the process txscript.Engine now takes an optional HashCache as well as the input value of the output being spent.
    • Support for the new sighash digest algorithm defined in BIP143 has been added. In order to make full use of the new optimizations, a new caching structure, the HashCache has been introduced which caches partial sighashes to be re-used within the mempool, and block across all concurrent script validation goroutines.
    • New functions have been added to calculate sig op cost using the new sig op calculation introduced as part of segwit.
    • Several new utility functions have been introduced for analyzing, creating, and classifying the new standard output types.
  • wire:
    • The new SFNodeWitness service bit has been added along with new inventory types for segwit, and and increase in the MaxBlockPayload size.
    • New methods have been added to support the new encoding format for transaction with witness data. In the process, a new field has been added to the wire.Message interface in order to cleanly facilitate optinoally encoding or decoding the witness data. Finally, a new methods has been added to wire.MsgTx to compute the wtxid, and ascertain if a transaction has any witness data.
  • btcd itself:
    • Only peers supporting segwit are chosen to be the syncPeer.
    • All wire.MsgGetData sent will request with the new witness inv types by default. Additionally the new inv types are no longer ignored when received by other peers (TODO(roasbeef): thought they were only to be used within getdata?).
    • The new cost metrics are used across the mempool when validating transactions.
    • Peers created by the server advertise support for segwit.

Thanks to @T909 for the initial work which laid the base for much of this PR, along with coming up with the initial idea for the HashCache.

TODO

  • Implement BIP 145 support
  • Update the rpcserver, and btcjson to display the new transactions, and block related data. The display changes can be summarized as: display of witness data needs to be added to the transaction display, add a new hash field to show the wtxid, add vsize (virtual size) to transaction display, show cost for blocks.
  • Significantly improve test coverage across all packages which have been modified, or had new functionality introduced.
  • Integrate Bitcoin Core's new tests within script_tests.json
  • Integrate Bitcoin Core's segwit transaction tests within tx_valid.json, and tx_invalid.json
  • Add new, proper error messages within txscript.Engine to be returned when a new segwit specific error is encountered. At the moment, it returns some ad-hoc errors I created to facilitate debugging.
  • Clean up the new witness validation logic in txscript.Engine.
  • Update cpuminer.go, and the GBT logic to account for the new cost metrics.
  • Add test coverage for the new sig op counting code, and witness commitment validation code.
  • Once BIP 9 has been implemented for btcd, guard new seg wit behavior with a checking of the versionbits state.
  • Add passive partial sighash eviction to HashCache
  • Modify the addrindex, and txindex to enable support for fully indexing blocks with witness data.

This change is Reviewable

@davecgh davecgh and 1 other commented on an outdated diff Apr 8, 2016
chaincfg/genesis.go
+ 0x32, 0x95, 0xc0, 0x10, 0xf5, 0x5f, 0xfb, 0x18,
+})
+
+// segNetGenesisMerkleRoot is the hash of the first transaction in the genesis
+// block for the segwit test network. It is the same as the merkle root
+// for the main network.
+var segNetGenesisMerkleRoot = genesisMerkleRoot
+
+// segNetGenesisBlock defines the genesis block of segwit testnet v4.
+var segNetGenesisBlock = wire.MsgBlock{
+ Header: wire.BlockHeader{
+ Version: 1,
+ PrevBlock: wire.ShaHash{}, // 0000000000000000000000000000000000000000000000000000000000000000
+ MerkleRoot: segNetGenesisMerkleRoot, // 4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b
+ Timestamp: time.Unix(1452831101, 0), // 2016-01-15 04:11:41 +0000 UTC
+ Bits: 0x1e01ffff, // ?? [000007ffff000000000000000000000000000000000000000000000000000000]
@davecgh
davecgh Apr 8, 2016 Member

?? == 503447551

@davecgh
davecgh Apr 8, 2016 Member

Also, the bits equate to 000001ffff000000000000000000000000000000000000000000000000000000 as opposed to the value in the comment.

@Roasbeef
Roasbeef Apr 10, 2016 Member

Fixed. The value in the comment was left over from segnet3.

@davecgh davecgh and 1 other commented on an outdated diff Apr 8, 2016
chaincfg/params.go
@@ -34,6 +34,10 @@ var (
// simNetPowLimit is the highest proof of work value a Bitcoin block
// can have for the simulation test network. It is the value 2^255 - 1.
simNetPowLimit = new(big.Int).Sub(new(big.Int).Lsh(bigOne, 255), bigOne)
+
+ // segnet4 has a pow limit of 1e01ffff which is 19 0-bits in front, I think
@davecgh
davecgh Apr 8, 2016 Member

It's 23 0-bits, but I'd prefer this to be consistent with the other comments:

    // segnet4PowLimit is the highest proof of work value a Bitcoin block
    // can have for the segregated witness test network.  It is the value
    // 2^233 - 1.

The math checks out: 2^233-1 == 0x000001ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff which when compacted to the Bits representation is 0x1e01ffff as expected.

@Roasbeef
Roasbeef Apr 10, 2016 Member

Fixed.

@davecgh davecgh and 1 other commented on an outdated diff Apr 8, 2016
chaincfg/params.go
@@ -272,6 +276,57 @@ var TestNet3Params = Params{
HDCoinType: 1,
}
+// SegNet4Params defines the network parameters for the test Bitcoin network
+// (version 4). Not to be confused with the regression test network, this
+// network is sometimes simply called "testnet".
@davecgh
davecgh Apr 8, 2016 Member

This is "segnet", not "testnet", right? That is unless it's replacing testnet version 3.

@Roasbeef
Roasbeef Apr 10, 2016 Member

Correct, removed the left over comment fragment from a copy-paste.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
@@ -457,6 +458,10 @@ func loadConfig() (*config, []string, error) {
activeNetParams = &simNetParams
cfg.DisableDNSSeed = true
}
+ if cfg.SegNet4 {
+ numNets++
+ activeNetParams = &segNet4Params
+ }
if numNets > 1 {
str := "%s: The testnet, regtest, and simnet params can't be " +
@davecgh
davecgh Apr 9, 2016 Member

Need to update the error for the new network.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
@@ -55,6 +55,13 @@ var simNetParams = params{
rpcPort: "18556",
}
+// segNet4Params contains parameters specific to the simulation test network
@davecgh
davecgh Apr 9, 2016 Member

to the simulation test ... -> to the segregated witness test ...

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
wire/msgtx.go
@@ -199,6 +199,20 @@ func (msg *MsgTx) TxSha() ShaHash {
return DoubleSha256SH(buf.Bytes())
}
+// WTxSha generates the ShaHash of the transaction serialized according to the
+// new witness serialization defined in BIP0141. The final output is used within
+// the Segregated Witness commitment of all the witnesses within a block. If a
+// transaction has no witness data, then the witness sha, is the same as its txid.
+func (msg *MsgTx) WitnessSha() ShaHash {
@davecgh
davecgh Apr 9, 2016 Member

Can we call this WitnessHash?

I realize Sha is used in the other things and this is just being consistent, but I'd like to eventually change the funcs to be be XHash because I don't like that the algorithm is baked into the name since it might not always be using SHA.

@Roasbeef
Roasbeef Apr 10, 2016 Member

Fair point, I've changed the function name to WitnessHash.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
wire/msgtx.go
@@ -255,6 +291,30 @@ func (msg *MsgTx) BtcDecode(r io.Reader, pver uint32) error {
return err
}
+ // A count of zero (meaning no TxIn's to the uninitiated) indicates
+ // this is a transaction with witness data.
+ var flag [1]byte
+ if count == 0 && pver == WitnessVersion {
+ // Next, we need to read the flag, which is a single byte.
+ if _, err = r.Read(flag[:]); err != nil {
@davecgh
davecgh Apr 9, 2016 Member

This needs to be io.ReadFull(r, flag[:]). Regular r.Read(flag[:]) does not enforce the byte to be read if it's not ready. That condition is also not an error (since the number of bytes read will simply be 0) and therefore this code, which is not checking the number of read bytes, could end up failing incorrectly when it checks the flag below and finds it to be 0x00 instead of 0x01 due to it not actually being read.

@Roasbeef
Roasbeef Apr 10, 2016 Member

Great catch, I've changed the line to instead use io.ReadFull.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
wire/msgtx.go
+ // this is a transaction with witness data.
+ var flag [1]byte
+ if count == 0 && pver == WitnessVersion {
+ // Next, we need to read the flag, which is a single byte.
+ if _, err = r.Read(flag[:]); err != nil {
+ return err
+ }
+
+ // At the moment, the flag MUST be 0x01. In the future other
+ // flag types may be supported.
+ if flag[0] != 0x01 {
+ str := fmt.Sprintf("witness tx but flag byte is %x", flag)
+ return messageError("MsgTx.BtcDecode", str)
+ }
+
+ // Wit the Segregated Witness specific fields decoded, we can
@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
wire/msgtx.go
@@ -255,6 +291,30 @@ func (msg *MsgTx) BtcDecode(r io.Reader, pver uint32) error {
return err
}
+ // A count of zero (meaning no TxIn's to the uninitiated) indicates
+ // this is a transaction with witness data.
+ var flag [1]byte
+ if count == 0 && pver == WitnessVersion {
@davecgh
davecgh Apr 9, 2016 Member

Shouldn't this be looking at msg.Version >= 2 rather than pver and a bogus WitnessVersion?

@Roasbeef
Roasbeef Apr 10, 2016 Member

See comments below concerning the encoding type.

@davecgh davecgh commented on the diff Apr 9, 2016
wire/msgtx.go
+
+ // At the moment, the flag MUST be 0x01. In the future other
+ // flag types may be supported.
+ if flag[0] != 0x01 {
+ str := fmt.Sprintf("witness tx but flag byte is %x", flag)
+ return messageError("MsgTx.BtcDecode", str)
+ }
+
+ // Wit the Segregated Witness specific fields decoded, we can
+ // now read in the actual txin count.
+ count, err = ReadVarInt(r, pver)
+ if err != nil {
+ return err
+ }
+ }
+
// Prevent more input transactions than could possibly fit into a
// message. It would be possible to cause memory exhaustion and panics
// without a sane upper bound on this count.
@davecgh
davecgh Apr 9, 2016 Member

Need to verify if maxTxInPerMessage is still a constant under a witness-style transaction. The same goes for maxTxOutPerMessage. It might not have changed, but need to double check that since the tx structure did change.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
wire/msgtx.go
@@ -300,6 +360,33 @@ func (msg *MsgTx) BtcDecode(r io.Reader, pver uint32) error {
msg.TxOut[i] = &to
}
+ // If the transaction's flag byte isn't 0x00 at this point, then one
+ // or more of its inputs has accompanying witness data.
+ if flag[0] != 0 && pver == WitnessVersion {
@davecgh
davecgh Apr 9, 2016 Member

Same here regarding msg.Version >= 2

@Roasbeef
Roasbeef Apr 10, 2016 Member

See comments below concerning the encoding type.

@davecgh davecgh commented on the diff Apr 9, 2016
wire/msgtx.go
+ // If the transaction's flag byte isn't 0x00 at this point, then one
+ // or more of its inputs has accompanying witness data.
+ if flag[0] != 0 && pver == WitnessVersion {
+ for _, txin := range msg.TxIn {
+ // For each input, the witness is encoded as a stack
+ // with one or more items. Therefore, we first read a
+ // varint which encodes the number of stack items.
+ witCount, err := ReadVarInt(r, pver)
+ if err != nil {
+ return err
+ }
+
+ // Then for witCount number of stack items, each item
+ // has a varint length prefix, followed by the witness
+ // item itself.
+ txin.Witness = make([][]byte, witCount)
@davecgh
davecgh Apr 9, 2016 Member

The witCount needs to be checked against some type of sane maximum based on the max possible that could fit in a transaction. Otherwise it would be trivial for an attacker to craft a message with a huge varint encoded here to force a massive allocation of memory and crash the process.

Remember the first rule of reading data from external sources is you can never trust it.

@Roasbeef
Roasbeef Apr 10, 2016 Member

Excellent suggestion. I've changed this fragment to limit the possible size of witCount.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
wire/msgtx.go
+ if flag[0] != 0 && pver == WitnessVersion {
+ for _, txin := range msg.TxIn {
+ // For each input, the witness is encoded as a stack
+ // with one or more items. Therefore, we first read a
+ // varint which encodes the number of stack items.
+ witCount, err := ReadVarInt(r, pver)
+ if err != nil {
+ return err
+ }
+
+ // Then for witCount number of stack items, each item
+ // has a varint length prefix, followed by the witness
+ // item itself.
+ txin.Witness = make([][]byte, witCount)
+ for j := uint64(0); j < witCount; j++ {
+ witPush, err := ReadVarBytes(r, pver, MaxMessagePayload,
@davecgh
davecgh Apr 9, 2016 Member

I don't think MaxMessagePayload is a correct value here. At the very most, it would be limited by the maximum block size. A more accurate value would the maximum block size minus the minimum possible bytes it could take to encode the other fields assuming none of them have any data.

@Roasbeef
Roasbeef Apr 10, 2016 Member

I've opted to limit the maximum size of a witness element we'll read to just over the maximum size allowed for a single stack item. The rationale is that a transaction with a witness element over that size would be rejected during script execution anyway.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
wire/msgtx.go
@@ -326,6 +413,13 @@ func (msg *MsgTx) Deserialize(r io.Reader) error {
return msg.BtcDecode(r, 0)
}
+// DeserializeWitness decodes a transaction from r into the receiver, where the
+// encoded transaction format within r may possibly utilize the new serialization
+// format created to encode transactions bearing witness data within inputs.
+func (msg *MsgTx) DeserializeWitness(r io.Reader) error {
+ return msg.BtcDecode(r, WitnessVersion)
@davecgh
davecgh Apr 9, 2016 Member

As mentioned above the transaction version should be used instead and then there is no need for a separate function. Plus, using the WitnessVersion is incorrect since DeserializeX, as mentioned elsewhere, is intended for the long term stable storage format for the database to which the network protocol version must not apply. It's fine (and preferable) if they happen to be the same encoding, but the distinction must be kept because future versions can change the serialization format on the wire, but it still must be able to read whatever the database format is.

@Roasbeef
Roasbeef Apr 10, 2016 Member

First, note that this is an outdated diff. In the latest version BtcDecode has gained an additional parameter which controls the encoding type.

Using the transaction version is insufficient, because this function might be deserializing the transaction encoded without the witness data because the transaction has been received from a non-upgraded peer. The server will never request the transaction from such a peer since it detects if it is witness enabled before sending the getdata message.

@davecgh
davecgh Apr 10, 2016 Member

Right, I realized you changed it as I got through the diffs and also read through the BIPs after making this comment, so I understand why the extra parameter is needed. Feel free to ignore this.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
wire/msgtx.go
@@ -338,6 +432,22 @@ func (msg *MsgTx) BtcEncode(w io.Writer, pver uint32) error {
return err
}
+ // If the pver is set to WitnessVersion, and the Flags field for the
+ // MsgTx aren't 0x00, then this indicates the transaction is to be
+ // encoded using the new witness inclusionary structure defined in
+ // BIP0141.
+ if pver == WitnessVersion {
@davecgh
davecgh Apr 9, 2016 Member

Again, this doesn't seem like a correct way to handle it. I would think it should be looking at msg.Version >= 2.

@Roasbeef
Roasbeef Apr 10, 2016 Member

Note that this is an out dated diff. Looks I goofed a bit re-organizing the commits. I now utilize a separate wire.MessageEncoding field to gate this behavior.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
wire/msgtx.go
@@ -338,6 +432,22 @@ func (msg *MsgTx) BtcEncode(w io.Writer, pver uint32) error {
return err
}
+ // If the pver is set to WitnessVersion, and the Flags field for the
+ // MsgTx aren't 0x00, then this indicates the transaction is to be
+ // encoded using the new witness inclusionary structure defined in
+ // BIP0141.
+ if pver == WitnessVersion {
+ // After the txn's Version field, we include two additional
+ // bytes specific to the witness encoding. The first byte is an
+ // always 0 marker byte, which allows decoders to distinguish a
+ // serialized transaction with witnesses from a regular (legacy)
+ // one. The second byte is the Flag field, which at the moment is
+ // always 0x01, but way be extended in the future to accommodate
+ // auxiliary non-commited fields.
+ w.Write([]byte{0x00})
@davecgh
davecgh Apr 9, 2016 Member

Errors are being ignored here. Bad!

@davecgh
davecgh Apr 9, 2016 Member

Also, I'd do a single write with a constant that has []byte{0x00, 0x01} along with the appropriate commenting to explain it.

@Roasbeef
Roasbeef Apr 10, 2016 Member

Bad indeed :)

I now properly check the error. I've also condensed it down to a single write with both bytes.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
@@ -400,6 +402,14 @@ func (sp *serverPeer) OnVersion(p *peer.Peer, msg *wire.MsgVersion) {
}
}
+ // Determine if the peer would like to receive witness data with
+ // transactions, or not.
+ if p.ProtocolVersion() >= wire.WitnessVersion &&
+ p.Services()&wire.SFNodeWitness == wire.SFNodeWitness {
+
+ sp.witnessEnabled = true
@davecgh
davecgh Apr 9, 2016 Member

This will race since it's not protected by a mutex.

@Roasbeef
Roasbeef Apr 10, 2016 Member

Fixed, all variable access are now properly protected by a mutex.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
txscript/engine.go
@@ -321,6 +330,72 @@ func (vm *Engine) Step() (done bool, err error) {
// Set stack to be the stack from first script minus the
// script itself
vm.SetStack(vm.savedFirstStack[:len(vm.savedFirstStack)-1])
+ } else if (vm.scriptIdx == 1 && vm.witness) ||
+ (vm.witness && vm.bip16 && vm.scriptIdx == 2) { // Nested P2SH.
+ // TODO(roasbeef): check that sigscript is just a push
+ vm.scriptIdx++
+
+ witness := vm.tx.TxIn[vm.txIdx].Witness
+ if vm.witnessVersion == 0 {
+ switch len(vm.witnessProgram) {
+ case 20: // P2WKH
@davecgh
davecgh Apr 9, 2016 Member

Please use constants for this and the case below.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
txscript/engine.go
@@ -198,6 +205,8 @@ func (vm *Engine) curPC() (script int, off int, err error) {
return vm.scriptIdx, vm.scriptOff, nil
}
+//func (vm *Engine) verifyWitnessProgram(uint version, wi
@davecgh
davecgh Apr 9, 2016 Member

This appears to be left over. I suspect the intention was to factor the code out of Step which I think makes sense.

@Roasbeef
Roasbeef Apr 10, 2016 Member

Yeah this was left over as an un-finished thought. I've extracted out the new code in Step to a separate function.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
txscript/engine.go
+ // Now we'll resume execution as if it were a
+ // regular p2pkh transaction.
+ pkScript, err := payToPubKeyHashScript(vm.witnessProgram)
+ if err != nil {
+ return false, err
+ }
+ pops, err := parseScript(pkScript)
+ if err != nil {
+ return false, err
+ }
+
+ // Set the stack to the provided witness
+ // stack, then append the pkScript
+ // generated above as the next script to execute.
+ vm.scripts = append(vm.scripts, pops)
+ vm.SetStack(witness)
@davecgh
davecgh Apr 9, 2016 Member

Doesn't this need to include the stack data from vm.savedFirstStack with the witness at the top of the stack (appended)? Otherwise it would wipe that data out. We need to carefully compare what Core does here because it's possible it's just wiping the stack out too, but it seems to me that it should be appended because a nested P2SH witness program could have other data pushed.

@Roasbeef
Roasbeef Apr 10, 2016 Member

This case will only be executed for p2wkh transactions. During core's execution, they start with an empty stack, then assign this stack variable to point to the transaction's included witness stack (vm.tx.TxIn[vm.txIdx.Witness). This verification is done locally within a separate function so there was nothing on the stack before hand. Therefore I feel this sections is correct for p2wkh programs. But as you point out, it's very important that we verify so.

The witness program for nested P2SH outputs must be the variant which pushes the 32-byte hash. So it will never execute this case which is reserved for p2wkh programs. To my current knowledge, when core validates nested P2SH outputs, it again calls the VerifyWitnessProgram function which begins with a fresh empty stack, assigning the stack to the transaction's provided witness data. Therefore once again the execution starts with solely the transaction's witness data on the stack. However, once the program execution concludes, the stack is resized forcing the stack to only contain a single value. This value is then checked for "truthyness", as well as the clean stack assertion.

My opinions of correct are drawn only from the fact that this branch currently sync successfully, and my my manual execution of p2wkh, p2wsh, and nested p2sh programs. However, of course there may be lingering edge cases this code doesn't yet address. Once I finish the lingering TODO's all add the new script, and transaction tests to ensure the code at least passes those additionally.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
txscript/engine.go
+ witnessHash := fastsha256.Sum256(pkScript)
+ if !bytes.Equal(witnessHash[:], vm.witnessProgram) {
+ return false, fmt.Errorf("witness program mismatch")
+ }
+
+ pops, err := parseScript(pkScript)
+ if err != nil {
+ return false, err
+ }
+
+ // The hash matched successfully, so
+ // use the witness as the stack, and
+ // set the pkScript to be the next script
+ // executed.
+ vm.scripts = append(vm.scripts, pops)
+ vm.SetStack(witness[:len(witness)-1])
@davecgh
davecgh Apr 9, 2016 Member

Same thing here regarding vm.savedFirstStack.

@Roasbeef
Roasbeef Apr 10, 2016 Member

See comment above.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
blockchain/merkle.go
@@ -141,3 +140,91 @@ func BuildMerkleTreeStore(transactions []*btcutil.Tx, witness bool) []*wire.ShaH
return merkles
}
+
+// ExtractWitnessCommitment attempts to locate, and return the witness
+// commitment for a block. The witness commitment is of the form:
+// SHA256(witness root || witness nonce).The function additionally returns a
@davecgh
davecgh Apr 9, 2016 Member

Missing spaces after period.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
blockchain/merkle.go
+func ExtractWitnessCommitment(tx *btcutil.Tx) ([]byte, bool) {
+ var witnessCommitment []byte
+
+ // The witness commitment *must* be located within one of the coinbase
+ // transaction's outputs.
+ if !IsCoinBase(tx) {
+ return witnessCommitment, false
+ }
+
+ msgTx := tx.MsgTx()
+ witFound := false
+ for i := len(msgTx.TxOut) - 1; i >= 0; i-- {
+ // The public key script that contains the witness commitment
+ // must shared a prefix with the WitnessMagicBytes, and be at
+ // least 38 bytes.
+ if bytes.HasPrefix(msgTx.TxOut[i].PkScript, WitnessMagicBytes) &&
@davecgh
davecgh Apr 9, 2016 Member

This is more efficient to check the length of the script before comparing the bytes. Also, I'd create a local for pkScript in order to avoid the overhead of checking the array bounds multiple times.

    for i := len(msgTx.TxOut) - 1; i >= 0; i-- {
        // The public key script that contains the witness commitment
        // must shared a prefix with the WitnessMagicBytes, and be at
        // least 38 bytes.
        pkScript := msgTx.TxOut[i].PkScript
        if len(pkScript) >= 38 &&
            bytes.HasPrefix(pkScript, WitnessMagicBytes) {

            witnessCommitment = pkScript[6:38]
            witFound = true
        }
    }
@davecgh
davecgh Apr 9, 2016 Member

Also, I'd prefer named constants for all of these magic offsets and lengths.

@davecgh
davecgh Apr 9, 2016 Member

Probably doesn't matter, but I'm not really sure returning whether or not the witness commitment was found is worth it here. It's easy enough to do if len(witnessCommitment) == 0 { /* not found */ }

@Roasbeef
Roasbeef Apr 10, 2016 Member

I've modified the code with your suggestions, including adding some new constants for the magic offsets, and lengths.

As you've probably seen within ValidateWitnessCommitment, if the coinbase doesn't have something which looks like a witness commitment within any of its outputs, then if the transactions within the block contain any witness data, then the block itself is invalid. Therefore it's necessary to also return if the commitment was found or not. Hmm, but I guess the function could simply return nil if the commitment was not found.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
blockchain/merkle.go
+// ValidateWitnessCommitment validates the witness commitment (if any) found
+// within the coinbase transaction of the passed block.
+func ValidateWitnessCommitment(blk *btcutil.Block) error {
+ // TODO(roasbeef): check should be conditional on witness activation
+ coinbaseTx := blk.Transactions()[0]
+ witnessCommitment, witnessFound := ExtractWitnessCommitment(coinbaseTx)
+
+ // If we can't find a witness commitment in any of the coinbase's
+ // outputs, then the block MUST NOT contain any transactions with
+ // witness data.
+ if !witnessFound {
+ for _, tx := range blk.Transactions() {
+ msgTx := tx.MsgTx()
+ if len(msgTx.TxIn[0].Witness) != 0 {
+ str := fmt.Sprintf("block contains transaction with witness" +
+ " data, yet not witness commitment present")
@davecgh
davecgh Apr 9, 2016 Member

s/not/no/

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
blockchain/merkle.go
+ str := fmt.Sprintf("block contains transaction with witness" +
+ " data, yet not witness commitment present")
+ return ruleError(ErrUnexpectedWitness, str)
+ }
+ }
+ return nil
+ }
+
+ // If the witness commitment can be located, then the coinbase
+ // transaction MUST have exactly one witness element within its witness
+ // data. Additionally, that element must be exactly 32-byte.
+ if len(coinbaseTx.MsgTx().TxIn[0].Witness) != 1 ||
+ len(coinbaseTx.MsgTx().TxIn[0].Witness[0]) != 32 {
+ str := fmt.Sprintf("the coinbase transaction must have exactly " +
+ "one item within its witness stack, and that item " +
+ "should be exactly 32-bytes")
@davecgh
davecgh Apr 9, 2016 Member

s/should/must/

@davecgh
davecgh Apr 9, 2016 Member

Please separate the error messages here so that they contain more details such as:

    // At this point the block contains a witness commitment, so the
    // coinbase transaction MUST have exactly one witness element within
    // its witness data and that element must be exactly
    // CoinbaseWitnessDataLen bytes.
    coinbaseWitness := coinbaseTx.MsgTx().TxIn[0].Witness
    if len(coinbaseWitness) != 1 {
        str := fmt.Sprintf("the coinbase transaction has %d items in "+
            "its witness stack when only one is allowed",
            len(coinbaseWitness))
        return ruleError(ErrInvalidWitnessCommitment, str)
    }
    witnessNonce := coinbaseWitness[0]
    if len(witnessNonce) != CoinbaseWitnessDataLen {
        str := fmt.Sprintf("the coinbase transaction witness nonce "+
            "has %d bytes when it must be %d bytes",
            len(witnessNonce), CoinbaseWitnessDataLen)
        return ruleError(ErrInvalidWitnessCommitment, str)
    }

Also notice I introduced a constant for that "32" magic and I created a witnessNonce variable which can be used below to make the code clearer.

@Roasbeef
Roasbeef Apr 10, 2016 Member

Done, I've used the code fragment you provided, creating a new const for CoinbaseWitnessDataLen. As a result, the code is much clearer, and more explicit, thanks!

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
blockchain/merkle.go
+ }
+ return nil
+ }
+
+ // If the witness commitment can be located, then the coinbase
+ // transaction MUST have exactly one witness element within its witness
+ // data. Additionally, that element must be exactly 32-byte.
+ if len(coinbaseTx.MsgTx().TxIn[0].Witness) != 1 ||
+ len(coinbaseTx.MsgTx().TxIn[0].Witness[0]) != 32 {
+ str := fmt.Sprintf("the coinbase transaction must have exactly " +
+ "one item within its witness stack, and that item " +
+ "should be exactly 32-bytes")
+ return ruleError(ErrInvalidWitnessCommitment, str)
+ }
+
+ // Finally, with the preliminary checks out of the way, we can finally
@davecgh
davecgh Apr 9, 2016 Member

finally repeated.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
blockchain/merkle.go
+ str := fmt.Sprintf("the coinbase transaction must have exactly " +
+ "one item within its witness stack, and that item " +
+ "should be exactly 32-bytes")
+ return ruleError(ErrInvalidWitnessCommitment, str)
+ }
+
+ // Finally, with the preliminary checks out of the way, we can finally
+ // check if the extracted witnessCommitment is equal to:
+ // SHA256(witnessMerkleRoot || witnessNonce). Where witnessNonce is the
+ // coinbase transaction's only witness item.
+ witnessMerkleTree := BuildMerkleTreeStore(blk.Transactions(), true)
+ witnessMerkleRoot := witnessMerkleTree[len(witnessMerkleTree)-1]
+
+ witnessPreimage := make([]byte, 64)
+ copy(witnessPreimage[:], witnessMerkleRoot[:])
+ copy(witnessPreimage[32:], coinbaseTx.MsgTx().TxIn[0].Witness[0])
@davecgh
davecgh Apr 9, 2016 Member

With the changes recommended above.

    copy(witnessPreimage[32:], witnessNonce)
@davecgh davecgh commented on the diff Apr 9, 2016
txscript/script.go
@@ -69,6 +69,87 @@ func IsPayToScriptHash(script []byte) bool {
return isScriptHash(pops)
}
+// isWitnessScriptHash returns true if the passed script is a
+// pay-to-witness-script-hash transaction, false otherwise.
+func isWitnessScriptHash(pops []parsedOpcode) bool {
@davecgh
davecgh Apr 9, 2016 Member

Please move this and all of the others that represent standard scripts into standard.go since this is not consensus critical code.

@davecgh
davecgh Apr 9, 2016 Member

Also, these should really only be true depending on the witness script version which I don't see anything for properly detecting and handling that. I suspect this function, and the others in the same vain, should be taking the witness version and checking it against 0.

The BIP says:

If the version byte is 0, and the witness program is 32 bytes:

Followed by:

If the version byte is 1 to 16, no further interpretation of the witness program or witness happens, and there is no size restriction for the witness. These versions are reserved for future extensions. 

In other words, currently, it is only a P2WSH for version 0.

@davecgh
Member
davecgh commented Apr 9, 2016

Thanks for all of the work on this. I know it was a lot of it. I also really appreciate the detail put into PR description regarding the changes.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
txscript/hashcache.go
@@ -0,0 +1,101 @@
+// Copyright (c) 2013-2016 The btcsuite developers
@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
txscript/script.go
+ hashType&SigHashNone != SigHashNone {
+ sigHash.Write(sigHashes.HashOutputs[:])
+ } else if hashType&sigHashMask == SigHashSingle { // TODO(roasbeef): verify range above
+ wire.WriteTxOut(&sigHash, 0, 0, txCopy.TxOut[idx])
+ }
+
+ // Finally, write out the transaction's locktime, and the sig hash
+ // type.
+ var bLockTime [4]byte
+ binary.LittleEndian.PutUint32(bLockTime[:], txCopy.LockTime)
+ sigHash.Write(bLockTime[:])
+ var bHashType [4]byte
+ binary.LittleEndian.PutUint32(bHashType[:], uint32(hashType))
+ sigHash.Write(bHashType[:])
+
+ hsh := wire.DoubleSha256SH(sigHash.Bytes())
@davecgh
davecgh Apr 9, 2016 Member
return wire.DoubleSha256(sigHash.Bytes())
@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
txscript/script.go
+ // signed.
+ var bAmount [8]byte
+ binary.LittleEndian.PutUint64(bAmount[:], uint64(amt))
+ sigHash.Write(bAmount[:])
+ var bSequence [4]byte
+ binary.LittleEndian.PutUint32(bSequence[:], txCopy.TxIn[idx].Sequence)
+ sigHash.Write(bSequence[:])
+
+ // If the current signature mode isn't single, or none, then we can
+ // re-use the pre-generated hashoutputs sighash fragment. Otherwise,
+ // we'll serialize and add only the target output index to the signature
+ // pre-image.
+ if hashType&SigHashSingle != SigHashSingle &&
+ hashType&SigHashNone != SigHashNone {
+ sigHash.Write(sigHashes.HashOutputs[:])
+ } else if hashType&sigHashMask == SigHashSingle { // TODO(roasbeef): verify range above
@davecgh
davecgh Apr 9, 2016 Member

This doesn't have the same behavior as the code in the BIP.

In particular, the code in the BIP has:

} else if ((nHashType & 0x1f) == SIGHASH_SINGLE && nIn < txTo.vout.size()) {
    CHashWriter ss(SER_GETHASH, 0);
    ss << txTo.vout[nIn];
    hashOutputs = ss.GetHash();
}
...
// Outputs (none/one/all, depending on flags)
ss << hashOutputs;

The code here does not match for two reasons:

  1. The BIP code ends up adding the hash of the serialized txout to the final calculated signature hash whereas this code is writing the entire serialized txout.
  2. The BIP code will result in hashOutputs being the 32-byte zero hash when idx is out of range whereas this code will write nothing in that case.
@Roasbeef
Roasbeef Apr 10, 2016 Member

Excellent, catch. I've modified this code fragment to more precisely match the code show within the BIP. Once I finish up the lingering TODOs on this PR, I'll add Bitcoin Core's new transaction tests which exercise various sighash combos in ensure this function produces the correct sighash each time.

@davecgh davecgh and 1 other commented on an outdated diff Apr 9, 2016
txscript/script.go
+// if fed an invalid input amount, the real sighash will differ causing the
+// produced signature to be invalid.
+func calcWitnessSignatureHash(subScript []parsedOpcode, sigHashes *TxSigHashes,
+ hashType SigHashType, tx *wire.MsgTx, idx int, amt int64) []byte {
+
+ // As a sanity check, ensure the passed input index for the transaction
+ // is valid.
+ if idx > len(tx.TxIn)-1 {
+ fmt.Printf("calcWitnessSignatureHash error: idx %d but %d txins",
+ idx, len(tx.TxIn))
+ return nil
+ }
+
+ // Copy tha transaction, we don't need to clear any of the sigScripts
+ // since we don't modify or utilize them.
+ txCopy := tx.Copy()
@davecgh
davecgh Apr 9, 2016 Member

There is no longer any reason to copy the transaction since it's not being modified.

@Roasbeef
Roasbeef Apr 10, 2016 Member

Correct, nice catch! This was left over from when I copied the logic over initially. I've been block validation speed with some larger blocks, and removing this along with optimizing the HashCache usage internally resulted in a nice speed up when validating a very large transaction (9k+ inputs).

@Roasbeef
Member

Thanks @davecgh, it sure was! There's just a bit more work left until this PR is complete. I've addressed the comments in your initial code review.

Since putting this PR up, I've been testing btcd on segnet with some massive transactions in order to shake out some lingering bugs. I found a few (one related to proper sig op coutning) which have been fixed in the latest commits, as well as optimizing some worst-case (related to unnecessary copying, and the HashCache) behavior triggered by a massive transaction I made.

@wallclockbuilder wallclockbuilder commented on an outdated diff May 4, 2016
wire/msgtx.go
var buf [4]byte
binary.LittleEndian.PutUint32(buf[:], uint32(msg.Version))
_, err := w.Write(buf[:])
if err != nil {
return err
}
+ // If the encoding version is set to WitnessEncoding, and the Flags
+ // field for the MsgTx aren't 0x00, then this indicates the transaction
+ // is to be encoded using the new witness inclusionary structure defined
+ // in BIP0141.
+ if enc == WitnessEncoding && msg.HasWitness() {
+ // After the txn's Version field, we include two additional
+ // bytes specific to the witness encoding. The first byte is an
+ // always 0x00 marker byte, which allows decoders to distinguish
+ // a serialized transaction with witnesses from a regular (legacy)
+ // one. The second byte is the Flag field, which at the moment is
+ // always 0x01, but way be extended in the future to accommodate
@wallclockbuilder
wallclockbuilder May 4, 2016 Collaborator

// always 0x01, but way may be extended in the future to accommodate
// auxiliary non-commited fields.

@wallclockbuilder wallclockbuilder commented on the diff May 4, 2016
blockchain/merkle.go
+// in the passed transaction. The witness commitment is stored as the data push
+// for an OP_RETURN with special magic bytes to aide in location.
+func ExtractWitnessCommitment(tx *btcutil.Tx) ([]byte, bool) {
+ var witnessCommitment []byte
+
+ // The witness commitment *must* be located within one of the coinbase
+ // transaction's outputs.
+ if !IsCoinBase(tx) {
+ return witnessCommitment, false
+ }
+
+ msgTx := tx.MsgTx()
+ witFound := false
+ for i := len(msgTx.TxOut) - 1; i >= 0; i-- {
+ // The public key script that contains the witness commitment
+ // must shared a prefix with the WitnessMagicBytes, and be at
@wallclockbuilder
wallclockbuilder May 4, 2016 Collaborator

// The public key script that contains the witness commitment
// must shared share a prefix with the WitnessMagicBytes, and be at
// least 38 bytes.

@davecgh
Member
davecgh commented May 10, 2016

@Roasbeef: I noticed the description can be updated now that the new database code was merged to master about a month ago.

@Roasbeef
Member
Roasbeef commented Jun 25, 2016 edited

@davecgh very true, I've updated the description a bit to reflect all the recent PR's merged into the repo.

One additional thing I need to implement is "chain rewinding" this is necessary in the case that a user upgrades their btcd node after segwit has activated on a particular net. So in this scenario, the btcd node has been syncing an older version of btcd that doesn't yet recognized the segwit soft-fork. As a result the node continues to sync, but is unable to fully validate witness data as it never sees it and doesn't understand what segwit is.

In this case, in order to properly be a fully validating node, the chain must be rewinded until the first block seen with witness data within it. At that point, the node can proceed to fully validate all transactions spending witness outputs.

@davecgh
Member
davecgh commented Aug 12, 2016

Heads up that this will need to be updated for the recent chaincfg.Params changes in PR #732. In particular, aside from a couple of field renames which the compiler will catch, the new segnet4 network params will need to have the new fields defined (CoinbaseMaturity, TargetTimePerBlock, TargetTimespan, BlocksPerRetarget, RetargetAdjustmentFactor, and MinDiffReductionTime) or they'll be zero and consensus obviously won't be accurate.

I believe you should just be able to copy the mainnet params for all of them

@justusranvier
Contributor

Is it possible that support for those BIPS could be made optional either at compilation time or at run time?

@Roasbeef
Member

@justusranvier I won't be making such an addition to this PR. You're welcome to do so yourself in a fork.

@Roasbeef
Member

I've just pushed a final set of commits which are focused mostly on the txscript package. With these commits sane error messages have been added to segwit execution/error paths, the *.json tests caught up to the latest in BC, and some additional witness validation constraints have been added.

Additionally, ScriptStrictMultiSig is now used along with ScriptVerifyWitness as detailed by BIP 147 which patches a lingering source of nuisance malleability related to multi-sig scripts.

I'll next take a snapshot of this tree, then re-structure my commits grouping them by BIP, and then by sub-system in order to make the final reviewer easier. Once the commits have been restructured, I'll remove the WIP tag from the PR 🎂.

@Roasbeef
Member

I've just pushed a restructured set of commits to make the final review easier as the commits began to sprawl a bit as I patched bugs and added additional tests. For the most part, the commits are structured according to the BIP(s) they fully or partially implement.

There are a few lingering TODO(roasbeef): statements which are minor and will be addressed over the next few days. One component that's currently missing from this PR are some of the new segwit related policies which other full-node implementations have started to adopt. As these new changes are purely policy, I've opted to leave them out of this PR and plan to selectively integrate some of the new policy rules in a later PR once this is merged. One area which I'd like to discuss is btcd's policy for preferentially connecting to segwit enabled nodes post-fork.

With this, I'm removing the WIP tag from this PR as the commits are now fairly stable, and I've been testing this PR on all the prior segnets and testnet3, and also directly with our Lightning implementation over the past few months. Once the BIP0009 PR is merged, I'll update this PR with the version bits parameters for all the Bitcoin networks.

The original tree which contained the commits before my most recent rebase can be found here.

@Roasbeef Roasbeef changed the title from [WIP] Add Support for Segregated Witness to Implement BIPs 141, 143, 144, and 147 (Segregated Witness) Oct 19, 2016
@dajohi
Member
dajohi commented Dec 8, 2016

@Roasbeef Can you make separate PRs for each BIP?

@Roasbeef
Member
Roasbeef commented Dec 17, 2016 edited

@dajohi respectfully, I'd rather not. The PR as it is (since its initial opening back in April) has received a good bit of review with the current commit structure. Additionally, I've attempted to carefully create a set of commits which are isolated to a single segwit related BIP, and further individual sub-system within the daemon.

In my opinion, abandoning this PR in favor of several individual PR's would slow down the integration of segwit features into btcd and partially waste the months work I've put into this PR. I've personally been testing this PR directly on all the past segnets and the latest testnet as I've been developing our implementation of Lightning and haven't run into any major issues as the PR currently stands now. One minor artefact is that if you attempt to sync this PR on testnet as it is now, you'll need to comment out the unconditionally inclusion of the txscript.ScriptMultisig (BIP 147) flag as there're historical transactions on testnet that don't adhere to the stricter rule. Once master settles down a bit, I'll rebase this PR on top so I can add the BIP9 checks.

I have a final set of commits locally with fix the last lingering unchecked item in the PR description, but I'm holding off on pushing it until my CSV PR lands into master. Master has shifted quite a bit from underneath this PR (as you can see by the large amount of currently conflicting files) so I'd rather wait for my other soft-fork PR to be merged so I don't have to constantly rebase and fix merge conflicts. Since the PR is so large rebasing on top of master as is now isn't strictly mechanical, and can take some time to ensure I'm not breaking any segwit functionality within this PR or introducing new bugs into the project.

I've recently gained access to an Antminer S9, and a running instance of the latest version of eloipool, so I'll able to properly test the GBT related mining aspects of this PR against a piece of widely used pool software and some standard mining hardware. I've successfully mined several blocks on the past segnets and a few on testnet, but those were using the cpuminer, so these final set of tests should instill a degree of confidence between this PR and widely used hardware/software infrastructure within the ecosystem.

Roasbeef added some commits Oct 18, 2016
@Roasbeef Roasbeef BIP0144+wire: introduce the new SFNodeWitness service bit
This commit introduces the new SFNodeWitness service bit which has been
added to the protocol as part of BIP0144. The new service bit allows
peers on the network to signal their acceptance and adherence to the
new rules defined as part of the segwit soft-fork package.
bdbb723
@Roasbeef Roasbeef BIP0144+wire: add a MessageEncoding variant for serialization/deseria…
…lization

This commit modifies the existing wire.Message interface to introduce a
new MessageEncoding variant which dictates the exact encoding to be
used when serializing and deserializing messages. Such an option is now
necessary due to the segwit soft-fork package, as btcd will need to be
able to optionally encode transactions/blocks without witness data to
un-upgraded peers.

Two new functions have been introduced: ReadMessageWithEncodingN and
WriteMessageWithEncodingN which wrap BtcDecode/BtcEncode with the
desired encoding format.
d6a84e9
@Roasbeef Roasbeef BIP0144+wire: introduce new segregated witness inventory types
This commit adds the new inventory types for segwit which are used by
the responder to explicitly request that transactions/blocks sent for a
particular inv hash should include all witness data.
c1e4db9
@Roasbeef Roasbeef BIP0144+wire: implement witness encoding/decoding for transactions
This commit implements the new witness encoding/decoding for
transactions as specified by BIP0144. After segwit activation, a
special transaction encoding is used to signal to upgraded nodes that
the transaction being deserialized bares witness data. The prior
BtcEncode and BtcDecode methods have been extended to be aware of the
new signaling bytes and the encoding of witness data within
transactions.

Additionally, a new method has been added to calculate the “stripped
size” of a transaction/block which is defined as the size of a
transaction/block *excluding* any witness data.
8468827
@Roasbeef Roasbeef BIP0144+peer: specify the wire encoding type when reading/writing mes…
…sages

This commit modifies the base peer struct to ascertain when a peer is
able to understand the new witness encoding, and specify the peer’s
supported encoding explicitly before/after the version handshake.
8e45601
@Roasbeef Roasbeef BIP0144: properly fetch witness data from witness-enabled peers
This commit modifies the logic within the block manager and service to
preferentially fetch transactions and blocks which include witness data
from fully upgraded peers.

Once the initial version handshake has completed, the server now tracks
which of the connected peers are witness enabled (they advertise
SFNodeWitness). From then on, if a peer is witness enabled, then btcd
will always request full witness data when fetching
transactions/blocks.
9b52054
@Roasbeef Roasbeef BIP0143+txscript: add segwit sighash, signing, and HashCache integration
This commit implements most of BIP0143 by adding logic to implement the
new sighash calculation, signing, and additionally introduces the
HasCache optimization which eliminates the O(N^2) computational
complexity for the SIGHASH_ALL sighash type.

The HashCache struct is the equivalent to the existing Sigcache struct,
but fore caching the reusable misstate for transactions which are
spending segwitty outputs.
638ac51
@Roasbeef Roasbeef BIP0141+txscript: awareness of new standard script templates, add hel…
…per funcs

This commit introduces a series of internal and external helper
functions which enable the txscript package to be aware of the new
standard script templates introduced as part of BIP0141. The two new
standard script templates recognized are pay-to-witness-key-hash
(P2WKH) and pay-to-witness-script-hash (P2WSH).
8276d12
@Roasbeef Roasbeef BIP0141+txscript: implement signature operation cost calculations 3862696
@Roasbeef Roasbeef txscript: fix off-by-one error due to new OP_CODESEPARATOR behavior i…
…n segwit

This commit fixes an off-by-one error which is only manifested by the
new behavior of OP_CODESEPARATOR within sig hashes triggered by the
segwit behavior. The current behavior within the Script VM
(txscript.Engine) known to be fully correct to the extent that it has
been verified. However, once segwit activates a consensus divergence
would emerge due exactly *when* the program counter was incremented in
the previous code (pre-this-commit).

Currently (pre-segwit) when calculating the pre-image to a transaction
sighash for signature verification, *all* instances of OP_CODESEPARATOR
are removed from the subScript being signed before generating the final
sighash. The addition of SegWIt has additional nerved the behavior of
OP_CODESEPARATOR by no longer removing them (and starting after the
last instance), but instead simply starting the subScript to be
directly *after* the last instance of an OP_CODESEPARATOR within the
pkScript.

Due to this new behavior, without this commit, an off-by-one error
(which only matters post-segwit), would case txscript to generate an
incorrect subScript since the instance of OP_CODESEPARATOR would remain
as part of the subScript instead of being sliced off as the new
behavior dictates. The off-by-one error itself is manifsted due to a
slight divergence in txscript.Engine’s logic compared to Bitcoin Core.
In Bitcoin Core script verification is as follows: first the next
op-code is fetched, then program counter is incremented, and finally
the op-code itself is executed. Before this commit, btcd flipped the
order of the last two steps, executing the op-code *before* the program
counter was incremented.

This commit fixes the post-segwit consensus divergence by incrementing
the program-counter *before* the next op-code is executed. It is
important to note that this divergence is only significant pre-segwit,
meaning that txscript.Engine is still consensus compliant independent
of this commit.
db0dbf0
@Roasbeef Roasbeef BIP0141+txscript: implement witness program validation
This commit implements full witness program validation for the
currently defined version 0 witness programs. This includes validation
logic for nested p2sh, p2wsh, and p2wkh. Additionally, when in witness
validation mode, an additional set of constrains are enforced such as
using the new sighash digest algorithm and enforcing clean stack
behavior within witness programs.
1159e3d
@Roasbeef Roasbeef BIP 147: enforce NULLDUMMY w/ segwit verification
This commit implements the flag activation portion of BIP 0147. The
verification behavior triggered by the NULLDUMMY script verification
flag has been present within btcd for some time, however it wasn’t
activated by default.

With this commit, once segwit has activated, the ScriptStrictMultiSig
will also be activated within the Script VM. Additionally, the
ScriptStrictMultiSig is now a standard script verification flag which
is used unconditionally within the mempool.
e83a1d0
@Roasbeef Roasbeef txscript: update reference tests to include new segwit related cases c3e55f9
@Roasbeef Roasbeef BIP0141+wire: add a WitnessHash method to tx which calls wtxid 8d36d7b
@Roasbeef Roasbeef chaincfg: add address bytes for p2wsh and p2wkh addresses 133429f
@Roasbeef Roasbeef BIP0141+blockchain: implement tx/block weight calculation funcitons
This commit implements the new “weight” metric introduced as part of
the segwit soft-fork. Post-fork activation, rather than limiting the
size of blocks and transactions based purely on serialized size, a new
metric “weight” will instead be used as a way to more accurately
reflect the costs of a tx/block on the system. With blocks constrained
by weight, the maximum block-size increases to ~4MB.
0d5b5c2
@Roasbeef Roasbeef BIP0141+blockchain: add functions for extracting and validating witne…
…ss commitment
ed531ed
@Roasbeef Roasbeef BIP0141+blockchain: implement segwit block validation rules
This commit implements the new block validation rules as defined by
BIP0141. The new rules include the constraints that if a block has
transactions with witness data, then there MUST be a commitment within
the conies transaction to the root of a new merkle tree which commits
to the wtxid of all transactions. Additionally, rather than limiting
the size of a block by size in bytes, blocks are now limited by their
total weight unit. Similarly, a newly define “sig op cost” is now used
to limit the signature validation cost of transactions found within
blocks.
03368dc
@Roasbeef Roasbeef blockchain/indexers: add addrindex support for p2wkh and p2wsh cbf130b
@Roasbeef Roasbeef mempool: modify mempool sanity checks to be segwit aware 9fed348
@Roasbeef Roasbeef btcjson: update RPC calls to return segwit related data 54c262c
@Roasbeef Roasbeef mining+config: modify GBT mining to limit by weight, add witness comm…
…itment

This commit modifies the existing block selection logic to limit
preferentially by weight instead of serialized block size, and also to
adhere to the new sig-op cost limits which are weighted according to
the witness discount.
eb21ba6
@Roasbeef Roasbeef chaincfg+integration: add BIP0009 deployment parameters for segwit
This commit adds set of BIP0009 (Version Bits) deployment parameters
for all networks detailing the activation parameters for the segwit
soft-fork.

Additionally, the BIP0009 integration test has been updated to test for
the proper transitioning of version bits state for the segwit soft
fork. Finally, the `getblockchaininfo` test has also been updated to
properly display the state of segwit.
6213a0f
@Roasbeef Roasbeef blockchain: guard enforcement of segwit rules by a BIP0009 state check
This commit updates the new segwit validation logic within block
validation to be guarded by an initial check to the version bits state
before conditionally enforcing the logic based off of the state.
e3f7cba
@Roasbeef Roasbeef mining: update GBT generation to include witness commitment
This commit updates the block template generation logic to only include
witness transactions once the soft-fork has activated and to also
include the OP_RETURN witness commitment (with additional block weight
accounting).
b60f504
@Roasbeef
Member
Roasbeef commented Jan 4, 2017 edited

Just pushed some final commits which: add the deployment parameters, guard the activation of the new block validation logic via a check to the BIP0009 version bits state, and properly include the witness commitment within an OP_RETURN output within the codebase. The final change was necessary as when I originally implemented the mining code (like back in Feb/March or something), the commitment was actually part of the coinbase txIn script. Since then, it has been moved to an additional output within the coinbase transaction.

With this final set of commits, all lingering TODO's have been addressed. Once the CSV PR goes in, I'll rebase over that.

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