Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add tests to trigger witness limit error paths #1917

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add tests to trigger witness limit error paths #1917

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
96cd3f4
cleanup: add newline for consistency and readability
otech47 Nov 6, 2022
b035335
Add test for decoding txn with many witness items
otech47 Nov 6, 2022
0b59cb7
Add test for decoding txn with oversized witness
otech47 Mar 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
146 changes: 145 additions & 1 deletion wire/msgtx_test.go
View file
Open in desktop
Expand Up @@ -282,7 +282,8 @@ func TestTxWire(t *testing.T) {
// Latest protocol version with no transactions.
{
noTx,
noTx, noTxEncoded,
noTx,
noTxEncoded,
otech47 marked this conversation as resolved.
Show resolved Hide resolved
ProtocolVersion,
BaseEncoding,
},
Expand Down Expand Up @@ -778,6 +779,149 @@ func TestTxWitnessSize(t *testing.T) {
}
}

// TestTxWitnessLimits performs negative tests to ensure decoding
// serialized transactions with witness data triggers proper error paths.
func TestTxWitnessLimits(t *testing.T) {
otech47 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Author

Choose a reason for hiding this comment

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

thinking more about it... if these limit checks are already happening deeper in consensus logic, I wonder if it doesn't make more sense to simply forget about witness limit checks and just do a <4MB size check on the entire txn? if these checks at a parsing level are really more about memory exhaustion safeguards, there shouldn't be a need to look for the witness specifically?

Copy link
Member

Choose a reason for hiding this comment

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

, I wonder if it doesn't make more sense to simply forget about witness limit checks and just do a <4MB size check on the entire txn?

That would certainly be the most conservative check. The issue with doing it on this layer though, is the possibility of future interactions that increase this value (say an ext block in the future or w/e). On the bitcoind side, they have such limits in place still, but it's around 32 MB or so (iirc the max p2p payload). So if we end up upgrading Bitcoin far in the future to 64 MB blocks or w/e, then that would cause a fork with these old clients.

// Use protocol version 70001 and transaction version 1 specifically
// here instead of the latest values because the test data is using
otech47 marked this conversation as resolved.
Show resolved Hide resolved
// bytes encoded with those versions.
pver := uint32(70001)
txVer := uint32(1)

var createTxnWithManyWitnessItems = func() []byte {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't look to use many of the variables closed over, so I think we can just make this a regular function and then call it below.

var encodedTxnBuffer []byte

txnBeforeWitnessEncoded := []byte{
0x1, 0x0, 0x0, 0x0, // Version
TxFlagMarker, // Marker byte indicating 0 inputs, or a segwit encoded tx
WitnessFlag, // Flag byte
0x1, // Varint for number of inputs
0xa5, 0x33, 0x52, 0xd5, 0x13, 0x57, 0x66, 0xf0,
0x30, 0x76, 0x59, 0x74, 0x18, 0x26, 0x3d, 0xa2,
0xd9, 0xc9, 0x58, 0x31, 0x59, 0x68, 0xfe, 0xa8,
0x23, 0x52, 0x94, 0x67, 0x48, 0x1f, 0xf9, 0xcd, // Previous output hash
0x13, 0x0, 0x0, 0x0, // Little endian previous output index
0x0, // No sig script (this is a witness input)
0xff, 0xff, 0xff, 0xff, // Sequence
0x1, // Varint for number of outputs
0xb, 0x7, 0x6, 0x0, 0x0, 0x0, 0x0, 0x0, // Output amount
0x16, // Varint for length of pk script
0x0, // Version 0 witness program
0x14, // OP_DATA_20
0x9d, 0xda, 0xc6, 0xf3, 0x9d, 0x51, 0xe0, 0x39,
0x8e, 0x53, 0x2a, 0x22, 0xc4, 0x1b, 0xa1, 0x89,
0x40, 0x6a, 0x85, 0x23, // 20-byte pub key hash
}
witnessItemCountEncoded := []byte{
0xfe, 0x01, 0x09, 0x3d, 0x00, // 4,000,001 items on the witness stack
}
encodedTxnBuffer = append(txnBeforeWitnessEncoded, witnessItemCountEncoded...)
// Add 4,000,001 witness items of 1-byte each
for i := uint(0); i < 4_000_001; i++ {
// First add the size of the witness item (1-byte)
encodedTxnBuffer = append(encodedTxnBuffer, "1"...)
// Then add the witness item itself (1-byte)
encodedTxnBuffer = append(encodedTxnBuffer, "0"...)
otech47 marked this conversation as resolved.
Show resolved Hide resolved
}
var locktimeEncoded = []byte{
0x0, 0x0, 0x0, 0x0, // Lock time
}
encodedTxnBuffer = append(encodedTxnBuffer, locktimeEncoded...)
otech47 marked this conversation as resolved.
Show resolved Hide resolved

return encodedTxnBuffer
}

var createTxnWithOversizedWitness = func() []byte {
var encodedTxnBuffer []byte

txnBeforeWitnessEncoded := []byte{
0x1, 0x0, 0x0, 0x0, // Version
TxFlagMarker, // Marker byte indicating 0 inputs, or a segwit encoded tx
WitnessFlag, // Flag byte
0x1, // Varint for number of inputs
0xa5, 0x33, 0x52, 0xd5, 0x13, 0x57, 0x66, 0xf0,
0x30, 0x76, 0x59, 0x74, 0x18, 0x26, 0x3d, 0xa2,
0xd9, 0xc9, 0x58, 0x31, 0x59, 0x68, 0xfe, 0xa8,
0x23, 0x52, 0x94, 0x67, 0x48, 0x1f, 0xf9, 0xcd, // Previous output hash
0x13, 0x0, 0x0, 0x0, // Little endian previous output index
0x0, // No sig script (this is a witness input)
0xff, 0xff, 0xff, 0xff, // Sequence
0x1, // Varint for number of outputs
0xb, 0x7, 0x6, 0x0, 0x0, 0x0, 0x0, 0x0, // Output amount
0x16, // Varint for length of pk script
0x0, // Version 0 witness program
0x14, // OP_DATA_20
0x9d, 0xda, 0xc6, 0xf3, 0x9d, 0x51, 0xe0, 0x39,
0x8e, 0x53, 0x2a, 0x22, 0xc4, 0x1b, 0xa1, 0x89,
0x40, 0x6a, 0x85, 0x23, // 20-byte pub key hash
}
// Add the number of items on the witness stack
witnessItemCountEncoded := []byte{
0x1, // 1 item on the witness stack
}
encodedTxnBuffer = append(
txnBeforeWitnessEncoded,
witnessItemCountEncoded...
)
// Add the size of the witness item
witnessLengthEncoded := []byte{
0xfe, 0x01, 0x09, 0x3d, 0x00, // 4,000,001 bytes
}
encodedTxnBuffer = append(encodedTxnBuffer, witnessLengthEncoded...)
// Add a witness item with 4,000,001 bytes of zeros
for i := uint(0); i < 4_000_001; i++ {
encodedTxnBuffer = append(encodedTxnBuffer, 0)
}
var locktimeEncoded = []byte{
0x0, 0x0, 0x0, 0x0, // Lock time
}
encodedTxnBuffer = append(encodedTxnBuffer, locktimeEncoded...)

return encodedTxnBuffer
}

encodedTxnWithManyWitnessItems := createTxnWithManyWitnessItems()
encodedTxnWithOversizedWitness := createTxnWithOversizedWitness()

tests := []struct {
buf []byte // Wire encoding
pver uint32 // Protocol version for wire encoding
enc MessageEncoding // Message encoding format
version uint32 // Transaction version
err error // Expected error
}{
// Transaction with 1 input and 4,000,001 items in the witness stack
{
buf: encodedTxnWithManyWitnessItems,
pver: pver,
enc: WitnessEncoding,
version: txVer,
err: &MessageError{},
},
// Transaction with 1 input and a large witness script
{
buf: encodedTxnWithOversizedWitness,
pver: pver,
enc: WitnessEncoding,
version: txVer,
err: &MessageError{},
},
}

t.Logf("Running %d tests", len(tests))
for i, test := range tests {
// Decode from wire format.
var msg MsgTx
r := bytes.NewReader(test.buf)
err := msg.BtcDecode(r, test.pver, test.enc)
if reflect.TypeOf(err) != reflect.TypeOf(test.err) {
t.Errorf("BtcDecode #%d wrong error got: %v, want: %v",
i, err, reflect.TypeOf(test.err))
continue
}
}
}

// multiTx is a MsgTx with an input and output and used in various tests.
var multiTx = &MsgTx{
Version: 1,
Expand Down