Now that segwit is active for a long time, there is no need to reject transactions with the reason that segwit hasn’t activated.
Strictly speaking, this is a bug fix, because with the release of 0.16, we create segwit transactions in our wallet by default without checking if they are allowed by local policy.
More broadly, this simplifies the code as if “premature witness” was always set to true with the corresponding command line args.
git diff fae0e4f~ fae0e4f --ignore-all-space ./test/
137@@ -138,11 +138,6 @@ def run_test(self):
138 # unsigned with redeem script
139 self.fail_accept(self.nodes[0], "mandatory-script-verify-flag", p2sh_ids[NODE_0][WIT_V0][0], False, witness_script(False, self.pubkey[0]))
140 self.fail_accept(self.nodes[0], "mandatory-script-verify-flag", p2sh_ids[NODE_0][WIT_V1][0], False, witness_script(True, self.pubkey[0]))
141- # signed
self.log.info("Verify default node can't accept any witness format txs before fork") downwards). The previuos self.fail_accept() calls are testing transactions that are invalid both before and after the fork due to missing signatures.
1153 tx.rehash()
1154
1155- test_transaction_acceptance(self.nodes[1].rpc, self.std_node, tx, with_witness=True, accepted=segwit_activated)
1156+ # This is always accepted, since the mempool policy is to consider segwit as always active
1157+ # and thus allow segwit outputs
1158+ test_transaction_acceptance(self.nodes[1].rpc, self.std_node, tx, with_witness=True, accepted=True)
V0 segwit outputs should be standard after activation, but not before.)
257 tx3.rehash()
258- # Note that this should be rejected for the premature witness reason,
259- # rather than a policy check, since segwit hasn't activated yet.
260- test_transaction_acceptance(self.nodes[1].rpc, self.std_node, tx3, True, False, b'no-witness-yet')
261+ # This will be rejected due to a policy check:
262+ # No witness is allowed, since it is not a witness program but a p2sh program
We'll add an unnecessary witness to this transaction that would cause down). bad-witness-nonstandard is tested further down.
bad-witness-nonstandard is a general reject reason and as far as I can see none of the tests below check for unnecessary witness. So I guess it is fine to keep this as is.
test_unnecessary_witness_before_segwit_activation() subtest should be removed, since this PR updates the logic so there’s no difference in behaviour before and after activation. Leaving this test as it is is misleading, since it implies that there’s some difference in logic before and after activation.
test_non_standard_witness(), not in test_unnecessary_witness_before_segwit_activation()
I think this is a good idea, but I struggle with one issue. I think it’s reasonably likely that I’m being overly pedantic, but here goes anyway:
I believe our code should be robust and internally consistent. The existing code is very robust – if we somehow end up on a branch where BIP 141 is not active, then we’ll stop allowing new segwit transactions to our mempool. After #11739, this is required, as it is no longer possible with this software to spend v0 witness outputs unless BIP 141 is active (because the witness is required, yet blocks cannot contain witness-bearing transactions unless BIP 141 is active).
Consequently this change would introduce potentially (admittedly unlikely) internally-inconsistent behavior depending on the state of the chain. This is the case even if all the nodes on the network were running the same software (contrast this with, say, BIP90-style consensus changes, where the internal consistency is unaffected by a large reorg, although external consistency – consensus with nodes running other software – could be a potential issue). I don’t know if this is overly pedantic, but it vaguely troubles me to allow for an internally-inconsistent edge case, even if it’s very unlikely.
(I was trying to come up with other examples of times we’ve done something like this and determined that this was okay, but I couldn’t come up with anything.)
All that said, I think this is a useful simplification that I’d like to make. One idea I thought of was something like #12360 (which would lock in segwit activation at a block height), which makes it even less likely that you could end up on a branch on which the software would not think that BIP 141 was active. But even that isn’t bulletproof as in theory we could end up on a shorter, more-work chain below that activation height.
So I’m not sure what the right course of action here is; if others aren’t concerned about this issue then maybe we can just take it as-is, or maybe someone has another idea to ensure that our code is always internally-consistent?
I think we are slightly more internally consistent with this change, considering that the wallet already treats segwit as always active and feeds the mempool with segwit transactions (that are potentially rejected, based on the versionbits-state of the node’s tip).
Note that our miner would not include segwit transactions until the segwit commitment is allowed in the coinbase. So I think we are internally consistent with this change.
I’d assume the only way this could break is when a miner picks transactions directly from the mempool and includes a transaction with witness in a block on a chain where segwit is not active. Although, to me it appears that this change isn’t making that possible or particularly worse, given that a miner could always manually pick random transactions that are invalid in blocks.
Agreed, I had overlooked the mining logic that remains in place, which would prevent including segwit transactions if somehow BIP 141 were not active on the chain. So indeed I believe that this change would not result in internally inconsistent behavior, merely suboptimal behavior, which I think is totally fine in such a case.
I do think that it would be somewhat better to also move forward with #12360 (which would cement the idea that the node will always eventually find segwit transactions to be valid, at some blockheight – so that we aren’t filling our mempool up with transactions that we might never think could be mined) but I don’t think that discussion needs to hold this up at all.
Concept ACK.
test/lint (no code changes)
