This sets the SCRIPT_ENABLE_SCHNORR flag to be on for all time, retroactively.
The Schnorr activation test is simplified to a feature test.
(effectively this is a backing-out of D2483)
Details
Details
- Reviewers
Mengerian jasonbcox Fabien deadalnix - Group Reviewers
Restricted Project - Maniphest Tasks
- T653: Clean up past upgrades
- Commits
- rSTAGINGfbcfe01efdec: Remove Schnorr activation
rABCfbcfe01efdec: Remove Schnorr activation
make check
test_runner.py --extended
- IBD with -checkpoints=0 and -assumevalid=0 on testnet
- IBD with -checkpoints=0 and -assumevalid=0 on mainnet
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- remschnorr
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 6364 Build 10775: Bitcoin ABC Buildbot (legacy) Build 10774: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
It is very hard to ensure this is correct.
The first step here is to make sure the flag is a standard and mandatory flag and check for this. Once this is the case, each use of the flag can be removed one by one.
Comment Actions
(Note that the functional test is identical to the prior version of this diff. This diff changes all externally-observable behaviour, and the follow ups just perform internal cleanup.)
| test/functional/abc-schnorr.py | ||
|---|---|---|
| 1 ↗ | (On Diff #9234) | It's hard to tell if you made edits to this file. Can you hold the rename until a later diff and just apply any changes you made? That way, we can review the edits and then the name change is a simple, no-line-changes diff. |
| test/functional/timing.json | ||
| 55 ↗ | (On Diff #9424) | ditto |
| test/functional/abc-schnorr.py | ||
|---|---|---|
| 1 ↗ | (On Diff #9234) | Sure, I'll give that a shot! |
Comment Actions
update the functional test per my last comment; we can't properly test 64 byte ECDSA here,
but we can test the multisig length check banning.
| test/functional/abc-schnorr-activation.py | ||
|---|---|---|
| 52 ↗ | (On Diff #9479) | OK, makes sense, the mocktime thing is removed now. And test now uses block_time = int(time.time()) + 1 below inside next_block, which seems to be a very common pattern in other python tests. |
| test/functional/abc-schnorr-activation.py | ||
|---|---|---|
| 97 ↗ | (On Diff #9566) | This should probably be avoided as well. I know it is a common pattern, but this is a bad one. |
Comment Actions
Wait, does that pass IBD? There may be some problem with the sigops count. Please double check before landing.
| test/functional/abc-schnorr-activation.py | ||
|---|---|---|
| 97 ↗ | (On Diff #9566) | Wouldn't it be better to just pick a time in the past and hardcode that here? |
| test/functional/abc-schnorr-activation.py | ||
|---|---|---|
| 97 ↗ | (On Diff #9566) | Hmm, I'll give that past-time thing a try. |
| test/functional/abc-schnorr-activation.py | ||
|---|---|---|
| 97 ↗ | (On Diff #9566) | Interesting, it doesn't work. What happens is that node0 never relays the blocks to node1. The test then fails on sync_blocks(self.nodes), line 262. A brief look in the debug logs shows that node0 never leaves IBD, presumably since it's getting ancient blocks. |
Comment Actions
IBD with checkpoints=0 and assumevalid=0 passed ^
| test/functional/abc-schnorr-activation.py | ||
|---|---|---|
| 97 ↗ | (On Diff #9566) | You can node.generate(1) at the very beginning of the test to bring it out of IBD. Alternatively, don't set this line self.setup_clean_chain = True and just extend the test chain from the latest block time. Both of these approaches require some tweaking of values elsewhere in the test though. As Amaury said, this pattern is common, but bad. I think we should move on from this diff and work to solve this problem holistically in the future. |
| test/functional/abc-schnorr-activation.py | ||
|---|---|---|
| 97 ↗ | (On Diff #9566) | Yes, a couple days ago I was experimenting with writing up another test for something else and I tried the setup_clean_chain=False thing with generate(1) at the start. It is not only nicer, but it also is apparently faster! This test doesn't need to be done the way it's done now, especially since it's not activation-related anymore. |
| test/functional/abc-schnorr-activation.py | ||
|---|---|---|
| 97 ↗ | (On Diff #9566) | Yes it's definitely faster, because the "unclean" chain first start the nodes and build the blocks, then restart the nodes before running the test. These stop/start are wasting time. |