Support having SegWit always active in regtest (sipa, ajtowns, jnewbery) #11389

sipa · 2017-09-23T06:07:04Z

Most tests shouldn't have to deal with the now-historical SegWit activation transition (and other deployments, but SegWit is certainly the hardest one to accomodate).

This PR makes a versionbits starttime of -1 equal to "always active", and enables it by default for SegWit on regtest. Individual tests can override this by using the existing -vbparams option.

A few unit tests and functional tests are adapted to indeed override vbparams, as they specifically test the transition.

This is in preparation for wallet SegWit support, but I thought having earlier eyes on it would be useful.

laanwj · 2017-09-25T11:15:00Z

Concept ACK

jl2012 · 2017-09-25T15:11:22Z

test/functional/sendheaders.py

@@ -215,7 +215,7 @@ def run_test(self):
        connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], inv_node))
        # Set nServices to 0 for test_node, so no block download will occur outside of


Please fix the comment

This was not fixed

TheBlueMatt · 2017-09-26T21:22:07Z

Concept ACK @sdaftuar may be interested.

jonasschnelli · 2017-09-28T02:47:29Z

Yes. Please.

utACK 2902cec68031d61b5707f24edaebb4c26ee1712b

laanwj · 2017-10-05T10:35:58Z

I'm adding this as "high priority for review" because it's a dependency for #11403.

jl2012 · 2017-10-05T12:57:20Z

utACK 2902cec, but please fix comment in sendheaders.py to "Set nServices to NODE_WITNESS for test_node" or something that matches the code

sipa · 2017-10-05T17:57:15Z

@jl2012 Done.

jnewbery

I think this is ok, although it does mean that our default test framework environment is now running with segwit active, but p2sh not active, which is impossible on mainnet (and is in fact causes an assert in the script interpreter here:

bitcoin/src/script/interpreter.cpp

Line 1552 in 17f2ace

assert((flags & SCRIPT_VERIFY_P2SH) != 0);

if a witness block is received)

assumevalid.py fails with this PR, but we can fix that by liberally sprinkling on some "-vbparams=segwit:0:999999999999". I have a commit that does that here: https://github.com/jnewbery/bitcoin/tree/pr11389.

Longer term, I think we should also make P2SH always active in regtest so we're not running in this impossible environment.

