Skip to content

Commit

Permalink
[p2p] Default to NODE_WITNESS in nLocalServices
Browse files Browse the repository at this point in the history 
- removes tests that use a node with `-segwitheight=-1`
- which then lets us set NODE_WITNESS by default in nLocalServices
- which in turn means we don't need to check if the node is witness
  enabled in net_processing.cpp
  • Loading branch information
@dhruv
dhruv committed Jan 26, 2021
1 parent 703ad74 commit 02934c0
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 83 deletions.
5 changes: 1 addition & 4 deletions src/chainparams.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -461,11 +461,8 @@ void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager& args)
{
if (args.IsArgSet("-segwitheight")) {
int64_t height = args.GetArg("-segwitheight", consensus.SegwitHeight);
if (height < -1 || height >= std::numeric_limits<int>::max()) {
if (height < 0 || height >= std::numeric_limits<int>::max()) {
throw std::runtime_error(strprintf("Activation height %ld for segwit is out of valid range. Use -1 to disable segwit.", height));
} else if (height == -1) {
LogPrintf("Segwit disabled for testing\n");
height = std::numeric_limits<int>::max();
}
consensus.SegwitHeight = static_cast<int>(height);
}
Expand Down
2 changes: 1 addition & 1 deletion src/chainparamsbase.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ void SetupChainParamsBaseOptions(ArgsManager& argsman)
argsman.AddArg("-chain=<chain>", "Use the chain <chain> (default: main). Allowed values: main, test, signet, regtest", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
argsman.AddArg("-regtest", "Enter regression test mode, which uses a special chain in which blocks can be solved instantly. "
"This is intended for regression testing tools and app development. Equivalent to -chain=regtest.", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
argsman.AddArg("-segwitheight=<n>", "Set the activation height of segwit. -1 to disable. (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-segwitheight=<n>", "Set the activation height of segwit. (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-testnet", "Use the test chain. Equivalent to -chain=test.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
argsman.AddArg("-vbparams=deployment:start:end", "Use given start/end times for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
argsman.AddArg("-signet", "Use the signet chain. Equivalent to -chain=signet. Note that the network is defined by the -signetchallenge parameter", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
Expand Down
6 changes: 1 addition & 5 deletions src/init.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1818,11 +1818,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
}
}

if (chainparams.GetConsensus().SegwitHeight != std::numeric_limits<int>::max()) {
// Advertise witness capabilities.
// The option to not set NODE_WITNESS is only used in the tests and should be removed.
nLocalServices = ServiceFlags(nLocalServices | NODE_WITNESS);
}
nLocalServices = ServiceFlags(nLocalServices | NODE_WITNESS);

// ********************************************************* Step 11: import blocks

Expand Down
14 changes: 5 additions & 9 deletions src/net_processing.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -729,7 +729,7 @@ static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman& connma
}
connman.ForNode(nodeid, [&connman](CNode* pfrom) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
AssertLockHeld(::cs_main);
uint64_t nCMPCTBLOCKVersion = (pfrom->GetLocalServices() & NODE_WITNESS) ? 2 : 1;
uint64_t nCMPCTBLOCKVersion = 2;
if (lNodesAnnouncingHeaderAndIDs.size() >= 3) {
// As per BIP152, we only get 3 of our peers to announce
// blocks using compact encodings.
Expand Down Expand Up @@ -1948,7 +1948,7 @@ void static ProcessGetData(CNode& pfrom, Peer& peer, const CChainParams& chainpa

static uint32_t GetFetchFlags(const CNode& pfrom) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
uint32_t nFetchFlags = 0;
if ((pfrom.GetLocalServices() & NODE_WITNESS) && State(pfrom.GetId())->fHaveWitness) {
if (State(pfrom.GetId())->fHaveWitness) {
nFetchFlags |= MSG_WITNESS_FLAG;
}
return nFetchFlags;
Expand Down Expand Up @@ -2698,8 +2698,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
// they may wish to request compact blocks from us
bool fAnnounceUsingCMPCTBLOCK = false;
uint64_t nCMPCTBLOCKVersion = 2;
if (pfrom.GetLocalServices() & NODE_WITNESS)
m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion));
m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion));
nCMPCTBLOCKVersion = 1;
m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion));
}
Expand All @@ -2717,7 +2716,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
bool fAnnounceUsingCMPCTBLOCK = false;
uint64_t nCMPCTBLOCKVersion = 0;
vRecv >> fAnnounceUsingCMPCTBLOCK >> nCMPCTBLOCKVersion;
if (nCMPCTBLOCKVersion == 1 || ((pfrom.GetLocalServices() & NODE_WITNESS) && nCMPCTBLOCKVersion == 2)) {
if (nCMPCTBLOCKVersion == 1 || nCMPCTBLOCKVersion == 2) {
LOCK(cs_main);
// fProvidesHeaderAndIDs is used to "lock in" version of compact blocks we send (fWantsCmpctWitness)
if (!State(pfrom.GetId())->fProvidesHeaderAndIDs) {
Expand All @@ -2731,10 +2730,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
pfrom.m_bip152_highbandwidth_from = fAnnounceUsingCMPCTBLOCK;
}
if (!State(pfrom.GetId())->fSupportsDesiredCmpctVersion) {
if (pfrom.GetLocalServices() & NODE_WITNESS)
State(pfrom.GetId())->fSupportsDesiredCmpctVersion = (nCMPCTBLOCKVersion == 2);
else
State(pfrom.GetId())->fSupportsDesiredCmpctVersion = (nCMPCTBLOCKVersion == 1);
State(pfrom.GetId())->fSupportsDesiredCmpctVersion = (nCMPCTBLOCKVersion == 2);
}
}
return;
Expand Down
93 changes: 29 additions & 64 deletions test/functional/p2p_segwit.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,10 @@
import struct
import time

from test_framework.blocktools import create_block, create_coinbase, add_witness_commitment, get_witness_script, WITNESS_COMMITMENT_HEADER
from test_framework.blocktools import create_block, create_coinbase, add_witness_commitment, WITNESS_COMMITMENT_HEADER
from test_framework.key import ECKey
from test_framework.messages import (
BIP125_SEQUENCE_NUMBER,
CBlock,
CBlockHeader,
CInv,
COutPoint,
Expand Down Expand Up @@ -217,7 +216,7 @@ def set_test_params(self):
self.extra_args = [
["-acceptnonstdtxn=1", "-segwitheight={}".format(SEGWIT_HEIGHT), "-whitelist=noban@127.0.0.1"],
["-acceptnonstdtxn=0", "-segwitheight={}".format(SEGWIT_HEIGHT)],
["-acceptnonstdtxn=1", "-segwitheight=-1"],
["-acceptnonstdtxn=1"],
]
self.supports_cli = False

Expand All @@ -227,8 +226,7 @@ def skip_test_if_missing_module(self):
def setup_network(self):
self.setup_nodes()
self.connect_nodes(0, 1)
self.connect_nodes(0, 2)
self.sync_all()
self.sync_all([self.nodes[0], self.nodes[1]])

# Helper functions

Expand Down Expand Up @@ -270,7 +268,6 @@ def run_test(self):
self.test_non_witness_transaction()
self.test_v0_outputs_arent_spendable()
self.test_block_relay()
self.test_getblocktemplate_before_lockin()
self.test_unnecessary_witness_before_segwit_activation()
self.test_witness_tx_relay_before_segwit_activation()
self.test_standardness_v0()
Expand Down Expand Up @@ -314,7 +311,7 @@ def func_wrapper(self, *args, **kwargs):
func(self, *args, **kwargs)
# Each subtest should leave some utxos for the next subtest
assert self.utxo
self.sync_blocks()
self.sync_blocks([self.nodes[0], self.nodes[1]])
# Assert segwit status is as expected at end of subtest
assert_equal(softfork_active(self.nodes[0], 'segwit'), self.segwit_active)

Expand Down Expand Up @@ -488,11 +485,6 @@ def test_v0_outputs_arent_spendable(self):
witness, and so can't be spent before segwit activation (the point at which
blocks are permitted to contain witnesses)."""

# node2 doesn't need to be connected for this test.
# (If it's connected, node0 may propagate an invalid block to it over
# compact blocks and the nodes would have inconsistent tips.)
self.disconnect_nodes(0, 2)

# Create two outputs, a p2wsh and p2sh-p2wsh
witness_program = CScript([OP_TRUE])
witness_hash = sha256(witness_program)
Expand Down Expand Up @@ -553,37 +545,9 @@ def test_v0_outputs_arent_spendable(self):
# TODO: support multiple acceptable reject reasons.
test_witness_block(self.nodes[0], self.test_node, block, accepted=False, with_witness=False)

self.connect_nodes(0, 2)

self.utxo.pop(0)
self.utxo.append(UTXO(txid, 2, value))

@subtest # type: ignore
def test_getblocktemplate_before_lockin(self):
txid = int(self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1), 16)

for node in [self.nodes[0], self.nodes[2]]:
gbt_results = node.getblocktemplate({"rules": ["segwit"]})
if node == self.nodes[2]:
# If this is a non-segwit node, we should not get a witness
# commitment.
assert 'default_witness_commitment' not in gbt_results
else:
# For segwit-aware nodes, check the witness
# commitment is correct.
assert 'default_witness_commitment' in gbt_results
witness_commitment = gbt_results['default_witness_commitment']

# Check that default_witness_commitment is present.
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())

# Clear out the mempool
self.nodes[0].generate(1)
self.sync_blocks()

@subtest # type: ignore
def test_witness_tx_relay_before_segwit_activation(self):

Expand Down Expand Up @@ -649,7 +613,7 @@ def test_standardness_v0(self):
# Mine it on test_node to create the confirmed output.
test_transaction_acceptance(self.nodes[0], self.test_node, p2sh_tx, with_witness=True, accepted=True)
self.nodes[0].generate(1)
self.sync_blocks()
self.sync_blocks([self.nodes[0], self.nodes[1]])

# Now test standardness of v0 P2WSH outputs.
# Start by creating a transaction with two outputs.
Expand Down Expand Up @@ -721,7 +685,7 @@ def test_standardness_v0(self):
test_transaction_acceptance(self.nodes[0], self.test_node, tx3, with_witness=True, accepted=True)

self.nodes[0].generate(1)
self.sync_blocks()
self.sync_blocks([self.nodes[0], self.nodes[1]])
self.utxo.pop(0)
self.utxo.append(UTXO(tx3.sha256, 0, tx3.vout[0].nValue))
assert_equal(len(self.nodes[1].getrawmempool()), 0)
Expand Down Expand Up @@ -760,7 +724,7 @@ def test_p2sh_witness(self):
block = self.build_next_block()
self.update_witness_block_with_transactions(block, [tx])
test_witness_block(self.nodes[0], self.test_node, block, accepted=True, with_witness=True)
self.sync_blocks()
self.sync_blocks([self.nodes[0], self.nodes[1]])

# Now test attempts to spend the output.
spend_tx = CTransaction()
Expand Down Expand Up @@ -1408,7 +1372,7 @@ def test_segwit_versions(self):
for i in range(NUM_SEGWIT_VERSIONS):
self.utxo.append(UTXO(tx.sha256, i, split_value))

self.sync_blocks()
self.sync_blocks([self.nodes[0], self.nodes[1]])
temp_utxo = []
tx = CTransaction()
witness_program = CScript([OP_TRUE])
Expand All @@ -1430,7 +1394,7 @@ def test_segwit_versions(self):
temp_utxo.append(UTXO(tx.sha256, 0, tx.vout[0].nValue))

self.nodes[0].generate(1) # Mine all the transactions
self.sync_blocks()
self.sync_blocks([self.nodes[0], self.nodes[1]])
assert len(self.nodes[0].getrawmempool()) == 0

# Finally, verify that version 0 -> version 2 transactions
Expand Down Expand Up @@ -1474,7 +1438,7 @@ def test_segwit_versions(self):
block = self.build_next_block()
self.update_witness_block_with_transactions(block, [tx2, tx3])
test_witness_block(self.nodes[0], self.test_node, block, accepted=True)
self.sync_blocks()
self.sync_blocks([self.nodes[0], self.nodes[1]])

# Add utxo to our list
self.utxo.append(UTXO(tx3.sha256, 0, tx3.vout[0].nValue))
Expand Down Expand Up @@ -1502,7 +1466,7 @@ def test_premature_coinbase_witness_spend(self):

# Now test a premature spend.
self.nodes[0].generate(98)
self.sync_blocks()
self.sync_blocks([self.nodes[0], self.nodes[1]])
block2 = self.build_next_block()
self.update_witness_block_with_transactions(block2, [spend_tx])
test_witness_block(self.nodes[0], self.test_node, block2, accepted=False)
Expand All @@ -1512,7 +1476,7 @@ def test_premature_coinbase_witness_spend(self):
block2 = self.build_next_block()
self.update_witness_block_with_transactions(block2, [spend_tx])
test_witness_block(self.nodes[0], self.test_node, block2, accepted=True)
self.sync_blocks()
self.sync_blocks([self.nodes[0], self.nodes[1]])

@subtest # type: ignore
def test_uncompressed_pubkey(self):
Expand Down Expand Up @@ -1641,7 +1605,7 @@ def test_signature_version_1(self):
block = self.build_next_block()
self.update_witness_block_with_transactions(block, [tx])
test_witness_block(self.nodes[0], self.test_node, block, accepted=True)
self.sync_blocks()
self.sync_blocks([self.nodes[0], self.nodes[1]])
self.utxo.pop(0)

# Test each hashtype
Expand Down Expand Up @@ -1820,7 +1784,7 @@ def test_non_standard_witness_blinding(self):
tx.rehash()
test_transaction_acceptance(self.nodes[0], self.test_node, tx, False, True)
self.nodes[0].generate(1)
self.sync_blocks()
self.sync_blocks([self.nodes[0], self.nodes[1]])

# We'll add an unnecessary witness to this transaction that would cause
# it to be non-standard, to test that violating policy with a witness
Expand Down Expand Up @@ -1849,7 +1813,7 @@ def test_non_standard_witness_blinding(self):
test_transaction_acceptance(self.nodes[0], self.test_node, tx3, False, True)

self.nodes[0].generate(1)
self.sync_blocks()
self.sync_blocks([self.nodes[0], self.nodes[1]])

# Update our utxo list; we spent the first entry.
self.utxo.pop(0)
Expand Down Expand Up @@ -1885,7 +1849,7 @@ def test_non_standard_witness(self):
test_transaction_acceptance(self.nodes[0], self.test_node, tx, with_witness=False, accepted=True)

self.nodes[0].generate(1)
self.sync_blocks()
self.sync_blocks([self.nodes[0], self.nodes[1]])

# Creating transactions for tests
p2wsh_txs = []
Expand Down Expand Up @@ -1949,29 +1913,30 @@ def test_non_standard_witness(self):

self.nodes[0].generate(1) # Mine and clean up the mempool of non-standard node
# Valid but non-standard transactions in a block should be accepted by standard node
self.sync_blocks()
self.sync_blocks([self.nodes[0], self.nodes[1]])
assert_equal(len(self.nodes[0].getrawmempool()), 0)
assert_equal(len(self.nodes[1].getrawmempool()), 0)

self.utxo.pop(0)

@subtest # type: ignore
def test_upgrade_after_activation(self):
"""Test the behavior of starting up a segwit-aware node after the softfork has activated."""
"""A pre-segwit node with insufficiently validated blocks needs IBD to enforce segwit rules"""

# All nodes are caught up and node 2 is a pre-segwit node that will soon upgrade.
assert_equal(self.nodes[0].getblockcount(), self.nodes[2].getblockcount())
assert_equal(self.nodes[1].getblockcount(), self.nodes[2].getblockcount())
assert SEGWIT_HEIGHT < self.nodes[2].getblockcount()
assert softfork_active(self.nodes[0], 'segwit')
assert softfork_active(self.nodes[1], 'segwit')
assert 'segwit' not in self.nodes[2].getblockchaininfo()['softforks']
# Node 2 hasn't been used or connected yet
assert_equal(self.nodes[2].getblockcount(), 0)
self.stop_node(2)
self.start_node(2, extra_args=["-segwitheight=10"])

# Generate 8 blocks without witness data
self.nodes[2].generate(8)
assert_equal(self.nodes[2].getblockcount(), 8)

# Restarting node 2 should result in a shutdown because the blockchain consists of
# insufficiently validated blocks per segwit consensus rules.
self.stop_node(2)
# Restarting node 2 (with segwit activation height set to 5) should result in a shutdown
# because the blockchain consists of 3 insufficiently validated blocks per segwit consensus rules.
with self.nodes[2].assert_debug_log(expected_msgs=["Segwit blocks are insufficiently validated. Please delete blocks dir and chain state dir and restart."], timeout=10):
self.nodes[2].start(["-segwitheight={}".format(SEGWIT_HEIGHT)])
self.nodes[2].start(["-segwitheight=5"])

# As directed, the user deletes the blocks and chainstate directories and restarts the node
datadir = get_datadir_path(self.options.tmpdir, 2)
Expand Down

0 comments on commit 02934c0

Please sign in to comment.