Default to NODE_WITNESS in nLocalServices

dhruv commented at 8:30 pm on February 5, 2021: member

Builds on #21009 and makes progress on remaining items in #17862

Removing RewindBlockIndex() in #21009 allows the following:

removal of tests using segwitheight=-1 in p2p_segwit.py.
move test_upgrade_after_activation() out of p2p_segwit.py reducing runtime
in turn, that allows us to drop support for -segwitheight=-1, which is only supported for that test.
that allows us to always set NODE_WITNESS in our local services. The only reason we don’t do that is to support -segwitheight=-1.
that in turn allows us to drop all of the GetLocalServices() & NODE_WITNESS checks inside net_processing.cpp, since our local services would always include NODE_WITNESS

dhruv force-pushed on Feb 5, 2021

DrahtBot added the label P2P on Feb 5, 2021

DrahtBot added the label Validation on Feb 5, 2021

DrahtBot commented at 1:22 am on February 6, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20362 by MarcoFalke
#18554 by rodentrabies

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj referenced this in commit 19a56d1519 on Apr 27, 2021

MarcoFalke added the label Needs rebase on Apr 27, 2021

MarcoFalke removed the label Validation on Apr 27, 2021

jnewbery commented at 9:00 am on April 27, 2021: member

This needs rebase now that #21009 is merged. I did my own rebase here: https://github.com/jnewbery/bitcoin/tree/pr21090.1 since I need it for #20799. Feel free to take that if it’s helpful.

I think it might make sense to remove the test_upgrade_after_activation() subtest from p2p_segwit.py and place it in its own file. It’s not anything to do with p2p and doesn’t have any dependencies on the rest of that test.

sidhujag referenced this in commit e3a02a794a on Apr 27, 2021

dhruv force-pushed on Apr 29, 2021

dhruv marked this as ready for review on Apr 29, 2021

dhruv commented at 6:16 pm on April 29, 2021: member

Rebased against master, ready for review. @jnewbery it does make a ton of sense to extract test_upgrade_after_activation() into another file. Once -reindex has the expected effect, the rest of the test merely verifies that the node syncs on to the strongest chain, and that is in covered in other tests. I’ve gone ahead and done that refactor. This also simplified p2p_segwit.py since it now only needs two nodes for the rest of the test cases.

in src/chainparams.cpp:494 in 117ae3f264 outdated