EDIT: also appears to break zmq_test.py (which I haven't tested locally or fixed)

jnewbery · 2017-10-05T17:24:20Z

src/test/test_bitcoin.cpp

@@ -105,6 +105,7 @@ TestingSetup::~TestingSetup()

 TestChain100Setup::TestChain100Setup() : TestingSetup(CBaseChainParams::REGTEST)
 {
+    UpdateVersionBitsParameters(Consensus::DEPLOYMENT_SEGWIT, 0, 999999999999ULL);


nit: add comment saying "never activate segwit for these tests"

not addressed in 6998584. I think a comment would make it clearer why you're updating the vb parameter here.

For anyone curious, the problem with these tests is that they create a block via CreateAndProcessBlock(), which uses BlockAssembler::CreateNewBlock(), then it adds transactions to the new block. The IncrementExtraNonce corrects for the new merkle root, but not the new witness root. GenerateCoinbaseCommitment wouldn't work as it only adds a commitment if there's not one there already.

Updating the params here is fine imo, but if anyone would prefer to avoid it, we'd just need a ForceUpdateWitnessCommitment() function.

Added a comment.

jnewbery · 2017-10-05T17:38:49Z

test/functional/nulldummy.py

@@ -40,7 +40,7 @@ class NULLDUMMYTest(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 1
        self.setup_clean_chain = True
-        self.extra_args = [['-whitelist=127.0.0.1', '-walletprematurewitness']]
+        self.extra_args = [['-whitelist=127.0.0.1', '-walletprematurewitness', '-vbparams=segwit:0:999999999999']]


request: add a comment to say that this script tests nulldummy activation, which is implemented to activate at the same height as SegWit, so we set SegWit to be inactive initially.

jnewbery · 2017-10-05T17:41:44Z

test/functional/p2p-fullblocktest.py

@@ -53,6 +53,7 @@ class FullBlockTest(ComparisonTestFramework):
    # Change the "outcome" variable from each TestInstance object to only do the comparison.
    def set_test_params(self):
        self.num_nodes = 1
+        self.extra_args = [["-vbparams=segwit:0:999999999999", "-whitelist=127.0.0.1"]]


I don't think this change is needed. In fact, -whitelist isn't needed either so we could remove this line entirely.

NicolasDorier · 2017-10-06T04:07:47Z

Can we do the same for CSV?

jl2012 · 2017-10-06T08:02:26Z

zmq_test.py failed. I think you need to fix it like you did in #11403

jl2012 · 2017-10-06T08:04:05Z

@NicolasDorier : why CSV? This one is needed because we need to make segwit addresses as default

JeremyRubin · 2017-10-06T17:21:11Z

I think it might be good to make it such that bit must also be set to -1 (or similarly like 65 or something) if the starttime = -1.

This ensures that there aren't further changes needed if the bit is reused by another soft fork.

achow101 · 2017-10-08T21:16:06Z

Concept ACK

jnewbery · 2017-10-09T19:55:04Z

I have a fix for the failing zmq_test.py in my branch: https://github.com/jnewbery/bitcoin/tree/pr11389

NicolasDorier · 2017-10-10T04:44:17Z

@jl2012 well there is no good reason to have a regtest without CSV activated.

sdaftuar · 2017-10-10T19:41:33Z

@jnewbery's observation about p2sh should be addressed: if we are setting segwit to always active in regtest, we should also set p2sh to always be active as well.

However if we think it's likely we'd merge something like what @jl2012 has proposed in #11398, then I think this implementation would immediately go unused, as we'd be replacing the need for the versionbits state machine to have a special hard coded value in favor of putting a hardcoded height in the validation layer above that. I think something like #11398 makes sense, and it supports setting segwit to always being on in regtest as well, so I'd prefer that we do that instead.

theuni · 2017-10-10T20:03:11Z

utACK modulo @jnewbery's comments.

sipa · 2017-10-12T07:29:20Z

I've pushed a rebase with some extra changes:

When a versionbits deployment has a negative start time, it is active from height (the absolute value of) that start time (rather than just -1 meaning always).
BIP16 deployment is changed into a versionbits deployment with accurate negative start times. This is certainly an improvement over the existing hardcoded chain-independent timestamp, though perhaps people prefer a BIP16Height approach or an approach like BIP90: Make buried deployments slightly more easily extensible #11426/Hardcode CSV and SEGWIT deployment #11398. I've not done that to reduce code churn here, and still easily override start times.

In general, I think we should make deployments buried when they're completely over, but there is still value in being able to have a mix, where the versionbits deployment applies to some chains, but regtest can have an overridden always-on or scheduled-at-some-height deployment for easy of testing before the feature is actually buried.

jnewbery · 2017-10-12T18:46:03Z

I much prefer the approach in #11398 . Once a deployment is sufficiently buried, I think we should remove all logic around how that deployment was activated. At that point it doesn't matter how the deployment was activated. We just want to know that if we're on a certain chain at a certain height we should start enforcing a rule.

I think inventing a fiction that old softforks were activated using BIP9 is a complication rather than a simplification.

To respond to your points in #11398:

Advantages:

Easier to review the change that buries a deployment.

You're right that this makes the code delta to bury a BIP9 deployment smaller, but for someone looking at the code for the first time, if (pindex->nHeight >= consensusparams.BIPxxHeight) is much easier to understand than if (VersionBitsState(pindex->pprev, consensusparams, Consensus::DEPLOYMENT_xx, versionbitscache) == THRESHOLD_ACTIVE) where I need to look at and understand the AbstractThresholdConditionChecker class and understand that -ve numbers mean pinned deployments.

Support making a deployment buried in regtest while it isn't yet in mainnet/testnet (which can simplify functional testing)

We already have control to make a BIP9 deployment active at a certain height in regtest using -vbparams. What advantages do you see for making a deployment buried instead of just activated at a height?

I'm ambivalent on #11426. To me that's just a syntax change and I don't think it's materially better or worse than what we have already.

sipa · 2017-10-12T20:20:07Z

@jnewbery Good points, but perhaps I'm not making my intent clear.

I'll start with describing my ideal end goal:

There is an option to configure the height at which (any) consensus rule activated, both for old deployments, and (originally) BIP9 deployments.
When no special activate-at-height is given, the normal activation rules (BIP9 or otherwise) apply.
After a deployment is buried, the height-activation can be made default for that deployment, and its old deployment logic removed. Depending on whether tests need it or historical exceptions, it could be removed entirely and hardcoded.

Having this means that for a new softfork it's possible to write a test that has the new rule always active, or only active at some particular height (and not needing to go through hoops to activate it by mining blocks manually) - while it's not even defined on mainnet.

This functionality partially overlaps with the requirements for #11403 (not needing to go through activation for tests that are unrelated to SegWit activation), which indirectly also needs a way to have P2SH always active.

This PR implements effectively what was described above, but with the caveats (1) only for BIP9 deployments and BIP16 and (2) through a pretty hacky way of classifying BIP16 as a VB deployment. All of that can be cleaned up by having explicit buried deployments, though. This is regtest-only functionality that doesn't need much compatibility between releases IMHO.

I think it's also orthogonal to the question of actually completely removing the segwit or P2SH deployments - both of those are useful too, but as discussed in the meeting today, that may not be super trivial.

Finally,

We already have control to make a BIP9 deployment active at a certain height in regtest using -vbparams. What advantages do you see for making a deployment buried instead of just activated at a height?

That just doesn't accomplish the goal, as BIP9 can at the earliest activate a deployment at 3*interval, which is at height 432 for regtest. For SegWit wallet support I really want a way to have its rules always active. It's certainly possible to go modify all the heights in all the tests, but that's busywork, and it really shouldn't be the test's responsibility to deal with that.

jnewbery · 2017-10-12T21:53:56Z

I'll start with describing my ideal end goal:

Good! We have the same ideal end goal.

This PR implements effectively what was described above, but with the caveats (1) only for BIP9 deployments and BIP16 and (2) through a pretty hacky way of classifying BIP16 as a VB deployment

Yes - that's what I see as a complication rather than a simplification. I prefer #11398 and also pinning SegWit to block 481824. Obviously I won't NACK this if it's a blocker for #11403. In Apache voting terms, I'm a -0.

I think it's also orthogonal to the question of actually completely removing the segwit or P2SH deployments - both of those are useful too, but as discussed in the meeting today, that may not be super trivial.

By that, do you mean pinning the activation to genesis? Yes, agree that it's orthogonal and can be considered later.

That just doesn't accomplish the goal, as BIP9 can at the earliest activate a deployment at 3*interval.

Yes - I understand now.

sipa · 2017-10-12T21:57:16Z

Yes - that's what I see as a complication rather than a simplification. I prefer #11398 and also pinning SegWit to block 481824.

Sure, that's a bigger step towards the intended goal, but I don't see that happening for 0.15.1. #11398 seems pretty invasive.

maflcko

utACK d618458

maflcko · 2017-11-07T20:49:07Z

test/functional/sendheaders.py

@@ -215,7 +215,7 @@ def run_test(self):
        connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], inv_node))
        # Set nServices to 0 for test_node, so no block download will occur outside of


