policy: Treat segwit as always active #13120

maflcko · 2018-04-30T01:15:19Z

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.

practicalswift · 2018-04-30T05:51:16Z

Concept ACK

jimpo · 2018-05-01T22:53:22Z

utACK fae0e4f

meshcollider · 2018-05-02T09:11:33Z

utACK fae0e4f

maflcko · 2018-05-02T18:33:23Z

Note that the test changes can be reviewed with leading white space ignored: git diff fae0e4f~ fae0e4f --ignore-all-space ./test/

jnewbery

Concept ACK. Comments on tests inline.

jnewbery · 2018-05-02T21:25:34Z

test/functional/feature_segwit.py

@@ -138,11 +138,6 @@ def run_test(self):
        # unsigned with redeem script
        self.fail_accept(self.nodes[0], "mandatory-script-verify-flag", p2sh_ids[NODE_0][WIT_V0][0], False, witness_script(False, self.pubkey[0]))
        self.fail_accept(self.nodes[0], "mandatory-script-verify-flag", p2sh_ids[NODE_0][WIT_V1][0], False, witness_script(True, self.pubkey[0]))
-        # signed


I think this whole block can be removed (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.

This pull request changes the (rejection-/acceptance-) policy about segwit transactions that are otherwise valid, not about segwit transactions that are otherwise invalid. Personally I don't see the advantage in removing those tests and I'd suggest to move the suggested changes into a separate pull request, since it seems unrelated to the changes in this pull request.

For the same reason as above, I think that including these tests is misleading for anyone reading the tests, since it implies that behaviour is different before and after segwit activation.

Thanks! Reworded the comment in the last commit

I still think these subtests should be moved to after segwit activation

jnewbery · 2018-05-02T21:28:34Z

test/functional/p2p_segwit.py

-        test_transaction_acceptance(self.nodes[1].rpc, self.std_node, tx, with_witness=True, accepted=segwit_activated)
+        # This is always accepted, since the mempool policy is to consider segwit as always active
+        # and thus allow segwit outputs
+        test_transaction_acceptance(self.nodes[1].rpc, self.std_node, tx, with_witness=True, accepted=True)


Update comment above this function (V0 segwit outputs should be standard after activation, but not before.)

Thanks! Fixed up the comment in a new commit.

jnewbery · 2018-05-02T21:32:00Z

test/functional/p2p_segwit.py

-        # rather than a policy check, since segwit hasn't activated yet.
-        test_transaction_acceptance(self.nodes[1].rpc, self.std_node, tx3, True, False, b'no-witness-yet')
+        # This will be rejected due to a policy check:
+        # No witness is allowed, since it is not a witness program but a p2sh program


Just remove this entire block (everything from We'll add an unnecessary witness to this transaction that would cause down). bad-witness-nonstandard is tested further down.

Please note that 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.

I disagree. The entire 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.

Please note that the policy difference only has effect on transactions (and not blocks). This test is testing that blocks can not include unnecessary witness. It is also testing that unnecessary witness that is added to transactions "in-flight" (e.g. by malicious peers) does not add them to the rejection cache and that they'd be accepted just fine when sent without the witness.

I have amended the misleading comment in my last commit.

This subtest should be in test_non_standard_witness(), not in test_unnecessary_witness_before_segwit_activation()

I have moved it to a separate test case so that the symbols don't collide

jnewbery

I don't think this PR should go in without cleaning up the segwit activation functional tests. Leaving vestigial tests from when behaviour differed depending on whether segwit was active or not is confusing for anyone reading the tests.

jnewbery · 2018-05-07T13:41:09Z

test/functional/p2p_segwit.py

-        # rather than a policy check, since segwit hasn't activated yet.
-        test_transaction_acceptance(self.nodes[1].rpc, self.std_node, tx3, True, False, b'no-witness-yet')
+        # This will be rejected due to a policy check:
+        # No witness is allowed, since it is not a witness program but a p2sh program


I disagree. The entire 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.

jnewbery · 2018-05-07T13:42:43Z

test/functional/feature_segwit.py

@@ -138,11 +138,6 @@ def run_test(self):
        # unsigned with redeem script
        self.fail_accept(self.nodes[0], "mandatory-script-verify-flag", p2sh_ids[NODE_0][WIT_V0][0], False, witness_script(False, self.pubkey[0]))
        self.fail_accept(self.nodes[0], "mandatory-script-verify-flag", p2sh_ids[NODE_0][WIT_V1][0], False, witness_script(True, self.pubkey[0]))
-        # signed


For the same reason as above, I think that including these tests is misleading for anyone reading the tests, since it implies that behaviour is different before and after segwit activation.

jnewbery

This PR makes the tests unnecessarily confusing.

jnewbery · 2018-05-08T19:03:50Z

ACK 340720c5d3bef6b61e6561aa505732ec51e4b565

sdaftuar · 2018-05-09T15:34:09Z

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?

maflcko · 2018-05-09T16:41:47Z

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.

sdaftuar · 2018-05-09T16:52:36Z

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.

maflcko · 2018-05-13T15:19:15Z

Squashed and rebased. (Only merge conflict was in the functional tests)

jnewbery · 2018-05-16T21:23:19Z

re-utACK fa2a73a41e5d196c7c82ed1fd451e989a31aef37

maflcko · 2018-05-29T20:52:14Z

Rebased to accommodate for moved file in test/lint (no code changes)

laanwj · 2018-06-12T15:20:29Z

utACK fa7a6cf

fa7a6cf policy: Treat segwit as always active (MarcoFalke) Pull request description: 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. Tree-SHA512: 484c26aa3a66faba6b41e8554a91a29bfc15fbf6caae3d5363a3966283143189c4bd5333a610b0669c1238f75620691264e73f6b9f1161cdacf7574d946436da

maflcko added the TX fees and policy label Apr 30, 2018

maflcko force-pushed the Mf1805-segwitGenesisPolicy branch from f338ccb to fae0e4f Compare April 30, 2018 01:22

dcousens approved these changes Apr 30, 2018

View reviewed changes

promag mentioned this pull request Apr 30, 2018

Throw an error for unknown args #13112

Merged

laanwj requested a review from sdaftuar May 1, 2018 14:45

jnewbery reviewed May 2, 2018

View reviewed changes

jnewbery reviewed May 7, 2018

View reviewed changes

maflcko force-pushed the Mf1805-segwitGenesisPolicy branch 2 times, most recently from fa004eb to fa650bc Compare May 8, 2018 17:57

jnewbery suggested changes May 8, 2018

View reviewed changes

maflcko force-pushed the Mf1805-segwitGenesisPolicy branch 4 times, most recently from fa82fcd to 340720c Compare May 8, 2018 19:00

sdaftuar mentioned this pull request May 10, 2018

Bury bip9 deployments #12360

Closed

maflcko force-pushed the Mf1805-segwitGenesisPolicy branch 2 times, most recently from fa9e564 to fa2a73a Compare May 13, 2018 15:13

jnewbery approved these changes May 16, 2018

View reviewed changes

policy: Treat segwit as always active

fa7a6cf

maflcko force-pushed the Mf1805-segwitGenesisPolicy branch from fa2a73a to fa7a6cf Compare May 29, 2018 20:50

laanwj merged commit fa7a6cf into bitcoin:master Jun 12, 2018

Empact mentioned this pull request Jun 12, 2018

Return the script type from Solver #13429

Merged

Bushstar mentioned this pull request Jun 13, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#344

Merged

ryanofsky mentioned this pull request Jun 26, 2018

Refactor: separate wallet from node #10973

Merged

maflcko deleted the Mf1805-segwitGenesisPolicy branch July 11, 2018 22:42

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

policy: Treat segwit as always active #13120

policy: Treat segwit as always active #13120

maflcko commented Apr 30, 2018

practicalswift commented Apr 30, 2018

jimpo commented May 1, 2018

meshcollider commented May 2, 2018

maflcko commented May 2, 2018

jnewbery left a comment

jnewbery May 2, 2018

maflcko May 4, 2018

jnewbery May 7, 2018

maflcko May 8, 2018

jnewbery May 8, 2018

jnewbery May 2, 2018

maflcko May 4, 2018

jnewbery May 2, 2018

maflcko May 4, 2018

jnewbery May 7, 2018

maflcko May 8, 2018

maflcko May 8, 2018

jnewbery May 8, 2018

maflcko May 8, 2018

jnewbery left a comment

jnewbery May 7, 2018

jnewbery May 7, 2018

jnewbery left a comment

jnewbery commented May 8, 2018

sdaftuar commented May 9, 2018

maflcko commented May 9, 2018

sdaftuar commented May 9, 2018

maflcko commented May 13, 2018

jnewbery commented May 16, 2018

maflcko commented May 29, 2018

laanwj commented Jun 12, 2018

policy: Treat segwit as always active #13120

policy: Treat segwit as always active #13120

Conversation

maflcko commented Apr 30, 2018

practicalswift commented Apr 30, 2018

jimpo commented May 1, 2018

meshcollider commented May 2, 2018

maflcko commented May 2, 2018

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

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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery left a comment

Choose a reason for hiding this comment

jnewbery commented May 8, 2018

sdaftuar commented May 9, 2018

maflcko commented May 9, 2018

sdaftuar commented May 9, 2018

maflcko commented May 13, 2018

jnewbery commented May 16, 2018

maflcko commented May 29, 2018

laanwj commented Jun 12, 2018