Flag that allows the spending of p2sh segwit coins
Details
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) Restricted Project - Commits
- rSTAGING0d94d9df7e42: Add SCRIPT_ALLOW_SEGWIT_RECOVERY
rABC0d94d9df7e42: Add SCRIPT_ALLOW_SEGWIT_RECOVERY
test_bitcoin --run_test=script_tests
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 4843 Build 7749: Bitcoin ABC Buildbot (legacy) Build 7748: arc lint + arc unit
Event Timeline
Comment Actions
Beautifully simple way to do an exception, which is reflected in the shortness of the Diff.
I want to think a bit about this whether there is some kind of weird loophole, but it looks solid.
Comment Actions
I'm very happy this is happening. Thanks for your work. The overall approach is sound, but there is a lot of details that can be polished. I put some comment that should help you do that.
| src/script/interpreter.cpp | ||
|---|---|---|
| 1587 ↗ | (On Diff #7078) | This whole function is very convoluted and has a lot of unnecessary duplication. While I do not expect it to fix it all, at least we should make sure that this isn't getting worse. |
| 1590 ↗ | (On Diff #7078) | Define this closer to it's use. A comment to explain what this is would be useful. |
| 1639 ↗ | (On Diff #7078) | IMO it is best to check for segwit specifically here. |
| 1661 ↗ | (On Diff #7078) | This should be refactored as this is VERY unreadable atm. You can for instance bail early if stack.size() == 1 and then figure out if there is an error and if so what should be done about it. |
| src/script/interpreter.cpp | ||
|---|---|---|
| 1639 ↗ | (On Diff #7078) | Good idea. I can limit all necessary changes to this P2SH conditional if I bail out early when all conditions are met, hence returning before the CLEANSTACK verification below. |
| 1661 ↗ | (On Diff #7078) | I'm returning this part to the original form, since no other change is needed for this diff. |
Comment Actions
It's getting there. Thanks for picking this up. I would suggest to work in parallel on the activation code. You can dig in the history to figure this out. The test case should go as follow:
- create a segwit output
- try to recover it, it fails. Check mempool and in blocks.
- activate the fork
- try to recover, it works. Check mempool and blocks.
- reorg one block.
- the transaction is still in the mempool.
- reorg one more block.
- the transaction has been removed from the mempool because it is invalid now.
You can grind in the software history to figure it out. We did so every time we activated one of these flags, so there are many examples to draw from.
| src/script/interpreter.cpp | ||
|---|---|---|
| 1642 ↗ | (On Diff #7080) | I think this would benefit from back-porting https://github.com/bitcoin/bitcoin/blob/master/src/script/script.cpp#L216 Then if the stack is empty, and the script is a segwit program, then this an bail immediately. |
| src/script/script_flags.h | ||
| 109 ↗ | (On Diff #7080) | SCRIPT_ALLOW_SEGWIT_RECOVERY or alike. As we've seen in the current code, it doesn't have to be intertwined with clean stack in any way. |
| src/test/script_tests.cpp | ||
| 1549 ↗ | (On Diff #7080) | Adding the segwit check would require to increase the amount of tests cases. Interestingly enough: $ grep -r src/test/ -e 'IsWitnessProgram' | wc -l 0 So, thanks core, I guess... |
Comment Actions
Oh I see. My original intention was to converge on the simplest solution (spec, implementation and verification-wise) that allowed the retrieval of segwit coins while keeping all the malleability gains from the cleanstack rule. This proposal was not specific to segwit so the requirement description (which I still need to write and create a PR on github) and verification are relatively simple. Is it really your wish to be this specific on segwit? All those segwit specific requirements (e.g: redeem script size between 4 and 42, starting with OP_0, ..., OP_16, etc) will have to go into the spec for the HF and, as you pointed out, will require a few extra tests.
I can execute your suggestion rather quickly, I'm just waiting for one last consideration on the extra (and maybe unnecessary) complexity of this new proposal.
BTW, IsWitnessProgram is still in ABC's codebase, no back-port needed =)
I'll start working on the activation code right away.
Comment Actions
I think it makes sense to be as restrictive as possible. We can still allow more in the future, but gong from more to less require to have special code to handle historical artifacts, which is not great.
| src/script/script.h | ||
|---|---|---|
| 636 ↗ | (On Diff #7127) | I would highly advise to keep the code as this. If you want t a convenience function, use a wrapper. This also needs a unit test. |
| src/test/script_tests.cpp | ||
| 1548 ↗ | (On Diff #7127) | Using a uint256 would ensure by definition that this is the right size. |
| 1580 ↗ | (On Diff #7127) | Take the previous valid value and add/remove one byte. If not,this too error prone. |
| 1589 ↗ | (On Diff #7127) | I'm pretty sure you can send 17 in there directly. Or at least that there is something significantly simpler than this. |
| src/script/script.h | ||
|---|---|---|
| 636 ↗ | (On Diff #7127) | Ok, I'll create a wrapper. Would you like me to write a new test/*.cpp file to test it? Aren't the tests integrating ALLOW_SEGWIT_RECOVERY and IsWitnessProgram enough after I include the ones you asked? I believe the test cases for IsWitnessProgram alone would be close to those. |
Comment Actions
Included a unit test for IsWitnessProgram, a few other test cases for ALLOW_SEGWIT_RECOVERY and created a wrapper for IsWitnessProgram()
| src/test/script_tests.cpp | ||
|---|---|---|
| 2416 | Use BOOST_CHECK_EQUAL Also check that the parsed version/hash are correct. | |