This was not fixed

maflcko · 2017-11-07T20:49:46Z

src/test/test_bitcoin.cpp

@@ -106,6 +106,9 @@ TestingSetup::~TestingSetup()

 TestChain100Setup::TestChain100Setup() : TestingSetup(CBaseChainParams::REGTEST)
 {
+    // CreateAndProcessBlock() does not support building SegWit blocks, so don't activate in these tests.
+    // TODO: fix the code to support SegWit blocks.
+    UpdateVersionBitsParameters(Consensus::DEPLOYMENT_SEGWIT, 0, Consensus::BIP9Deployment::NO_TIMEOUT);


µnit: could use named args /* nStartTime */..., maybe?

maflcko · 2017-11-07T21:38:05Z

test/functional/test_framework/mininode.py

@@ -1649,7 +1649,7 @@ class NodeConn(asyncore.dispatcher):
        "regtest": b"\xfa\xbf\xb5\xda",   # regtest
    }

-    def __init__(self, dstaddr, dstport, rpc, callback, net="regtest", services=NODE_NETWORK, send_version=True):
+    def __init__(self, dstaddr, dstport, rpc, callback, net="regtest", services=NODE_NETWORK|NODE_WITNESS, send_version=True):


µnit: Since you change the default here, no need to overwrite in p2p-segwit.py

