search

tradecraftio / tradecraft

Tradecraft integration/staging tree https://tradecraft.io/download
Other
13 stars 9 forks source link

Add version prefix to witness redeem script #52

Closed maaku closed 4 years ago

maaku commented 4 years ago

Work in progress; not ready for merge.

This pull request adds an script inner-version and a prefix byte to the witness redeem script. This allows for script versions to be updated without "wasting" one of the remaining segwit version prefixes in the scriptPubKey. In essence the segwit script version is only needed to specify changes to the way in which the script is hashed (e.g. taproot would be an example), whereas upgrades in script functionality can be accomplished with the inner-version prefixed to the script itself. The prefix byte indicating a V0_WITNESS script is the NOP opcode, so most code not aware of script prefixes would continue to work (for now).

In terms of consensus code, I much prefer this approach to alternatives, such as making all unused opcodes have return True semantics. There's less opportunity for foot-gun interactions, the script interpreter is not required to handle other prefix codes that specify non-script behavior, and it provides a common mechanism for specifying script engines and/or versions when recursing, whether by tail-call or taproot/graftroot.

However in implementation it became clear that the wallet code is not written in a way that makes it very easy to support version prefixes. So some refactoring of the wallet may need to occur before work continues on this patch set.

Note that RPCs have not been updated, there's still 4 failures in the unit test suite, and there's probably a LOT of tests that need to be updated to have the proper script version prefix--if the test was checking that validation succeeds, then it will always succeed if it lacks the right prefix, causing the silent error of the test not testing anymore what it was supposed to test.

maaku commented 4 years ago

All tests have been updated. There is a residual issue being tracked in #61 but it is not a blocker for merge.