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");
Duplicated line?
Yes? :trollface:
Fixed
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)
Any good reason for reference?
None. Removed.
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 */
Is it helpful to continue referencing the BIP numbers in these comments? I would guess so.
Sure. Comments added.
Why not refer to the BIP numbers solely instead of alternating?
the segwit deployment covered more than one bip
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));
Nit: any reason this isn't just rv.pushKV("active", ...); for consistency?
ah. It's just been merged. Will rebase.
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"
We don't seem to have any testing for the contents of this key - is this the case? Happy to either write some for this PR or file a follow-up.
You're right. 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?
Oops. Bad rebase. Should be fixed now.
utACK 0be1f9d8a6bb2b8c47d6816b46d6764aee3cc06e
rebased
Rebased.
Rebased
1158 | @@ -1158,6 +1159,21 @@ bool AppInitParameterInteraction() 1159 | } 1160 | } 1161 | } 1162 | +
This can be done directly in CRegtestParams's constructor, that way, the function UpdateSegwitHeight would not be needed.
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.
I guess we still need UpdateSegwitHeight() then, or perhaps we can pass the other args (instead of g_args, or rather g_args plus "-segwitheight=432") to SelectParams in https://github.com/bitcoin/bitcoin/pull/12360/files#diff-d5ba361c5f8be78eb4cc0c787c1fc78eR54 ? I guess not all BasicTestingSetup want 432 there...
I think I'll leave it as is for now. It may be possible to tidy this up in a future commit.
Rebased
rebased
Tend to NACK. Just because buried deployments can be done safely doesn't mean they should be done for the sole purpose of code refactoring without substantial motivation, advantages or simplifications. Since this is refactoring consensus code and also changing the consensus rules, I think a writeup on the motivation and tradeoffs is required. Note that none of the statements of the motivation given in BIP90 are applicable here, so I doubt that the overall gain (if any) is worth the time spent on this (code/discussions/formal writeups/review/...). I'd rather keep the deployments as versionbits deployments and instead treat 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.
Potentially also the mempool, after which #12124 could be rebased and reopened.
@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.
rebased
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");
It seems odd to add code just for the tests, that has no application outside of regtest. I'd prefer if the code paths in the tests that test segwit can be disabled are just removed. Afterward this code block and all other dead code (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.
Comment needs update to remove "BIP9"?
fixed (here and in p2p_segwit.py).
Fixed @MarcoFalke's comment and tidied up p2p_segwit.py a little.
rebased
Could the rpc changes potentially be split up into a separate pull request, to not distract from the consensus changes?
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.
rebased
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.
Fixed failing p2p_segwit.py
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.
<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase
Closing - 6 concept ACKs but no review so I guess this isn't worth rebasing.