Page MenuHomePhabricator

Remove segwit recovery activation
AbandonedPublic

Authored by Mengerian on May 17 2019, 17:35.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Maniphest Tasks
T653: Clean up past upgrades
Summary

Remove segwit recovery activation.

Test Plan

make check
Update functional test to check for accepting blocks containing segwit recovery transactions,
rejecting segwit recovery transactions from mempool acceptance, and correct network banning.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
sr-2
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6086
Build 10221: Bitcoin ABC Teamcity Staging
Build 10220: arc lint + arc unit

Event Timeline

florian created this revision.May 17 2019, 17:35
Owners added a reviewer: Restricted Owners Package.May 17 2019, 17:35
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 17 2019, 17:35
florian added a comment.May 17 2019, 17:40

I can split this diff in two, if needed. I'm not sure this test should be created.

src/validation.cpp
736 ↗(On Diff #8717)

I'm using scriptVerifyFlags instead of extraFlags because it's not activation related anymore.

test/functional/abc-segwit-recovery-nonstandardness.py
43 ↗(On Diff #8717)

I suppose the comand line argument -greatwallactivationtime will be removed soon. Decided to use the real value in chainparams for regtest.

Mengerian requested changes to this revision.May 17 2019, 22:31
Mengerian added a subscriber: Mengerian.
Mengerian added inline comments.
test/functional/abc-segwit-recovery-nonstandardness.py
43 ↗(On Diff #8717)

Yeah, I think the GREAT_WALL_START_TIME can just be removed... The test should run the same independent of when activation happened.

Now it's just a matter of deciding what time the test should run at, any arbitrary time would do. I guess whatever other tests do would be fine.

This revision now requires changes to proceed.May 17 2019, 22:31
florian updated this revision to Diff 8731.Sun, May 19, 05:59

Address comment.

deadalnix requested changes to this revision.Mon, May 20, 00:16

Ok the approach this diff takes is a bit backward. It doesn't really remove the activation of anything, it just change the activation point to the genesis block. Here is a better approach:

  • Create a flag that disalow segwit recovery.
  • Make that flag a standard flag.
  • Remove all mention of the flag in all validation path except mempool admission.
src/validation.cpp
736 ↗(On Diff #8731)

Add these to extraFlags. This is important or the error reporting will not be correct.

783 ↗(On Diff #8731)

See here.

This revision now requires changes to proceed.Mon, May 20, 00:16

@deadalnix I spoke with Florian recently, and he said he will be away for the next 30 days.

I will proceed with the flag logic inversion, and adding the flag to standard in separate Diffs, and then commandeer this one if needed to finish the work off.

Mengerian edited reviewers, added: florian; removed: Mengerian.Wed, May 29, 05:44
Mengerian commandeered this revision.

I will rebase this on top of D3148 as-per @deadalnix review comment.

Mengerian retitled this revision from Remove segwit recovery activation to Remove segwit recovery activation Depends on D3248.EditedWed, May 29, 06:07
Mengerian updated this revision to Diff 8985.

Work in progress
Rebase on D3148

Mengerian retitled this revision from Remove segwit recovery activation Depends on D3248 to Remove segwit recovery activation.Wed, May 29, 06:08
Mengerian edited the summary of this revision. (Show Details)
Mengerian edited the summary of this revision. (Show Details)
Mengerian planned changes to this revision.Wed, May 29, 06:11

To do: add to standard flags.

Mengerian updated this revision to Diff 9017.Thu, May 30, 17:39

Change functional test to run at "current" time.
Rebase on updated D3248

Mengerian edited the summary of this revision. (Show Details)Thu, May 30, 17:41
Mengerian updated this revision to Diff 9018.

Edit description.

markblundeberg added inline comments.
test/functional/abc-segwit-recovery-nonstandardness.py
361 ↗(On Diff #9018)

Keep this block.

Mengerian edited the summary of this revision. (Show Details)Thu, May 30, 21:53
Mengerian edited the test plan for this revision. (Show Details)
Mengerian removed a reviewer: florian. Mengerian added 1 blocking reviewer(s): Restricted Owners Package.
Mengerian updated this revision to Diff 9021.

Restore functional test portion that test accepting blocks containing Segwit recovery transactions.

Mengerian updated this revision to Diff 9029.Fri, May 31, 04:28

Rename functional test

Mengerian updated this revision to Diff 9031.Fri, May 31, 05:57

Rename test class

deadalnix requested changes to this revision.Sun, Jun 2, 23:32
deadalnix added inline comments.
src/validation.cpp
725

This is extremely dangerous. You shouldl enable the flag as your default option, then only disable it in specific conditions. Your failure mode must always be to do the right thing.

This revision now requires changes to proceed.Sun, Jun 2, 23:32
Mengerian added a comment.EditedTue, Jun 4, 04:01

@deadalnix I don't have a clear understanding of what you want.

Based on your comments to this Diff, and to D3148, it seems you don't like the intermediate states that result from breaking the Segwit Recovery clean up into three steps.

I made D3194 which squashes the three into one. Please have a look at D3194 to see if this is more along the lines of what you are looking for.

Mengerian abandoned this revision.Wed, Jun 5, 05:07

Abandon in favor of D3194