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.May 19 2019, 05:59

Address comment.

deadalnix requested changes to this revision.May 20 2019, 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.May 20 2019, 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 commandeered this revision.May 29 2019, 05:44
Mengerian edited reviewers, added: florian; removed: Mengerian.

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

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

Work in progress
Rebase on D3148

Mengerian retitled this revision from Remove segwit recovery activation Depends on D3248 to Remove segwit recovery activation.May 29 2019, 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.May 29 2019, 06:11

To do: add to standard flags.

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

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

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

Edit description.

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

Keep this block.

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

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

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

Rename functional test

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

Rename test class

deadalnix requested changes to this revision.Jun 2 2019, 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.Jun 2 2019, 23:32
Mengerian added a comment.EditedJun 4 2019, 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.Jun 5 2019, 05:07

Abandon in favor of D3194