Default to NODE_WITNESS in nLocalServices #21090

dhruv · 2021-02-05T20:30:32Z

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

DrahtBot · 2021-02-06T01:22:16Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: Implicitly sync after generate* to preempt races and intermittent test failures #20362 by MarcoFalke
wallet: ensure wallet files are not reused across chains #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.

d831e71 [validation] RewindBlockIndex no longer needed (Dhruv Mehta) Pull request description: Closes #17862 Context from [original comment](#17862 (comment)) (minor edits): `RewindBlockIndex()` is a mechanism to allow nodes to be upgraded after segwit activation, while still keeping their chainstate/datadir in a consistent state. It works as follows: - A pre-segwit (i.e. v0.13.0 or older) node is running. - Segwit activates. The pre-segwit node remains sync'ed to the tip, but is not enforcing the new segwit rules. - The user upgrades the node to a segwit-aware version (v0.13.1 or newer). - On startup, in `AppInitMain()`, `RewindBlockIndex()` is called. This walks the chain backwards from the tip, disconnecting and erasing blocks that from after segwit activation that weren't validated with segwit rules. - those blocks are then redownloaded (with witness data) and validated with segwit rules. This logic probably isn't required any more since: - Segwit activated at height 481824, when the block chain was 130GB and the total number of txs was 250 million. Today, we're at height 667704, the blockchain is over 315GB and the total number of txs is over 600 million. Even if 20% of that added data is witness data (a high estimate), then around 150GB of transactions would need to be rewound to get back to segwit activation height. It'd probably be faster to simply validate from genesis, especially since we won't be validating any scripts before the assumevalid block. It's also unclear whether rewinding 150GB of transactions would even work. It's certainly never been tested. - Bitcoin Core v0.13 is hardly used any more. https://luke.dashjr.org/programs/bitcoin/files/charts/software.html shows less than 50 nodes running it. The software was EOL on Aug 1st 2018. It's very unlikely that anyone is running 0.13 and will want to upgrade to 0.22. This PR introduces `NeedsRedownload()` which merely checks for insufficiently validated segwit blocks and requests that the user restarts the node with `-reindex`. Reindexing the block files upon restart will make the node rebuild chain state and block index from the `blk*.dat` files on disk. The node won't be able to index the blocks with `BLOCK_OPT_WITNESS`, so they will be missing from the chain and be re-downloaded, with witness data. Removing this code allows the following (done in follow-up #21090): - removal of tests using `segwitheight=-1` in `p2p_segwit.py`. - 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` ACKs for top commit: jnewbery: utACK d831e71 jamesob: ACK d831e71 laanwj: Cursory code review ACK d831e71. Agree with the direction of the change, thanks for simplifying the logic here. glozow: utACK d831e71 Tree-SHA512: 3eddf5121ccd081ad7f15a5c6478ef867083edc8ba0bf1ee759e87bc070ee3d2f0698a3feba8db8dc087987c8452887b6f72cff05b3e178f41cb10a515fb8053

jnewbery · 2021-04-27T09:00:05Z

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.

d831e71 [validation] RewindBlockIndex no longer needed (Dhruv Mehta) Pull request description: Closes bitcoin#17862 Context from [original comment](bitcoin#17862 (comment)) (minor edits): `RewindBlockIndex()` is a mechanism to allow nodes to be upgraded after segwit activation, while still keeping their chainstate/datadir in a consistent state. It works as follows: - A pre-segwit (i.e. v0.13.0 or older) node is running. - Segwit activates. The pre-segwit node remains sync'ed to the tip, but is not enforcing the new segwit rules. - The user upgrades the node to a segwit-aware version (v0.13.1 or newer). - On startup, in `AppInitMain()`, `RewindBlockIndex()` is called. This walks the chain backwards from the tip, disconnecting and erasing blocks that from after segwit activation that weren't validated with segwit rules. - those blocks are then redownloaded (with witness data) and validated with segwit rules. This logic probably isn't required any more since: - Segwit activated at height 481824, when the block chain was 130GB and the total number of txs was 250 million. Today, we're at height 667704, the blockchain is over 315GB and the total number of txs is over 600 million. Even if 20% of that added data is witness data (a high estimate), then around 150GB of transactions would need to be rewound to get back to segwit activation height. It'd probably be faster to simply validate from genesis, especially since we won't be validating any scripts before the assumevalid block. It's also unclear whether rewinding 150GB of transactions would even work. It's certainly never been tested. - Bitcoin Core v0.13 is hardly used any more. https://luke.dashjr.org/programs/bitcoin/files/charts/software.html shows less than 50 nodes running it. The software was EOL on Aug 1st 2018. It's very unlikely that anyone is running 0.13 and will want to upgrade to 0.22. This PR introduces `NeedsRedownload()` which merely checks for insufficiently validated segwit blocks and requests that the user restarts the node with `-reindex`. Reindexing the block files upon restart will make the node rebuild chain state and block index from the `blk*.dat` files on disk. The node won't be able to index the blocks with `BLOCK_OPT_WITNESS`, so they will be missing from the chain and be re-downloaded, with witness data. Removing this code allows the following (done in follow-up bitcoin#21090): - removal of tests using `segwitheight=-1` in `p2p_segwit.py`. - 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` ACKs for top commit: jnewbery: utACK d831e71 jamesob: ACK bitcoin@d831e71 laanwj: Cursory code review ACK d831e71. Agree with the direction of the change, thanks for simplifying the logic here. glozow: utACK d831e71 Tree-SHA512: 3eddf5121ccd081ad7f15a5c6478ef867083edc8ba0bf1ee759e87bc070ee3d2f0698a3feba8db8dc087987c8452887b6f72cff05b3e178f41cb10a515fb8053

