Page MenuHomePhabricator

Add SCRIPT_ALLOW_SEGWIT_RECOVERY
ClosedPublic

Authored by florian on Jan 31 2019, 20:54.

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
Summary

Flag that allows the spending of p2sh segwit coins

Test Plan

test_bitcoin --run_test=script_tests

Diff Detail

Event Timeline

Owners added a reviewer: Restricted Owners Package.Jan 31 2019, 20:54

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.

deadalnix requested changes to this revision.Jan 31 2019, 21:33

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.

This revision now requires changes to proceed.Jan 31 2019, 21:33
florian added inline comments.
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.

florian marked 2 inline comments as done.

Simplified the changes in VerifyScript

deadalnix requested changes to this revision.Feb 1 2019, 00:18

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:

  1. create a segwit output
  2. try to recover it, it fails. Check mempool and in blocks.
  3. activate the fork
  4. try to recover, it works. Check mempool and blocks.
  5. reorg one block.
  6. the transaction is still in the mempool.
  7. reorg one more block.
  8. 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...

This revision now requires changes to proceed.Feb 1 2019, 00:18

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.

florian requested review of this revision.Feb 2 2019, 22:04
deadalnix requested changes to this revision.Feb 2 2019, 23:28

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.

This revision now requires changes to proceed.Feb 2 2019, 23:28
florian retitled this revision from Add SCRIPT_ENABLE_CLEANSTACK_EXCEPTION to Add ALLOW_SEGWIT_RECOVERY.Feb 3 2019, 02:24
florian edited the summary of this revision. (Show Details)

Refactored to be restrictive to segwit programs.

florian planned changes to this revision.Feb 3 2019, 02:26
florian retitled this revision from Add ALLOW_SEGWIT_RECOVERY to Add SCRIPT_ALLOW_SEGWIT_RECOVERY.Feb 3 2019, 02:32

Removed unused parameters from IsWitnessProgram

deadalnix requested changes to this revision.Feb 3 2019, 03:04
deadalnix added inline comments.
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.
Too short is also not tested.

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.
You also need to check valid version other than 0.

This revision now requires changes to proceed.Feb 3 2019, 03:04
florian requested review of this revision.Feb 3 2019, 03:37
florian marked 3 inline comments as done.
florian added inline comments.
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.

Included a unit test for IsWitnessProgram, a few other test cases for ALLOW_SEGWIT_RECOVERY and created a wrapper for IsWitnessProgram()

deadalnix requested changes to this revision.Feb 3 2019, 17:42
deadalnix added inline comments.
src/test/script_tests.cpp
2416 ↗(On Diff #7128)

Use BOOST_CHECK_EQUAL

Also check that the parsed version/hash are correct.

This revision now requires changes to proceed.Feb 3 2019, 17:42
florian marked an inline comment as done.

Included checks of the parsed results of IsWitnessProgram

This revision is now accepted and ready to land.Feb 4 2019, 12:51
This revision was automatically updated to reflect the committed changes.