This hardcodes CSV and segwit activation heights, similar to the BIP 90 buried deployments for BIPs 34, 65 and 66.
Both CSV and segwit have been active for over 8 months. Hardcoding the activation height is a code simplification, makes it easier to understand segwit activation status, and reduces technical debt.
1105- rv.push_back(Pair("id", name));
1106- rv.push_back(Pair("version", version));
1107- rv.push_back(Pair("reject", SoftForkMajorityDesc(version, pindex, consensusParams)));
1108- return rv;
1109+ rv.pushKV("type", "buried");
1110+ rv.pushKV("type", "buried");
58@@ -59,6 +59,11 @@ void CChainParams::UpdateVersionBitsParameters(Consensus::DeploymentPos d, int64
59 consensus.vDeployments[d].nTimeout = nTimeout;
60 }
61
62+void CChainParams::UpdateSegwitHeight(const int& height)
55@@ -58,6 +56,10 @@ struct Params {
56 int BIP65Height;
57 /** Block height at which BIP66 becomes active */
58 int BIP66Height;
59+ /** Block height at which CSV becomes active */
60+ int CSVHeight;
61+ /** Block height at which Segwit becomes active */
1107- rv.push_back(Pair("reject", SoftForkMajorityDesc(version, pindex, consensusParams)));
1108- return rv;
1109+ rv.pushKV("type", "buried");
1110+ // getblockchaininfo reports the softfork as active from when the chain height is
1111+ // one below the activation height
1112+ rv.push_back(Pair("active", chainActive.Tip()->nHeight + 1 >= height));
rv.pushKV("active", ...); for consistency?
1169- " \"reject\": { (object) progress toward rejecting pre-softfork blocks\n"
1170- " \"status\": xx, (boolean) true if threshold reached\n"
1171+ " \"chainwork\": \"xxxx\" (string) total amount of work in active chain, in hexadecimal\n"
1172+ " \"pruned\": xx, (boolean) if the blocks are subject to pruning\n"
1173+ " \"pruneheight\": xxxxxx, (numeric) lowest-height complete block stored\n"
1174+ " \"softforks\": { (object) status of softforks\n"
rpc_blockchain.py should be updated. If you’re happy to contribute a commit to add tests, I can include it in this PR.
1154@@ -1160,36 +1155,28 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
1155 " \"difficulty\": xxxxxx, (numeric) the current difficulty\n"
1156 " \"mediantime\": xxxxxx, (numeric) median time for the current best block\n"
1157 " \"verificationprogress\": xxxx, (numeric) estimate of verification progress [0..1]\n"
1158- " \"initialblockdownload\": xxxx, (bool) (debug information) estimate of whether this node is in Initial Block Download mode.\n"
Curious why removal of this key and others (e.g. size_on_disk) is bundled with this changeset. Seems like RPC interface changes should be handled separately.
Edit: looks like these were just removed from the help message; still probably don’t want to do that in this changeset, no?
1158@@ -1158,6 +1159,21 @@ bool AppInitParameterInteraction()
1159 }
1160 }
1161 }
1162+
Seems like a good idea, but segwit height is also updated in one of the unit tests:
https://github.com/bitcoin/bitcoin/pull/12360/files#diff-d5ba361c5f8be78eb4cc0c787c1fc78eR118
I think we need the UpdateSegwitHeight() function call there.
IsWitnessEnabled as true for modules that don’t need to know when the witness commitment started to be a requirement for blocks containing witness transactions. I’d guess compact block relay qualifies for this, since it only relays recent blocks. Alternatively the “mining” code, which only makes sense to call on chains where IsWitnessEnabled is already true.
@MarcoFalke - I disagree. Once a softfork is deployed and is sufficiently buried, the activation mechanism is purely academic. Removing complexity and improving the simplicity of the code is motivation enough.
I think a writeup on the motivation and tradeoffs is required
Yes - I intend to send a writeup to the mailing list if this PR is merged.
As a meta-point, although review and feedback is always welcome, concept NACKs are much more useful early after a PR has been opened. This PR is a rebase of #11398, which was opened 7 months ago and has now been rebased ~10 times (including once requested by you). This PR and its antecedent have already been ACKed or concept ACKed by 5 contributors. If your objection is time spent on discussion/review, then that ship has already sailed :)
Removing complexity and improving the simplicity of the code is motivation enough.
I am not aware of any plans to get rid of the concept of versionbits-deployments, so the “simplifications and improvements” of the code are limited to that module of the code and don’t have beneficial side-effects or simplifications (such as BIP-90).
I am not saying that the changes can not be made, but rather that there is no need to to them right now. It is good to keep this patch in mind and there is no harm in having it in a rebased form… My Concept NACK is more a -0.
Please add some minimal motivation to the opening comment, if you plan to keep on curating this pull. I’d assume for new developers those changes to the consensus code are hard to follow in the absence of any motivation and the presence of merely a blob of code.
1158+ int64_t height = gArgs.GetArg("-segwitheight", chainparams.GetConsensus().SegwitHeight);
1159+ if (height < -1 || height >= std::numeric_limits<int>::max()) {
1160+ return InitError(strprintf("Activation height %ld for segwit is out of valid range. Use -1 to disable segwit.", height));
1161+ }
1162+ else if (height == -1) {
1163+ LogPrintf("Segwit disabled for testing\n");
IsScriptWitnessEnabled, …) can be discarded.
I completely agree. The only reason the ‘Segwit disabled’ code here and elsewhere exists is for p2p_segwit.py, where it’s used to test whether GBT doesn’t return a default witness commitment if segwit is disabled. That’s completely circular - we’re adding behaviour into the product so we can test whether that behaviour exists.
I’d like to completely overhaul p2p_segwit.py and remove this special casing, but in a future PR. This PR is simply maintaining the existing functionality of setting -vbparams=segwit:0:0.
In my view, segwit activation at some height is the consensus that the software assumes – given that our own wallet now generates segwit transactions by default it would be internally inconsistent to tolerate being on a chain on which segwit might never be active. Moreover, we’re contemplating merging a change in #13120 that would change the mempool logic to always accept segwit transactions, even if we were on a chain on which segwit was not active.
I do agree with @MarcoFalke’s comment that this is not a necessary change; however I think it is still a desirable one – a height check seems simpler than VersionBits-based logic, even if making that change touches a bunch of code. For instance, future code readers interested in segwit logic should not have to wonder under what circumstances the VersionBits calls might not return THRESHOLD_ACTIVE. Further, I think replacing the VB logic with a height check also more accurate captures the uncertainty around the activation, where it was unclear whether segwit activated according to the rules specified in BIP 141, BIP 91, or BIP 148 (or something else?). Maintaining the software’s BIP 141 activation logic is an unnecessary and somewhat arbitrary burden, even if so far it has been a light one.
I am not aware of any plans to get rid of the concept of versionbits-deployments, so the “simplifications and improvements” of the code are limited to that module of the code and don’t have beneficial side-effects or simplifications (such as BIP-90).
I disagree with the second half that sentence – I think that the circumstances around whether segwit is or might be active do concern other parts of the code (such as the wallet, mempool, and mining, which must make choices around what transactions to generate, accept or select) and therefore it’s easier to reason about the possibilities if we remove chain-specific calculations and replace them with the height checks proposed here.
In summary, I think replacing the activation logic with a height check makes the code more internally consistent and simpler to understand and reason about, so I think this is worth doing. Concept ACK.
41@@ -42,9 +42,9 @@ def set_test_params(self):
42 self.setup_clean_chain = True
43 self.num_nodes = 3
44 # This test tests SegWit both pre and post-activation, so use the normal BIP9 activation.
p2p_segwit.py).
Could the rpc changes potentially be split up into a separate pull request, to not distract from the consensus changes?
It’s possible, but this PR already has two ACKs and a Concept ACK. I think that splitting it up would be generating more work for reviewers.
This combines reporting of buried (formally ISM) softfork deployments
and BIP9 versionbits softfork deployments into one JSON object in the
getblockchaininfo return object.
It also removes the redundant feature_bip9_softforks.py.
feature_bip9_sofforks.py was intended to be a generic test for versionbits
deployments. However, it only tests CSV activation and was not updated
to test segwit activation. CSV activation is tested by
feature_csv_activation.py, so this test is duplicated effort.
Hard code CSV deployment height to 419328 for mainnet.
It’s possible, but this PR already has two ACKs and a Concept ACK. I think that splitting it up would be generating more work for reviewers.
The ACK are outdated and would need to be re-ACKed on the new commits anyway.
Hardcode segwit deployment height to 481824 for mainnet.
This is a revamped version of @jl2012’s #11398, which had concept ACKs from @gmaxwell @dcousens and @sdaftuar . This PR also has ACKs from @jamesob and @jtimon .
Are people still interested in this? I haven’t had any feedback for the last three rebases (except @MarcoFalke’s NACKs), and I’d rather not continue rebasing if people have cooled on this idea.