ryanofsky

utACK d618458. Changes since last review: numeric limits NO_TIMEOUT value, comment fix, always active comment, fixed -> active rename, python import tweak.

…jtowns, jnewbery) d618458 Have SegWit active by default (Pieter Wuille) 4bd8921 Unit tests for always-active versionbits. (Anthony Towns) d07ee77 Always-active versionbits support (Pieter Wuille) 18e0718 [consensus] Pin P2SH activation to block 173805 on mainnet (John Newbery) 526023a Improve handling of BIP9Deployment limits (Anthony Towns) Pull request description: Most tests shouldn't have to deal with the now-historical SegWit activation transition (and other deployments, but SegWit is certainly the hardest one to accomodate). This PR makes a versionbits starttime of -1 equal to "always active", and enables it by default for SegWit on regtest. Individual tests can override this by using the existing `-vbparams` option. A few unit tests and functional tests are adapted to indeed override vbparams, as they specifically test the transition. This is in preparation for wallet SegWit support, but I thought having earlier eyes on it would be useful. Tree-SHA512: 3f07a7b41cf46476e6c7a5c43244e68c9f41d223482cedaa4c02a3a7b7cd0e90cbd06b84a1f3704620559636a2268f5767d4c52d09c1b354945737046f618fe5

meshcollider · 2017-11-08T03:14:18Z

Post-merge utACK d618458

maflcko · 2017-11-09T19:45:34Z

Removing from backport

