policy: Treat segwit as always active #13120

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:Mf1805-segwitGenesisPolicy changing 9 files +107 −118
  1. MarcoFalke commented at 1:15 am on April 30, 2018: member

    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.

  2. MarcoFalke added the label TX fees and policy on Apr 30, 2018
  3. MarcoFalke force-pushed on Apr 30, 2018
  4. dcousens approved
  5. practicalswift commented at 5:51 am on April 30, 2018: contributor
    Concept ACK
  6. laanwj requested review from sdaftuar on May 1, 2018
  7. jimpo commented at 10:53 pm on May 1, 2018: contributor
    utACK fae0e4f
  8. MarcoFalke commented at 6:33 pm on May 2, 2018: member
    Note that the test changes can be reviewed with leading white space ignored: git diff fae0e4f~ fae0e4f --ignore-all-space ./test/
  9. in test/functional/feature_segwit.py:141 in fae0e4f330 outdated
    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
    


    jnewbery commented at 9:25 pm on May 2, 2018:
    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.

    MarcoFalke commented at 5:22 pm on May 4, 2018:
    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.

    jnewbery commented at 1:42 pm on May 7, 2018:
    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.

    MarcoFalke commented at 5:58 pm on May 8, 2018:
    Thanks! Reworded the comment in the last commit

    jnewbery commented at 6:17 pm on May 8, 2018:
    I still think these subtests should be moved to after segwit activation
  10. in test/functional/p2p_segwit.py:1110 in fae0e4f330 outdated
    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)
    


    jnewbery commented at 9:28 pm on May 2, 2018:
    Update comment above this function (V0 segwit outputs should be standard after activation, but not before.)

    MarcoFalke commented at 5:49 pm on May 4, 2018:
    Thanks! Fixed up the comment in a new commit.
  11. in test/functional/p2p_segwit.py:258 in fae0e4f330 outdated
    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
    


    jnewbery commented at 9:32 pm on May 2, 2018:
    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.

    MarcoFalke commented at 5:34 pm on May 4, 2018:
    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.

    jnewbery commented at 1:41 pm on May 7, 2018:
    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.

    MarcoFalke commented at 6:01 pm on May 8, 2018:
    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.

    MarcoFalke commented at 6:02 pm on May 8, 2018:
    I have amended the misleading comment in my last commit.

    jnewbery commented at 6:19 pm on May 8, 2018:
    This subtest should be in test_non_standard_witness(), not in test_unnecessary_witness_before_segwit_activation()

    MarcoFalke commented at 6:44 pm on May 8, 2018:
    I have moved it to a separate test case so that the symbols don’t collide
  12. jnewbery commented at 9:32 pm on May 2, 2018: member
    Concept ACK. Comments on tests inline.
  13. jnewbery commented at 1:43 pm on May 7, 2018: member
    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.
  14. MarcoFalke force-pushed on May 8, 2018
  15. MarcoFalke force-pushed on May 8, 2018
  16. jnewbery changes_requested
  17. jnewbery commented at 6:20 pm on May 8, 2018: member
    This PR makes the tests unnecessarily confusing.
  18. MarcoFalke force-pushed on May 8, 2018
  19. MarcoFalke force-pushed on May 8, 2018
  20. MarcoFalke force-pushed on May 8, 2018
  21. MarcoFalke force-pushed on May 8, 2018
  22. jnewbery commented at 7:03 pm on May 8, 2018: member
    ACK 340720c5d3bef6b61e6561aa505732ec51e4b565
  23. sdaftuar commented at 3:34 pm on May 9, 2018: member

    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?

  24. MarcoFalke commented at 4:41 pm on May 9, 2018: member

    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.

  25. sdaftuar commented at 4:52 pm on May 9, 2018: member

    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.

  26. MarcoFalke force-pushed on May 13, 2018
  27. MarcoFalke force-pushed on May 13, 2018
  28. MarcoFalke commented at 3:19 pm on May 13, 2018: member
    Squashed and rebased. (Only merge conflict was in the functional tests)
  29. jnewbery commented at 9:23 pm on May 16, 2018: member
    re-utACK fa2a73a41e5d196c7c82ed1fd451e989a31aef37
  30. jnewbery approved
  31. policy: Treat segwit as always active fa7a6cf1b3
  32. MarcoFalke force-pushed on May 29, 2018
  33. MarcoFalke commented at 8:52 pm on May 29, 2018: member
    Rebased to accommodate for moved file in test/lint (no code changes)
  34. laanwj commented at 3:20 pm on June 12, 2018: member
    utACK fa7a6cf1b36284db70e941bd2915fd6edbb0f9d6
  35. laanwj merged this on Jun 12, 2018
  36. laanwj closed this on Jun 12, 2018

  37. laanwj referenced this in commit ca2a23387b on Jun 12, 2018
  38. MarcoFalke deleted the branch on Jul 11, 2018
  39. MarcoFalke locked this on Sep 8, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 13:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me