489@@ -490,11 +490,8 @@ void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager& args)
490 {
491     if (args.IsArgSet("-segwitheight")) {
492         int64_t height = args.GetArg("-segwitheight", consensus.SegwitHeight);
493-        if (height < -1 || height >= std::numeric_limits<int>::max()) {
494+        if (height < 0 || height >= std::numeric_limits<int>::max()) {
495             throw std::runtime_error(strprintf("Activation height %ld for segwit is out of valid range. Use -1 to disable segwit.", height));

MarcoFalke commented at 6:42 pm on April 29, 2021:

0            throw std::runtime_error(strprintf("Activation height %ld for segwit is out of valid range.", height));

dhruv commented at 1:05 am on April 30, 2021:

Done.

dhruv force-pushed on Apr 29, 2021

dhruv force-pushed on Apr 30, 2021

in src/net_processing.cpp:2688 in f43a12789b outdated

2584@@ -2586,7 +2585,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2585         bool fAnnounceUsingCMPCTBLOCK = false;
2586         uint64_t nCMPCTBLOCKVersion = 0;
2587         vRecv >> fAnnounceUsingCMPCTBLOCK >> nCMPCTBLOCKVersion;
2588-        if (nCMPCTBLOCKVersion == 1 || ((pfrom.GetLocalServices() & NODE_WITNESS) && nCMPCTBLOCKVersion == 2)) {
2589+        if (nCMPCTBLOCKVersion == 1 || nCMPCTBLOCKVersion == 2) {

dhruv commented at 1:03 am on April 30, 2021:

I think the only allowed values for nCMPCTBLOCKVersion are 1 or 2. Can this conditional be removed altogether (especially since fWantsCmpctWitness is conditioned appropriately below)? Or perhaps replaced with if (0 < nCMPCTBLOCKVersion < 3) ?

jnewbery commented at 7:11 am on April 30, 2021:

Yes, the only permitted values for nCMPCTBLOCKVersion are 1 and 2, which is why this conditional is here. We want to ignore any sendcmpct message received where nCMPCTBLOCKVersion is not 1 or 2.

if (0 < nCMPCTBLOCKVersion < 3) does not do what you I think you expect it to do.

dhruv commented at 1:51 pm on April 30, 2021:

Ok, I’ll leave the conditional in there then. Nice mental compiling! For others following along if (0 < nCMPCTBLOCKVersion < 3) ends up comparing a bool resulting from the first half of the expression to 3. Fortunately clang++ issues a warning.

0warning: result of comparison of constant 3 with
1      expression of type 'bool' is always true
2      [-Wtautological-constant-out-of-range-compare]
3    if (0 < i < 3) {

dhruv commented at 1:04 am on April 30, 2021: member

Rebased with master. Weirdly DrahtBot didn’t mention it was needed (but the CI did).

Ready for further review with one question below.

DrahtBot removed the label Needs rebase on Apr 30, 2021

in test/functional/p2p_segwit.py:222 in f43a12789b outdated

221@@ -224,7 +222,6 @@ def skip_test_if_missing_module(self):
222     def setup_network(self):

jnewbery commented at 7:28 am on April 30, 2021:

This method override can be removed entirely, since it’s doing the same as the default setup_network() in test_framework.py

dhruv commented at 2:11 pm on April 30, 2021:

Done

in test/functional/feature_presegwit_node_upgrade.py:18 in f43a12789b outdated

15+        self.setup_clean_chain = True
16+        self.num_nodes = 1
17+
18+    def setup_network(self):
19+        self.setup_nodes()
20+

jnewbery commented at 7:32 am on April 30, 2021:

Not needed. This is default behaviour.

dhruv commented at 2:11 pm on April 30, 2021:

Done

in test/functional/feature_presegwit_node_upgrade.py:20 in f43a12789b outdated

20+
21+    def run_test(self):
22+        self.test_upgrade_after_activation()
23+
24+    def test_upgrade_after_activation(self):
25+        """A pre-segwit node with insufficiently validated blocks needs to redownload blocks"""

jnewbery commented at 7:33 am on April 30, 2021:

No need to have a run_test() method that only calls one method. Just put the logic in run_test().

0    def run_test(self):
1        """A pre-segwit node with insufficiently validated blocks needs to redownload blocks"""

dhruv commented at 2:16 pm on April 30, 2021:

I am not sure I understand. It seems we call main() which then calls test_framework.py::run_test(). I am not sure how I could remove this run_test() and put the logic in test_framework.py::run_test()?

Also, it seems BitcoinTestFramework requires overriding run_test().

jnewbery commented at 2:58 pm on April 30, 2021:

Have you tried applying this diff?

dhruv commented at 3:48 pm on April 30, 2021:

You’re right. My apologies for misreading the diff as just removing the run_test() function (which I did try manually). Updated. Thanks for your patience.

in test/functional/feature_presegwit_node_upgrade.py:33 in f43a12789b outdated

28+        node = self.nodes[0]
29+
30+        # Node hasn't been used or connected yet
31+        assert_equal(node.getblockcount(), 0)
32+
33+        self.restart_node(0, extra_args=["-segwitheight=10"])

jnewbery commented at 7:34 am on April 30, 2021:

Why not just start the node with "-segwitheight=10" first time, rather than starting and then restarting?

dhruv commented at 2:16 pm on April 30, 2021:

Nice! Done.

dhruv force-pushed on Apr 30, 2021

dhruv commented at 2:54 pm on April 30, 2021: member

Comments addressed. Ready for further review.

dhruv force-pushed on Apr 30, 2021

jnewbery commented at 8:04 am on May 3, 2021: member

Code review ACK 0dd10ccfd6ef13e5caa06fc380d6eb7f3a8fd22e

Looks great. Thanks @dhruv!

DrahtBot commented at 9:34 am on May 3, 2021: member

🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

in src/chainparams.cpp:497 in a552c1ae36 outdated

489@@ -490,11 +490,8 @@ void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager& args)
490 {
491     if (args.IsArgSet("-segwitheight")) {
492         int64_t height = args.GetArg("-segwitheight", consensus.SegwitHeight);
493-        if (height < -1 || height >= std::numeric_limits<int>::max()) {
494-            throw std::runtime_error(strprintf("Activation height %ld for segwit is out of valid range. Use -1 to disable segwit.", height));
495-        } else if (height == -1) {
496-            LogPrintf("Segwit disabled for testing\n");
497-            height = std::numeric_limits<int>::max();

mzumsande commented at 10:14 pm on June 13, 2021:

With the segwitheight=-1 option removed, I think that also IsScriptWitnessEnabled() in validation.cpp has become obsolete and could be removed as well (the comment there also seems to say that it’s only kept along for p2p_segwit.py).

mzumsande commented at 10:32 pm on June 13, 2021:

There is another check for consensusParams.SegwitHeight != std::numeric_limits<int>::max()) in GenerateCoinbaseCommitment(), also in validation.

dhruv commented at 6:32 pm on June 20, 2021:

Great catches. Thank you, @mzumsande! Both comments addressed.

mzumsande commented at 10:14 pm on June 13, 2021: member

Concept ACK.

dhruv force-pushed on Jun 20, 2021

dhruv commented at 6:33 pm on June 20, 2021: member

Addressed comments from @mzumsande. Ready for further review.

jnewbery commented at 7:48 pm on June 22, 2021: member

utACK c44bac14b4

DrahtBot added the label Needs rebase on Jul 1, 2021

in test/functional/p2p_segwit.py:578 in c24ffdcbaf outdated

558-
559-                # Check that default_witness_commitment is present.
560-                witness_root = CBlock.get_merkle_root([ser_uint256(0),
561-                                                       ser_uint256(txid)])
562-                script = get_witness_script(witness_root, 0)
563-                assert_equal(witness_commitment, script.hex())

MarcoFalke commented at 12:43 pm on July 2, 2021:

why is the test for default_witness_commitment removed?

jnewbery commented at 12:52 pm on July 2, 2021:

Segwit is a buried deployment. There are no miners calling getblocktemplate before the segwit activation is locked in.

MarcoFalke commented at 1:01 pm on July 2, 2021:

The commitment is produced regardless of whether segwit locked in or not.

jnewbery commented at 9:07 am on July 5, 2021:

Oh, I see now that default_witness_commitment is not tested anywhere else. Perhaps it should be tested in mining_basic.py?

dhruv commented at 5:18 am on July 8, 2021:

Thank you for the catch, @MarcoFalke ! Test is now moved into mining_basic.py

jnewbery commented at 7:40 am on July 6, 2021: member

@dhruv - do you plan to rebase this?

dhruv commented at 1:34 pm on July 6, 2021: member

@jnewbery I do. Been on break for a long weekend where I live.

theStack commented at 0:04 am on July 7, 2021: member

Concept ACK

[test] remove or move tests using `-segwitheight=-1` eba5b1cd64

[p2p] remove unused segwitheight=-1 option

This also lets us default to NODE_WITNESS in nLocalServices

6f8b198b82

[p2p] remove redundant NODE_WITNESS checks

nLocalServices defaults to NODE_WITNESS and these checks are obsolete

ac82b99db7

[validation] Set witness script flag with p2sh for blocks 189128c220

[validation] Always include merkle root in coinbase commitment a806647d26

dhruv force-pushed on Jul 8, 2021

dhruv commented at 5:18 am on July 8, 2021: member

Rebased. Addressed comments. Ready for further review.

DrahtBot removed the label Needs rebase on Jul 8, 2021

in test/functional/mining_basic.py:55 in a806647d26

50@@ -49,6 +51,9 @@ def set_test_params(self):
51         self.setup_clean_chain = True
52         self.supports_cli = False
53 
54+    def skip_test_if_missing_module(self):
55+        self.skip_if_no_wallet()

jnewbery commented at 8:22 am on July 8, 2021:

This is ok for now, but it’d be nice to rework this test so that it doesn’t require a wallet.

We could also update the test to verify that the witness commitment commits to the wtxids of the transactions in the block rather than the txids. Currently the block being tested contains only the coinbase transaction and one transaction that spends from a P2PKH output, so the txid is the same as wtxid. It could be updated to include transactions that spend segwit outputs so that the txid and wtxid are different.

dhruv commented at 9:15 pm on July 8, 2021:

I can do those in a follow-up PR. Can you tell me more about how I could remove the wallet dependency? My understanding is that to have getblocktemplate include a transaction that the witness root commits to, I need a valid transaction in the mempool, and to create that valid tx, I need a wallet. I feel like I am missing something about the test framework perhaps?

theStack commented at 0:17 am on July 9, 2021:

@dhruv: Luckily we have a minimalistic wallet implementation for the functional test framework (first introduced by MarcoFalke last year, see #19800). Look for modules importing MiniWallet for example usage, see also issue #20078. Also, feel free to tag me in your follow-up PR, will be happy to review.

MarcoFalke commented at 7:24 pm on July 22, 2021:

Agree

jnewbery commented at 8:53 am on July 8, 2021: member

utACK a806647d260132a00cd633160040625c7dd17803

Thanks for rebasing!

theStack approved

theStack commented at 11:57 pm on July 8, 2021: member

ACK a806647d260132a00cd633160040625c7dd17803

Thanks to the PR review club log the changes were straight-forward to review. Built and ran tests locally, also checked via git grep DeploymentEnabled.*SEGWIT and git grep GetLocalServices.*WITNESS that all occurences of now-superfluous is-segwit-enabled checks were tackled.

mzumsande commented at 8:41 pm on July 17, 2021: member

Code-Review ACK a806647d260132a00cd633160040625c7dd17803

laanwj commented at 3:36 pm on July 22, 2021: member

Code review ACK a806647d260132a00cd633160040625c7dd17803, nice cleanup

laanwj merged this on Jul 22, 2021

laanwj closed this on Jul 22, 2021

in test/functional/mining_basic.py:109 in eba5b1cd64 outdated

105+
106+        # Check that default_witness_commitment is correct.
107+        witness_root = CBlock.get_merkle_root([ser_uint256(0),
108+                                               ser_uint256(txid)])
109+        script = get_witness_script(witness_root, 0)
110+        assert_equal(witness_commitment, script.hex())

MarcoFalke commented at 7:24 pm on July 22, 2021:

Python will throw key-error, so you don’t need the “# Check that default_witness_commitment is present.” section.

See:

0>>> {'b':1}['a']
1Traceback (most recent call last):
2  File "<stdin>", line 1, in <module>
3KeyError: 'a'

Suggestion:

0         assert_equal(tmpl['default_witness_commitment'], script.hex())

sidhujag referenced this in commit 9aad2a398f on Jul 23, 2021

DrahtBot locked this on Aug 18, 2022

Default to NODE_WITNESS in nLocalServices #21090

Conflicts