b224a47 Add address_types test (Pieter Wuille) 7ee54fd Support downgrading after recovered keypool witness keys (Pieter Wuille) 940a219 SegWit wallet support (Pieter Wuille) f37c64e Implicitly know about P2WPKH redeemscripts (Pieter Wuille) 57273f2 [test] Serialize CTransaction with witness by default (Pieter Wuille) cf2c0b6 Support P2WPKH and P2SH-P2WPKH in dumpprivkey (Pieter Wuille) 37c03d3 Support P2WPKH addresses in create/addmultisig (Pieter Wuille) 3eaa003 Extend validateaddress information for P2SH-embedded witness (Pieter Wuille) 30a27dc Expose method to find key for a single-key destination (Pieter Wuille) 985c795 Improve witness destination types and use them more (Pieter Wuille) cbe1974 [refactor] GetAccount{PubKey,Address} -> GetAccountDestination (Pieter Wuille) 0c8ea63 Abstract out IsSolvable from Witnessifier (Pieter Wuille) Pull request description: This implements a minimum viable implementation of SegWit wallet support, based on top of #11389, and includes part of the functionality from #11089. Two new configuration options are added: * `-addresstype`, with options `legacy`, `p2sh`, and `bech32`. It controls what kind of addresses are produced by `getnewaddress`, `getaccountaddress`, and `createmultisigaddress`. * `-changetype`, with the same options, and by default equal to `-addresstype`, that controls what kind of change is used. All wallet private and public keys can be used for any type of address. Support for address types dependent on different derivation paths will need a major overhaul of how our internal detection of outputs work. I expect that that will happen for a next major version. The above also applies to imported keys, as having a distinction there but not for normal operations is a disaster for testing, and probably for comprehension of users. This has some ugly effects, like needing to associate the provided label to `importprivkey` with each style address for the corresponding key. To deal with witness outputs requiring a corresponding redeemscript in wallet, three approaches are used: * All SegWit addresses created through `getnewaddress` or multisig RPCs explicitly get their redeemscripts added to the wallet file. This means that downgrading after creating a witness address will work, as long as the wallet file is up to date. * All SegWit keys in the wallet get an _implicit_ redeemscript added, without it being written to the file. This means recovery of an old backup will work, as long as you use new software. * All keypool keys that are seen used in transactions explicitly get their redeemscripts added to the wallet files. This means that downgrading after recovering from a backup that includes a witness address will work. These approaches correspond to solutions 3a, 1a, and 5a respectively from https://gist.github.com/sipa/125cfa1615946d0c3f3eec2ad7f250a2. As argued there, there is no full solution for dealing with the case where you both downgrade and restore a backup, so that's also not implemented. `dumpwallet`, `importwallet`, `importmulti`, `signmessage` and `verifymessage` don't work with SegWit addresses yet. They're remaining TODOs, for this PR or a follow-up. Because of that, several tests unexpectedly run with `-addresstype=legacy` for now. Tree-SHA512: d425dbe517c0422061ab8dacdc3a6ae47da071450932ed992c79559d922dff7b2574a31a8c94feccd3761c1dffb6422c50055e6dca8e3cf94a169bc95e39e959

[My cry for help from Slack] I’m playing with heterogenous regtest network, where I have two bitcoin nodes: b1/bitcoind, b2/btcd, initially I cpu-mine 500 blocks on b1 and 1 block on b2, then I connect b1 to b2, mine one extra block on b1 (for block sync to kick in - it does not get triggered automatically for some reason), I also have LN nodes, l1/c-lightning connected to b1, and l2, l3/lnd, connected to b2, I make sure that all l nodes see latest tip of the blockchain (via lncli getinfo), but when I try to open channel between l2 and l3, it complains (in the logs) that segwit is not activated yet, but `b1 getblockchaininfo` reports `bip9_softforks.segwit.status` as `active`, I’m puzzled. Any insights? the error on l3 is: `charlie_1 | 2019-04-28 20:15:50.744 [ERR] FNDG: Unable to broadcast funding tx for ChannelPoint(6c5afbbb0c8b88a6e0390d27e11a04295d54b5dd08d9ebee4ceb0e2ddd46023a:0): -22: TX rejected: transaction 6c5afbbb0c8b88a6e0390d27e11a04295d54b5dd08d9ebee4ceb0e2ddd46023a has witness data, but segwit isn't active yet` as a side note: I was seeing this error in scenarios when I had only btcd and it didn’t have activated segwit, which was solved by mining those 300+ blocks at the beginning `b2 getblockchaininfo` reports `bip9_softforks` as "segwit": { "status": "started", "bit": 1, "startTime": 0, "timeout": 9223372036854775807, "since": 0 } Never mind, I think I found the root cause. bitcoind has segwit auto-enabled since this PR bitcoin/bitcoin#11389. This probably confuses btcd because it follows normal activation rules. I had to add magical `-vbparams=segwit:0:999999999999` to instruct bitcoind to follow normal activation rules for segwit, this seems to keep both nodes on the same consensus about segwit in regtest mode.

Segwit is always activated on regtest: bitcoin/bitcoin#11389 However we need some bitcoins and coinbase Bitcoins can only be spent after 100 confirmations.

fanquake added the Tests label Sep 23, 2017

jl2012 reviewed Sep 25, 2017

View reviewed changes

sipa mentioned this pull request Sep 26, 2017