dhruv · 2021-04-29T18:16:54Z

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.

src/chainparams.cpp

dhruv

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

Ready for further review with one question below.

src/net_processing.cpp

test/functional/p2p_segwit.py

test/functional/feature_presegwit_node_upgrade.py

dhruv · 2021-04-30T14:54:26Z

Comments addressed. Ready for further review.

DrahtBot · 2021-05-03T09:34:12Z

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

mzumsande

Concept ACK.

src/chainparams.cpp

dhruv · 2021-06-20T18:33:35Z

Addressed comments from @mzumsande. Ready for further review.

jnewbery · 2021-06-22T19:48:43Z

utACK c44bac1

maflcko · 2021-07-02T12:43:08Z

test/functional/p2p_segwit.py

-                witness_root = CBlock.get_merkle_root([ser_uint256(0),
-                                                       ser_uint256(txid)])
-                script = get_witness_script(witness_root, 0)
-                assert_equal(witness_commitment, script.hex())


why is the test for default_witness_commitment removed?

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

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

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

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

jnewbery · 2021-07-06T07:40:11Z

@dhruv - do you plan to rebase this?

dhruv · 2021-07-06T13:34:32Z

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

theStack · 2021-07-07T00:04:54Z

Concept ACK

This also lets us default to NODE_WITNESS in nLocalServices

nLocalServices defaults to NODE_WITNESS and these checks are obsolete

dhruv · 2021-07-08T05:18:32Z

Rebased. Addressed comments. Ready for further review.

jnewbery

utACK a806647

Thanks for rebasing!

jnewbery · 2021-07-08T08:22:44Z

test/functional/mining_basic.py

+    def skip_test_if_missing_module(self):
+        self.skip_if_no_wallet()


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.

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?

@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.

theStack

ACK a806647

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 · 2021-07-17T20:41:51Z

Code-Review ACK a806647

laanwj · 2021-07-22T15:36:07Z

Code review ACK a806647, nice cleanup

maflcko · 2021-07-22T19:24:04Z

test/functional/mining_basic.py

+        witness_root = CBlock.get_merkle_root([ser_uint256(0),
+                                               ser_uint256(txid)])
+        script = get_witness_script(witness_root, 0)
+        assert_equal(witness_commitment, script.hex())


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

See:

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

Suggestion:

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

maflcko · 2021-07-22T19:24:38Z

test/functional/mining_basic.py

+    def skip_test_if_missing_module(self):
+        self.skip_if_no_wallet()


dhruv force-pushed the default-to-node-witness-2021 branch from 0ef4395 to 743286d Compare February 5, 2021 20:33

This was referenced Feb 5, 2021

Remove RewindBlockIndex logic #21009

Merged

net processing: Only support version 2 compact blocks #20799

Merged

DrahtBot added P2P Validation labels Feb 5, 2021

This was referenced Feb 6, 2021

[Bundle 3/n] Prune remaining g_chainman usage in validation functions #21055

Merged

tree-wide: De-globalize ChainstateManager #20158

Closed

Introduce deploymentstatus #19438

Merged

This was referenced Feb 16, 2021

net-processing refactoring -- lose globals, move implementation details from .h to .cpp #20758

Closed

[Bundle 4/n] Prune g_chainman usage in validation-adjacent modules #21270

Merged

jnewbery mentioned this pull request Apr 27, 2021

Remove RewindBlockIndex logic #17862

Closed

maflcko added Needs rebase and removed Validation labels Apr 27, 2021

dhruv mentioned this pull request Apr 29, 2021

test: Fix intermittent issue in p2p_segwit.py #21792

Merged

dhruv force-pushed the default-to-node-witness-2021 branch 2 times, most recently from e889ad4 to 117ae3f Compare April 29, 2021 18:10

dhruv marked this pull request as ready for review April 29, 2021 18:12

maflcko reviewed Apr 29, 2021

View reviewed changes

