lightninglabs / neutrino

Privacy-Preserving Bitcoin Light Client
MIT License
900 stars 183 forks source link

verification: skip checking input scripts #234

Closed ellemouton closed 2 years ago

ellemouton commented 2 years ago

In this commit, the checks for input script pub keys is removed since the function used to reconstruct script pub keys from witness data currently does not take taproot outputs into consideration and so is producing the wrong script pubkeys which then causes the verification to fail. This code can be put back once the ComputeScriptPubKey function takes taproot into account.

off the top of my head, I think that the ComputeScriptPubKey function might need to be changed to return a set of options since i think sometimes we wont be able to tell the difference between a P2WPKH, P2TR key path with annex, P2TR script path.

chappjc commented 2 years ago

off the top of my head, I think that the ComputeScriptPubKey function might need to be changed to return a set of options since i think sometimes we wont be able to tell the difference between a P2WPKH, P2TR key path with annex, P2TR script path.

It might not be possible to tell the difference between v0 P2WSH and P2WPKH either since it's possible to have a 33-byte redeem script with one other item in the witness stack, so that it will look like a keyhash script when it's really a scripthash script.

More generally though, given that the witness script version is in the output script, which this is trying to find, I don't know how feasible it is do go from witness data -> pkscript.

Roasbeef commented 2 years ago

I'm curious what can really be done with an input's witness data given that the witness version is not known without the script itself, and future versions could have a witness stack data pattern that matches multiple witness output script versions.

Yeah it's difficult, the heursitic was mean to be moreso on a best effort basis to ensure that spends we cared about (p2wsh multi-sig mainly in the case of LN) were properly included in the filter. Re that single stack element, it's possible that we pattern match further by asserting it's a pubkey in the new taproot format. However not clear where that ends as it becomes a game a wack-a-mole.

What I think we really need here is a new p2p message that allows nodes to fetch the undo data, which would include the prev pkScripts (we can verify this is actually legit), and use that to assert the validity of a filter.

In the short term, I think this PR is the right move, but maybe we keep the code in tact and instead just log instead of erroring out? Want to think of a way we can still retain the extra validation in the short term...

ellemouton commented 2 years ago

Want to think of a way we can still retain the extra validation in the short term

One thing i can think of is: if we downloaded this block cause it matched on one of our inputs, we can pass these inputs into the function so that if we get to a tx that spends one of our txid:vout's, we defs will know the scriptPubKey that should match against the filter. No guessing game in that case about what the correct scriptPubKey for the witness should be.

sipa commented 2 years ago

I'm not enough up to date to know if this is relevant, but:

BIP341 was designed to permit some recognizability of what type a spend is without seeing the UTXO being spent.