SegWit wallet support #11403

Merged

laanwj requested a review from sdaftuar October 4, 2017 12:50

maflcko added this to the 0.15.1 milestone Oct 5, 2017

maflcko added the Needs backport label Oct 5, 2017

sipa force-pushed the 201709_itsalwayssegwitinregtestia branch from 2902cec to 94a355c Compare October 5, 2017 17:57

jnewbery reviewed Oct 5, 2017

View reviewed changes

sdaftuar mentioned this pull request Oct 10, 2017

Hardcode CSV and SEGWIT deployment #11398

Closed

sipa force-pushed the 201709_itsalwayssegwitinregtestia branch from 94a355c to 0756d19 Compare October 12, 2017 07:17

ajtowns and others added 2 commits November 6, 2017 19:23

Unit tests for always-active versionbits.

4bd8921

Have SegWit active by default

d618458

sipa force-pushed the 201709_itsalwayssegwitinregtestia branch from ccbc90e to d618458 Compare November 7, 2017 03:29

maflcko reviewed Nov 7, 2017

View reviewed changes

maflcko changed the title ~~Support having SegWit always active in regtest~~ Support having SegWit always active in regtest (sipa, ajtowns, jnewbery) Nov 7, 2017

maflcko reviewed Nov 7, 2017

View reviewed changes

ryanofsky reviewed Nov 7, 2017

View reviewed changes

maflcko merged commit d618458 into bitcoin:master Nov 7, 2017

Sjors mentioned this pull request Nov 9, 2017

Check specific validation error in miner tests #11220

Merged

maflcko removed the Needs backport label Nov 9, 2017

maflcko removed this from the 0.15.2 milestone Nov 9, 2017

ajtowns mentioned this pull request Nov 28, 2017

Assertion failure in validation.cpp:4203 (re: pindexFirstNeverProcessed) #11782

Closed

Sjors mentioned this pull request Jan 9, 2018

Build previous releases and run functional tests #12134

Merged

meshcollider mentioned this pull request Jan 17, 2018

TODO for release notes 0.16.0 #11054

Closed

NicolasDorier mentioned this pull request Jan 18, 2018

[Consensus] Segwit does not activate on BTC regtest stratisproject/StratisBitcoinFullNode#1028

Open

D4nte mentioned this pull request Jun 21, 2019

Segwit is always activated but we need funds comit-network/comit-rs#1049

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		@@ -215,7 +215,7 @@ def run_test(self):
		connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], inv_node))
		# Set nServices to 0 for test_node, so no block download will occur outside of

Support having SegWit always active in regtest (sipa, ajtowns, jnewbery) #11389

Support having SegWit always active in regtest (sipa, ajtowns, jnewbery) #11389

Conversation

sipa commented Sep 23, 2017

laanwj commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Sep 26, 2017

jonasschnelli commented Sep 28, 2017

laanwj commented Oct 5, 2017

jl2012 commented Oct 5, 2017

sipa commented Oct 5, 2017

jnewbery left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasDorier commented Oct 6, 2017

jl2012 commented Oct 6, 2017

jl2012 commented Oct 6, 2017

JeremyRubin commented Oct 6, 2017

achow101 commented Oct 8, 2017

jnewbery commented Oct 9, 2017

NicolasDorier commented Oct 10, 2017

sdaftuar commented Oct 10, 2017

theuni commented Oct 10, 2017

sipa commented Oct 12, 2017

jnewbery commented Oct 12, 2017 • edited Loading

sipa commented Oct 12, 2017 • edited Loading

jnewbery commented Oct 12, 2017

sipa commented Oct 12, 2017 • edited Loading

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

meshcollider commented Nov 8, 2017

maflcko commented Nov 9, 2017

jnewbery left a comment •

edited

Loading

jnewbery commented Oct 12, 2017 •

edited

Loading

sipa commented Oct 12, 2017 •

edited

Loading

sipa commented Oct 12, 2017 •

edited

Loading