-
Notifications
You must be signed in to change notification settings - Fork 7
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.
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
Only require 32 byte scripts on the out payment txo, not on every txo in #3
base: master
Are you sure you want to change the base?
Conversation
the transaction...
✅ Deploy Preview for distracted-gates-f46892 canceled.
|
@@ -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); |
There was a problem hiding this comment.
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
@@ -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]); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
@@ -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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -231,7 +232,6 @@ library BtcProofUtils { | |||
offset += 4; | |||
uint256 nInScriptBytes; | |||
(nInScriptBytes, offset) = readVarInt(rawTx, offset); | |||
require(nInScriptBytes <= 32, "Scripts over 32 bytes unsupported"); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the transaction...