src/chainparams.cpp Outdated Show resolved Hide resolved

dhruv force-pushed the default-to-node-witness-2021 branch 2 times, most recently from bf325dd to f43a127 Compare April 30, 2021 00:58

dhruv commented Apr 30, 2021

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

DrahtBot removed the Needs rebase label Apr 30, 2021

jnewbery reviewed Apr 30, 2021

View reviewed changes

dhruv force-pushed the default-to-node-witness-2021 branch from f43a127 to 86608da Compare April 30, 2021 14:11

jnewbery mentioned this pull request Jun 7, 2021

qa: Another intermittent failure in p2p_compactblocks.py #21535

Closed

mzumsande reviewed Jun 13, 2021

View reviewed changes

src/chainparams.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Jun 16, 2021

wallet: ensure wallet files are not reused across chains #18554

Merged

dhruv force-pushed the default-to-node-witness-2021 branch from 0dd10cc to c44bac1 Compare June 20, 2021 18:31

This was referenced Jun 24, 2021

test: Implicitly sync after generate* to preempt races and intermittent test failures #20362

Closed

Implement BIP-119 Validation (CheckTemplateVerify) #21702

Closed

DrahtBot added the Needs rebase label Jul 1, 2021

maflcko reviewed Jul 2, 2021

View reviewed changes

dhruv added 5 commits July 7, 2021 22:12

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

eba5b1c

[p2p] remove unused segwitheight=-1 option

6f8b198

This also lets us default to NODE_WITNESS in nLocalServices

[p2p] remove redundant NODE_WITNESS checks

ac82b99

nLocalServices defaults to NODE_WITNESS and these checks are obsolete

[validation] Set witness script flag with p2sh for blocks

189128c

[validation] Always include merkle root in coinbase commitment

a806647

dhruv force-pushed the default-to-node-witness-2021 branch from c44bac1 to a806647 Compare July 8, 2021 05:16

DrahtBot removed the Needs rebase label Jul 8, 2021

jnewbery reviewed Jul 8, 2021

View reviewed changes

theStack approved these changes Jul 8, 2021

View reviewed changes

laanwj merged commit 5d83e7d into bitcoin:master Jul 22, 2021

maflcko reviewed Jul 22, 2021

View reviewed changes

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 23, 2021

Merge bitcoin#21090: Default to NODE_WITNESS in nLocalServices

9aad2a3

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default to NODE_WITNESS in nLocalServices #21090

Default to NODE_WITNESS in nLocalServices #21090

dhruv commented Feb 5, 2021 •

edited

Loading

DrahtBot commented Feb 6, 2021 •

edited

Loading

jnewbery commented Apr 27, 2021

dhruv commented Apr 29, 2021

dhruv left a comment •

edited

Loading

dhruv commented Apr 30, 2021

DrahtBot commented May 3, 2021

mzumsande left a comment

dhruv commented Jun 20, 2021

jnewbery commented Jun 22, 2021

maflcko Jul 2, 2021

jnewbery Jul 2, 2021

maflcko Jul 2, 2021

jnewbery Jul 5, 2021

dhruv Jul 8, 2021

jnewbery commented Jul 6, 2021

dhruv commented Jul 6, 2021

theStack commented Jul 7, 2021

dhruv commented Jul 8, 2021

jnewbery left a comment

jnewbery Jul 8, 2021

dhruv Jul 8, 2021

theStack Jul 9, 2021

maflcko Jul 22, 2021

theStack left a comment

mzumsande commented Jul 17, 2021

laanwj commented Jul 22, 2021

maflcko Jul 22, 2021

maflcko Jul 22, 2021

		def skip_test_if_missing_module(self):
		self.skip_if_no_wallet()

Default to NODE_WITNESS in nLocalServices #21090

Default to NODE_WITNESS in nLocalServices #21090

Conversation

dhruv commented Feb 5, 2021 • edited Loading

DrahtBot commented Feb 6, 2021 • edited Loading

Conflicts

jnewbery commented Apr 27, 2021

dhruv commented Apr 29, 2021

dhruv left a comment • edited Loading

Choose a reason for hiding this comment

dhruv commented Apr 30, 2021

DrahtBot commented May 3, 2021

mzumsande left a comment

Choose a reason for hiding this comment

dhruv commented Jun 20, 2021

jnewbery commented Jun 22, 2021

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

jnewbery commented Jul 6, 2021

dhruv commented Jul 6, 2021

theStack commented Jul 7, 2021

dhruv commented Jul 8, 2021

jnewbery 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

Choose a reason for hiding this comment

theStack left a comment

Choose a reason for hiding this comment

mzumsande commented Jul 17, 2021

laanwj commented Jul 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruv commented Feb 5, 2021 •

edited

Loading

DrahtBot commented Feb 6, 2021 •

edited

Loading

dhruv left a comment •

edited

Loading