From 2bccf663c1c0908b0f5fcf25c0422408fead19cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christopher=20J=C3=A4mthagen?= Date: Wed, 4 Jan 2017 16:15:50 +0100 Subject: [PATCH 1/8] Use 48-bit commitment transaction numbers Fix SetStateNumHint and GetStateNumHint to properly set and get the stateNumHints using the lower 24 bits of the locktime of the commitment transaction as the lower 24 bits of the obfuscated state number and the lower 24 bits of the sequence field as the higher 24 bits. --- channeldb/channel.go | 2 +- lnwallet/channel.go | 4 +-- lnwallet/script_utils.go | 48 ++++++++++++++++++++--------------- lnwallet/script_utils_test.go | 2 +- lnwallet/wallet.go | 2 +- 5 files changed, 33 insertions(+), 25 deletions(-) diff --git a/channeldb/channel.go b/channeldb/channel.go index d179878ffec..9a52b494e4a 100644 --- a/channeldb/channel.go +++ b/channeldb/channel.go @@ -185,7 +185,7 @@ type OpenChannel struct { // StateHintObsfucator are the btyes selected by the initiator (derived // from their shachain root) to obsfucate the state-hint encoded within // the commitment transaction. - StateHintObsfucator [4]byte + StateHintObsfucator [6]byte // ChanType denotes which type of channel this is. ChanType ChannelType diff --git a/lnwallet/channel.go b/lnwallet/channel.go index f92295a7ee6..dfeb5066344 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -875,7 +875,7 @@ func (lc *LightningChannel) closeObserver(channelCloseNtfn *chainntnfs.SpendEven // Decode the state hint encoded within the commitment transaction to // determine if this is a revoked state or not. obsfucator := lc.channelState.StateHintObsfucator - broadcastStateNum := uint64(GetStateNumHint(commitTxBroadcast, obsfucator)) + broadcastStateNum := GetStateNumHint(commitTxBroadcast, obsfucator) currentStateNum := lc.currentHeight @@ -1125,7 +1125,7 @@ func (lc *LightningChannel) fetchCommitmentView(remoteChain bool, // quickly recovering the necessary penalty state in the case of an // uncooperative broadcast. obsfucator := lc.channelState.StateHintObsfucator - stateNum := uint32(nextHeight) + stateNum := nextHeight if err := SetStateNumHint(commitTx, stateNum, obsfucator); err != nil { return nil, err } diff --git a/lnwallet/script_utils.go b/lnwallet/script_utils.go index dc4f853a4a3..8aea1d6ca65 100644 --- a/lnwallet/script_utils.go +++ b/lnwallet/script_utils.go @@ -29,12 +29,12 @@ const ( // StateHintSize is the total number of bytes used between the sequence // number and locktime of the commitment transaction use to encode a hint // to the state number of a particular commitment transaction. - StateHintSize = 4 + StateHintSize = 6 // maxStateHint is the maximum state number we're able to encode using // StateHintSize bytes amongst the sequence number and locktime fields // of the commitment transaction. - maxStateHint = (1 << 31) - 1 + maxStateHint = (1 << 48) - 1 ) // witnessScriptHash generates a pay-to-witness-script-hash public key script @@ -770,18 +770,19 @@ func deriveElkremRoot(elkremDerivationRoot *btcec.PrivateKey, } // SetStateNumHint encodes the current state number within the passed -// commitment transaction by re-purposing the sequence fields in the input of -// the commitment transaction to encode the obfuscated state number. The state -// number is encoded using 31-bits of the sequence number, with the top bit set -// in order to disable BIP0068 (sequence locks) semantics. Finally before -// encoding, the obfuscater is XOR'd against the state number in order to hide -// the exact state number from the PoV of outside parties. +// commitment transaction by re-purposing the locktime and sequence fields +// in the commitment transaction to encode the obfuscated state number. +// The state number is encoded using 48 bits. The lower 24 bits of the +// locktime are the lower 24 bits of the obfuscated state number and the +// lower 24 bits of the sequence field are the higher 24 bits. Finally +// before encoding, the obfuscater is XOR'd against the state number in +// order to hide the exact state number from the PoV of outside parties. // TODO(roasbeef): unexport function after bobNode is gone -func SetStateNumHint(commitTx *wire.MsgTx, stateNum uint32, +func SetStateNumHint(commitTx *wire.MsgTx, stateNum uint64, obsfucator [StateHintSize]byte) error { // With the current schema we are only able able to encode state num - // hints up to 2^31. Therefore if the passed height is greater than our + // hints up to 2^48. Therefore if the passed height is greater than our // state hint ceiling, then exit early. if stateNum >= maxStateHint { return fmt.Errorf("unable to encode state, %v is greater "+ @@ -793,16 +794,20 @@ func SetStateNumHint(commitTx *wire.MsgTx, stateNum uint32, "instead has %v", len(commitTx.TxIn)) } - // Convert the obfuscater into a uint32, then XOR that against the + // Convert the obfuscator into a uint64, then XOR that against the // targeted height in order to obfuscate the state number of the // commitment transaction in the case that either commitment // transaction is broadcast directly on chain. - xorInt := binary.BigEndian.Uint32(obsfucator[:]) & (^wire.SequenceLockTimeDisabled) + var obfs [8]byte + copy(obfs[2:], obsfucator[:]) + xorInt := binary.BigEndian.Uint64(obfs[:]) + stateNum = stateNum ^ xorInt // Set the height bit of the sequence number in order to disable any // sequence locks semantics. - commitTx.TxIn[0].Sequence = stateNum | wire.SequenceLockTimeDisabled + commitTx.TxIn[0].Sequence = uint32(stateNum>>24) | wire.SequenceLockTimeDisabled + commitTx.LockTime = uint32(stateNum & 0xFFFFFF) return nil } @@ -813,14 +818,17 @@ func SetStateNumHint(commitTx *wire.MsgTx, stateNum uint32, // // See setStateNumHint for further details w.r.t exactly how the state-hints // are encoded. -func GetStateNumHint(commitTx *wire.MsgTx, obsfucator [StateHintSize]byte) uint32 { - // Convert the obfuscater into a uint32, this will be used to +func GetStateNumHint(commitTx *wire.MsgTx, obsfucator [StateHintSize]byte) uint64 { + // Convert the obfuscater into a uint64, this will be used to // de-obfuscate the final recovered state number. - xorInt := binary.BigEndian.Uint32(obsfucator[:]) & (^wire.SequenceLockTimeDisabled) - - // Retrieve the sole state hint from the sequence number of the - // transaction. In the process un-set the top bit. - stateNumXor := commitTx.TxIn[0].Sequence & (^wire.SequenceLockTimeDisabled) + var obfs [8]byte + copy(obfs[2:], obsfucator[:]) + xorInt := binary.BigEndian.Uint64(obfs[:]) + + // Retrieve the state hint from the sequence number and locktime + // of the transaction. + stateNumXor := uint64(commitTx.TxIn[0].Sequence&0xFFFFFF) << 24 + stateNumXor |= uint64(commitTx.LockTime & 0xFFFFFF) // Finally, to obtain the final state number, we XOR by the obfuscater // value to de-obfuscate the state number. diff --git a/lnwallet/script_utils_test.go b/lnwallet/script_utils_test.go index fb0ec41fda7..cc402916dc3 100644 --- a/lnwallet/script_utils_test.go +++ b/lnwallet/script_utils_test.go @@ -571,7 +571,7 @@ func TestCommitTxStateHint(t *testing.T) { copy(obsfucator[:], testHdSeed[:StateHintSize]) for i := 0; i < 10000; i++ { - stateNum := uint32(i) + stateNum := uint64(i) err := SetStateNumHint(commitTx, stateNum, obsfucator) if err != nil { diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index c4123c9a4e2..81a2d8c327d 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -1397,7 +1397,7 @@ func deriveStateHintObsfucator(elkremRoot *elkrem.ElkremSender) ([StateHintSize] return obsfucator, nil } -// initStateHints properly sets the obsfucated state ints ob both commitment +// initStateHints properly sets the obsfucated state hints on both commitment // transactions using the passed obsfucator. func initStateHints(commit1, commit2 *wire.MsgTx, obsfucator [StateHintSize]byte) error { From 24045b5c46ad3ffb36dcef5418e81b557d9deba4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christopher=20J=C3=A4mthagen?= Date: Thu, 5 Jan 2017 08:55:52 +0100 Subject: [PATCH 2/8] lnwire: use [6]byte obsfucator --- lnwire/single_funding_complete.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lnwire/single_funding_complete.go b/lnwire/single_funding_complete.go index 323a855737d..5e1eb5d6df9 100644 --- a/lnwire/single_funding_complete.go +++ b/lnwire/single_funding_complete.go @@ -41,14 +41,14 @@ type SingleFundingComplete struct { // the commitment transaction's only input. The initiator generates // this value by hashing the 0th' state derived from the revocation PRF // an additional time. - StateHintObsfucator [4]byte + StateHintObsfucator [6]byte } // NewSingleFundingComplete creates, and returns a new empty // SingleFundingResponse. func NewSingleFundingComplete(chanID uint64, fundingPoint wire.OutPoint, commitSig *btcec.Signature, revokeKey *btcec.PublicKey, - obsfucator [4]byte) *SingleFundingComplete { + obsfucator [6]byte) *SingleFundingComplete { return &SingleFundingComplete{ ChannelID: chanID, From c687a7e2ed744d0842dea8a61ed1d3b8b1f71a04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christopher=20J=C3=A4mthagen?= Date: Mon, 9 Jan 2017 09:40:34 +0100 Subject: [PATCH 3/8] channeldb: change [4]byte obsfucator to [6]byte in test --- channeldb/channel_test.go | 2 +- lnwire/single_funding_complete_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/channeldb/channel_test.go b/channeldb/channel_test.go index c56f6197c13..1baf1abb7bd 100644 --- a/channeldb/channel_test.go +++ b/channeldb/channel_test.go @@ -132,7 +132,7 @@ func createTestChannelState(cdb *DB) (*OpenChannel, error) { } } - var obsfucator [4]byte + var obsfucator [6]byte copy(obsfucator[:], key[:]) return &OpenChannel{ diff --git a/lnwire/single_funding_complete_test.go b/lnwire/single_funding_complete_test.go index a340bc6ca04..43dfc9996ec 100644 --- a/lnwire/single_funding_complete_test.go +++ b/lnwire/single_funding_complete_test.go @@ -7,8 +7,8 @@ import ( ) func TestSingleFundingCompleteWire(t *testing.T) { - var obsfucator [4]byte - copy(obsfucator[:], bytes.Repeat([]byte("k"), 4)) + var obsfucator [6]byte + copy(obsfucator[:], bytes.Repeat([]byte("k"), 6)) // First create a new SFC message. sfc := NewSingleFundingComplete(22, *outpoint1, commitSig1, pubKey, From 63ba46c6bd25f73578bdad0ae8896eb751025cf8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christopher=20J=C3=A4mthagen?= Date: Mon, 9 Jan 2017 09:53:16 +0100 Subject: [PATCH 4/8] lnwire: add serializing and deserializing for type [6]byte and remove serialization for type [4]byte. --- lnwire/lnwire.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lnwire/lnwire.go b/lnwire/lnwire.go index 752355e8861..3ae9e389098 100644 --- a/lnwire/lnwire.go +++ b/lnwire/lnwire.go @@ -171,6 +171,10 @@ func writeElement(w io.Writer, element interface{}) error { return err } + if _, err := w.Write(e[:]); err != nil { + return err + } + case [6]byte: if _, err := w.Write(e[:]); err != nil { return err } @@ -467,6 +471,10 @@ func readElement(r io.Reader, element interface{}) error { if _, err := io.ReadFull(r, *e); err != nil { return err } + case *[6]byte: + if _, err := io.ReadFull(r, e[:]); err != nil { + return err + } case []byte: if _, err := io.ReadFull(r, e); err != nil { return err From 50992e8b3d176527d104234837ad7855556f23a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christopher=20J=C3=A4mthagen?= Date: Mon, 9 Jan 2017 14:58:58 +0100 Subject: [PATCH 5/8] lnwallet: allow maximum state size to be used + tests Add tests to assert maximum state can be used. Also test that more than one input in the commitment transaction will fail and that having state number larger than maxStateHint will fail. --- lnwallet/script_utils.go | 2 +- lnwallet/script_utils_test.go | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/lnwallet/script_utils.go b/lnwallet/script_utils.go index 8aea1d6ca65..2065411b049 100644 --- a/lnwallet/script_utils.go +++ b/lnwallet/script_utils.go @@ -784,7 +784,7 @@ func SetStateNumHint(commitTx *wire.MsgTx, stateNum uint64, // With the current schema we are only able able to encode state num // hints up to 2^48. Therefore if the passed height is greater than our // state hint ceiling, then exit early. - if stateNum >= maxStateHint { + if stateNum > maxStateHint { return fmt.Errorf("unable to encode state, %v is greater "+ "state num that max of %v", stateNum, maxStateHint) } diff --git a/lnwallet/script_utils_test.go b/lnwallet/script_utils_test.go index cc402916dc3..62f2da7ebec 100644 --- a/lnwallet/script_utils_test.go +++ b/lnwallet/script_utils_test.go @@ -583,5 +583,27 @@ func TestCommitTxStateHint(t *testing.T) { t.Fatalf("state number mismatched, expected %v, got %v", stateNum, extractedStateNum) } + + //Test from maximum allowed state + stateNum = uint64(maxStateHint - i) + err = SetStateNumHint(commitTx, stateNum, obsfucator) + if err != nil { + t.Fatalf("unable to set state num %v: %v", i, err) + } + + extractedStateNum = GetStateNumHint(commitTx, obsfucator) + if extractedStateNum != stateNum { + t.Fatalf("state number mismatched, expected %v, got %v", + stateNum, extractedStateNum) + } + } + + if err := SetStateNumHint(commitTx, maxStateHint+1, obsfucator); err == nil { + t.Fatalf("state number should not exceed %X", maxStateHint) + } + + commitTx.AddTxIn(&wire.TxIn{}) + if err := SetStateNumHint(commitTx, 0, obsfucator); err == nil { + t.Fatalf("more than one input in commit transaction should not be valid") } } From 9a8e80ec87b35f738aa9073307098756338bfb8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christopher=20J=C3=A4mthagen?= Date: Mon, 9 Jan 2017 19:03:19 +0100 Subject: [PATCH 6/8] test: Add table driven tests for script_utils Add table-driven tests for testing GetStateHint and SetStateHint in package lnwallet. --- lnwallet/script_utils_test.go | 87 ++++++++++++++++++++++------------- 1 file changed, 56 insertions(+), 31 deletions(-) diff --git a/lnwallet/script_utils_test.go b/lnwallet/script_utils_test.go index 62f2da7ebec..f4890cdc6cd 100644 --- a/lnwallet/script_utils_test.go +++ b/lnwallet/script_utils_test.go @@ -563,47 +563,72 @@ func TestHTLCReceiverSpendValidation(t *testing.T) { } } +var stateHintTests = []struct { + name string + from uint64 + to uint64 + inputs int + shouldFail bool +}{ + { + name: "states 0 to 1000", + from: 0, + to: 1000, + inputs: 1, + shouldFail: false, + }, + { + name: "states 'maxStateHint-1000' to 'maxStateHint'", + from: maxStateHint - 1000, + to: maxStateHint, + inputs: 1, + shouldFail: false, + }, + { + name: "state 'maxStateHint+1'", + from: maxStateHint + 1, + to: maxStateHint + 10, + inputs: 1, + shouldFail: true, + }, + { + name: "commit transaction with two inputs", + inputs: 2, + shouldFail: true, + }, +} + func TestCommitTxStateHint(t *testing.T) { - commitTx := wire.NewMsgTx(2) - commitTx.AddTxIn(&wire.TxIn{}) var obsfucator [StateHintSize]byte copy(obsfucator[:], testHdSeed[:StateHintSize]) - for i := 0; i < 10000; i++ { - stateNum := uint64(i) + for _, test := range stateHintTests { + commitTx := wire.NewMsgTx(2) - err := SetStateNumHint(commitTx, stateNum, obsfucator) - if err != nil { - t.Fatalf("unable to set state num %v: %v", i, err) + // Add supplied number of inputs to the commitment transaction. + for i := 0; i < test.inputs; i++ { + commitTx.AddTxIn(&wire.TxIn{}) } - extractedStateNum := GetStateNumHint(commitTx, obsfucator) - if extractedStateNum != stateNum { - t.Fatalf("state number mismatched, expected %v, got %v", - stateNum, extractedStateNum) - } + for i := test.from; i <= test.to; i++ { + stateNum := uint64(i) - //Test from maximum allowed state - stateNum = uint64(maxStateHint - i) - err = SetStateNumHint(commitTx, stateNum, obsfucator) - if err != nil { - t.Fatalf("unable to set state num %v: %v", i, err) - } + err := SetStateNumHint(commitTx, stateNum, obsfucator) + if err != nil && !test.shouldFail { + t.Fatalf("unable to set state num %v: %v", i, err) + } else if err == nil && test.shouldFail { + t.Fatalf("Failed(%v): test should fail but did not", test.name) + } - extractedStateNum = GetStateNumHint(commitTx, obsfucator) - if extractedStateNum != stateNum { - t.Fatalf("state number mismatched, expected %v, got %v", - stateNum, extractedStateNum) + extractedStateNum := GetStateNumHint(commitTx, obsfucator) + if extractedStateNum != stateNum && !test.shouldFail { + t.Fatalf("state number mismatched, expected %v, got %v", + stateNum, extractedStateNum) + } else if extractedStateNum == stateNum && test.shouldFail { + t.Fatalf("Failed(%v): test should fail but did not", test.name) + } } - } - - if err := SetStateNumHint(commitTx, maxStateHint+1, obsfucator); err == nil { - t.Fatalf("state number should not exceed %X", maxStateHint) - } - - commitTx.AddTxIn(&wire.TxIn{}) - if err := SetStateNumHint(commitTx, 0, obsfucator); err == nil { - t.Fatalf("more than one input in commit transaction should not be valid") + t.Logf("Passed: %v", test.name) } } From 272304d7bc6bb7657e873f1bc332e02d718cd04b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christopher=20J=C3=A4mthagen?= Date: Fri, 13 Jan 2017 15:29:56 +0100 Subject: [PATCH 7/8] lnwallet: fix bug that makes commitment transaction unspendable Introduce TimelockShift which is used to make sure the commitment transaction is spendable by setting the locktime with it so that it is larger than 500,000,000, thus interpreting it as Unix epoch timestamp and not a block height. It is also smaller than the current timestamp which has bit (1 << 30) set, so there is no risk of having the commitment transaction be rejected. This way we can safely use the lower 24 bits of the locktime field for part of the obscured commitment transaction number. --- lnwallet/script_utils.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lnwallet/script_utils.go b/lnwallet/script_utils.go index 2065411b049..a687ee0d0c3 100644 --- a/lnwallet/script_utils.go +++ b/lnwallet/script_utils.go @@ -23,6 +23,16 @@ var ( SequenceLockTimeSeconds = uint32(1 << 22) SequenceLockTimeMask = uint32(0x0000ffff) OP_CHECKSEQUENCEVERIFY byte = txscript.OP_NOP3 + + // TimelockShift is used to make sure the commitment transaction is + // spendable by setting the locktime with it so that it is larger than + // 500,000,000, thus interpreting it as Unix epoch timestamp and not + // a block height. It is also smaller than the current timestamp which + // has bit (1 << 30) set, so there is no risk of having the commitment + // transaction be rejected. This way we can safely use the lower 24 bits + // of the locktime field for part of the obscured commitment transaction + // number. + TimelockShift = uint32(1 << 29) ) const ( @@ -807,7 +817,7 @@ func SetStateNumHint(commitTx *wire.MsgTx, stateNum uint64, // Set the height bit of the sequence number in order to disable any // sequence locks semantics. commitTx.TxIn[0].Sequence = uint32(stateNum>>24) | wire.SequenceLockTimeDisabled - commitTx.LockTime = uint32(stateNum & 0xFFFFFF) + commitTx.LockTime = uint32(stateNum&0xFFFFFF) | TimelockShift return nil } From ba325a95a1da5d1ff202578d5b6b963cc1ecac8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christopher=20J=C3=A4mthagen?= Date: Sat, 14 Jan 2017 09:17:00 +0100 Subject: [PATCH 8/8] lnwallet: extend test to check for valid locktime and sequence --- lnwallet/script_utils_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/lnwallet/script_utils_test.go b/lnwallet/script_utils_test.go index f4890cdc6cd..fabdcfc0dd7 100644 --- a/lnwallet/script_utils_test.go +++ b/lnwallet/script_utils_test.go @@ -4,6 +4,7 @@ import ( "bytes" "fmt" "testing" + "time" "github.com/btcsuite/fastsha256" "github.com/roasbeef/btcd/btcec" @@ -602,6 +603,7 @@ func TestCommitTxStateHint(t *testing.T) { var obsfucator [StateHintSize]byte copy(obsfucator[:], testHdSeed[:StateHintSize]) + timeYesterday := uint32(time.Now().Unix() - 24*60*60) for _, test := range stateHintTests { commitTx := wire.NewMsgTx(2) @@ -621,6 +623,25 @@ func TestCommitTxStateHint(t *testing.T) { t.Fatalf("Failed(%v): test should fail but did not", test.name) } + locktime := commitTx.LockTime + sequence := commitTx.TxIn[0].Sequence + + // Locktime should not be less than 500,000,000 and not larger + // than the time 24 hours ago. One day should provide a good + // enough buffer for the tests. + if locktime < 5e8 || locktime > timeYesterday { + if !test.shouldFail { + t.Fatalf("The value of locktime (%v) may cause the commitment "+ + "transaction to be unspendable", locktime) + } + } + + if sequence&wire.SequenceLockTimeDisabled == 0 { + if !test.shouldFail { + t.Fatalf("Sequence locktime is NOT disabled when it should be") + } + } + extractedStateNum := GetStateNumHint(commitTx, obsfucator) if extractedStateNum != stateNum && !test.shouldFail { t.Fatalf("state number mismatched, expected %v, got %v",