Skip to content

Only require 32 byte scripts on the out payment txo, not on every txo in #3

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Only require 32 byte scripts on the out payment txo, not on every txo in #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b17de55
Only require 32 byte scripts on the out payment txo, not on every txo in
kahuang Aug 10, 2022
1994519
Add test
kahuang Aug 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/BtcProofUtils.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ library BtcProofUtils {
// 1. Finally, validate raw transaction pays stated recipient.
BitcoinTx memory parsedTx = parseBitcoinTx(txProof.rawTx);
BitcoinTxOut memory txo = parsedTx.outputs[txOutIx];
require(txo.scriptLen <= 32, "scripts over 32 bytes not supported");
bytes20 actualScriptHash = getP2SH(txo.scriptLen, txo.script);
require(destScriptHash == actualScriptHash, "Script hash mismatch");
require(txo.valueSats >= satoshisExpected, "Underpayment");
Expand Down Expand Up @@ -231,7 +232,6 @@ library BtcProofUtils {
offset += 4;
uint256 nInScriptBytes;
(nInScriptBytes, offset) = readVarInt(rawTx, offset);
require(nInScriptBytes <= 32, "Scripts over 32 bytes unsupported");

@dcposch dcposch Aug 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

one more thing we could do.

so a bit of digging shows that scriptSig (output script) should always be < 32 bytes for all standard bitcoin transaction types

but scriptPubKey (input script) can come from anywhere & can be longer

you could remove bytes32 script entirely from TxInput for some gas savings.

bitcoin mirror users want to prove transactions pay certain people (that's the main use cases) and maybe that a transaction spends a previous utxo (BitcoinTxIn.prevTxId) but i can't think of any reason a user would ever need to prove things about the scriptPubKey (and if they do, they can use offset+length information this parser gives them to read it themselves directly)

txIn.scriptLen = uint32(nInScriptBytes);
txIn.script = bytes32(rawTx[offset:offset + nInScriptBytes]);
offset += nInScriptBytes;
Expand All @@ -254,7 +254,6 @@ library BtcProofUtils {
offset += 8;
uint256 nOutScriptBytes;
(nOutScriptBytes, offset) = readVarInt(rawTx, offset);
require(nOutScriptBytes <= 32, "Scripts over 32 bytes unsupported");
txOut.scriptLen = uint32(nOutScriptBytes);
txOut.script = bytes32(rawTx[offset:offset + nOutScriptBytes]);

@dcposch dcposch Aug 10, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

won't this conversion to bytes32 fail if the slice is longer than 32 bytes?

we can still remove the restriction on input scripts, but we may have to do something like

if (nOutScriptBytes > 32) txOut.script = 0x0;
else txOu.script = bytes32(...);

... and just add a comment on the script field in the struct that it'll be 0 if scriptLen is over 32

@kahuang kahuang Aug 10, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yea wasn't sure how much surgery to do if tests were looking @ this

Is there a reason to parse in txes at all? We don't seem to verify anything from them

@kahuang kahuang Aug 10, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think the conversion truncates, so it doesn't break, its just not 100% correct in the struct. In any case, we validate for the only tx that matters in the function that calls it. Can clean this up before we go live

offset += nOutScriptBytes;
Expand Down
17 changes: 17 additions & 0 deletions src/test/BtcProofUtils.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,14 @@ contract BtcProofUtilsTest is DSTest {
// 1c. finally, verify the recipient of a transaction *output*
bytes constant b0 = hex"0000000000000000000000000000000000000000";

// validate a btc testnet tx
bytes32 constant testHash = hex"0000000082316dc75c041439bad0de12bd5d63d60072f6d998beab875b7d6bf5";

@dcposch dcposch Aug 10, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

👍

bytes constant testHeader = hex"000000200cfa2057879e93554331d13c3d3e1b4c2ae794266211b67602000000000000008ade4c1f4017cd5c2cfa7591c1c687499bdd795eb025c16bc6f8f6ac741ff0d47cd6f262ffff001d086e3eda";
bytes32 constant testTxId = hex"ecc2ee194c359cc6577dadef1f96d5a71fceb1c3dbba4f72bfc86f43e72352c6";
bytes constant testTxMerkleProof = hex"e917037ffe1cdbdbb26c7adc0d05211d0f7bcb975fd80c0670918ea5a19677721eb2a119a1dd1d1c26f6094c666224bd95f58bc624dda43105b70ca3f1f06aaa4aff515e71f351a4f89819075f859fd1cab765bd8143ab04b0a6761f9cd9c91e9a71809c5fe1c8c811b778f8e71d63aa0aaf678ed1e949494a74231643c341c23691816e29a9ffcef3c1074b5f8257c9c4feca0df002733365d1d582b9b75e268129cdfc68c29f74506f3366661eb2ce27632c6137bfb8189a8ecddefd9e31b38e3c746e1923677db2431e90092093a645e9e0c78c8f87c58c084f1c27fcea1e";
bytes constant testRawTx = hex"0200000001215df3a0486c794d69b9940105d8bc65d9d37d632f99449521beac74f5f371c2010000006b483045022100b434afeab3ffb7eb9d97082a8bacd56e7e6356f069cb2c51fb759737ec52ddc4022069bdb95b64aff3baeb18ee3e5f1684c25f08d8ec0357abaa7a7b34307fde2888012102bed852ebbca239a0be85e8997eeaa936b3831607188b87af14df94af2e27494fffffffff02102700000000000017a91483388e7ddf65a4db7ef18c98954b2ff31fe1210f8719430700000000001976a9146b18ccc784fd682a4842ea767561ab084d9004d588ac00000000";


function testGetP2SH() public {
bytes32 validP2SH = hex"a914ae2f3d4b06579b62574d6178c10c882b9150374087";
bytes32 invalidP2SH1 = hex"a914ae2f3d4b06579b62574d6178c10c882b9150374086";
Expand Down Expand Up @@ -328,6 +336,15 @@ contract BtcProofUtilsTest is DSTest {
destScriptHash,
25200001
);


this.validate(
testHash,
BtcTxProof(testHeader, testTxId, 1, testTxMerkleProof, testRawTx),
0,
hex"83388e7ddf65a4db7ef18c98954b2ff31fe1210f",
1000
);
}

function validate(
Expand Down
2 changes: 1 addition & 1 deletion submitter/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ async function main() {
console.log("not enough new blocks");
return;
}
const targetHeight = Math.min(btcLatestHeight, mirrorLatestHeight + 30);
const targetHeight = Math.min(btcLatestHeight, mirrorLatestHeight + 20);

@kahuang kahuang Aug 10, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

with 30 I was running into rate limiting from the getblocks api


// then, find the most common ancestor
console.log("finding last common Bitcoin block headers");
Expand Down