Remove segwit recovery activation.
Details
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) Restricted Project - Maniphest Tasks
- T653: Clean up past upgrades
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
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- sr-2
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 6074 Build 10197: Bitcoin ABC Buildbot (legacy) Build 10196: arc lint + arc unit
Event Timeline
Comment Actions
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. |
| 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. |
Comment Actions
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. |
Comment Actions
@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.
| test/functional/abc-segwit-recovery-nonstandardness.py | ||
|---|---|---|
| 361 | Keep this block. | |
Comment Actions
Restore functional test portion that test accepting blocks containing Segwit recovery transactions.
| 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. |
Comment Actions
@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.