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 Passed
Unit
No Test Coverage
Build Status
Buildable 6085
Build 10219: Bitcoin ABC Buildbot (legacy)
Build 10218: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.May 17 2019, 17:35

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
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 edited reviewers, added: florian; removed: Mengerian.

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.

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)

To do: add to standard flags.

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

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 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.

deadalnix requested changes to this revision.Jun 2 2019, 23:32
deadalnix added inline comments.
src/validation.cpp
725 ↗(On Diff #9031)

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

@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.