Segregated witness #7910

pull sipa wants to merge 128 commits into bitcoin:master from sipa:segwit-master changing 80 files +5305 −571
  1. sipa commented at 3:22 pm on April 19, 2016: member

    This is the complete code for segregated witness on top of master (implementing BIP141, BIP143, BIP144, BIP145). Apart from commentary and merging (see further), the code is never rewritten/squashed/rebased, in order not to invalidate review. A rebased/squashed version is available here: #8149.

    See also the detailed and up-to-date list of commits here: #7910 (comment)

    There are certainly some items left to do:

    • Incorporate changes to BIP145/BIP9 when finalized (see https://github.com/bitcoin/bips/pull/365)
    • Define a deployment time for testnet
    • Seed nodes that provide segwit-capable peers
    • Do tests in a mixed network of upgraded and non-upgraded nodes
    • Remove segnet from mainline merge
    • Define a deployment time for mainnet
    • Per-transaction caching of sighashes to fix the O(n^2) sighash problem (see https://github.com/sipa/bitcoin/pull/70)

    Many thanks to all contributors so far (including @jl2012, @NicolasDorier, @CodeShark, @sdaftuar, @morcos, @luke-jr, @afk11, @theuni, @TheBlueMatt, @MarcoFalke, @LongShao007, @jonasnick, @mruddy), the ideas it is based on (by @gmaxwell and @luke-jr), and many reviewers (including @SergioDemianLerner, @instagibbs, @dcousens, and @btcdrak).

  2. Keep addrman's nService bits consistent with outbound observations 644b2c5090
  3. Verify that outbound connections have expected services afdc413823
  4. Only store and connect to NODE_NETWORK nodes fdec2bce16
  5. --- [SEGWIT] begin: segnet --- e69265d9a4
  6. Don't check the genesis block PoW 
    Inspired by Jorge Timon's "Testchain: Don't check the genesis block", but
restricted to just PoW check.

Code by Matt Corallo, originally submitted as a patch to segwit.
    66dbd818f9
  7. sipa commented at 4:09 pm on April 19, 2016: member

    Since github shows the commits sorted by author date rather than dependency order, here is a list of all commits:

    • preparation (https://github.com/sipa/bitcoin/compare/fb0ac482eee761ec17ed2c11df11e054347a026d...e69265d)
      • 644b2c5 Keep addrman’s nService bits consistent with outbound observations
      • afdc413 Verify that outbound connections have expected services
      • fdec2bc Only store and connect to NODE_NETWORK nodes
    • segnet (https://github.com/sipa/bitcoin/compare/e69265d...bb4bb47)
      • 66dbd81 Don’t check the genesis block PoW
      • 70ebe86 Create segnet4
      • f8bcd86 Add segnet seed nodes
      • 58847fe qt: Work (don’t crash) with -segnet
    • P2P/node/consensus (https://github.com/sipa/bitcoin/compare/bb4bb47...351a2fe)
      • 3b1ff49 Add segregated witness transaction serialization
      • e35d020 Removed ppszTypeName from protocol.cpp
      • adb1c09 getdata enum issue fix
      • ac6886d Introduce and preferentially peer with NODE_WITNESS service bit
      • cb9d4d3 Witness commitment validation
      • 2c87be1 Script validation logic for witnesses
      • 1e47805 Enable SCRIPT_VERIFY_WITNESS for mempool transactions
      • 97f34f7 Activate script consensus rules through BIP9
      • 6a8021f Only download blocks from witness peers after fork
      • 271e45e Observe input amounts: verification
      • d374139 Add signature version 1 with updated sighash
      • 92c6dc3 Return witness data
      • 3955218 Implement block size/sigop cost rules, limits, and GBT support
      • 5a47b98 Add command line options to loosen mempool acceptance rules
      • e573571 bitcoinconsensus: add method that accepts amount, and return error when verify_script receives VERIFY_WITNESS flag
      • a6386c2 Increase MAX_PROTOCOL_MESSAGE_LENGTH
      • 1b6c6f1 Add rewind logic to deal with post-fork software updates
    • wallet (https://github.com/sipa/bitcoin/compare/351a2fe...a2d6f8a)
      • abbe085 Witness script signing
      • c3fe53f Add witness address RPCs (using P2SH)
      • ec385fa signrawtransaction can sign P2WSH
    • tests (https://github.com/sipa/bitcoin/compare/a2d6f8a...cfb0a83)
      • 2ed1d11 Signing tests
      • 3c0b16f Add rpc test for segwit
      • 24746d2 Add transaction tests for segwit
      • 3409ad6 Add segwit support to script_tests
      • e5ffcc1 Autogeneration support for witness in script_tests
      • 5e6abe5 Update p2p test framework with segwit support
      • 7b539f9 P2P test for segwit
    • fixups (https://github.com/sipa/bitcoin/compare/cfb0a83...77c613c)
      • bb2bfca fixup Implement block size/sigop cost rules, limits, and GBT support: use int64_t for sigopcost everywhere
      • 64c1527 fixup Implement block size/sigop cost rules, limits, and GBT support: update block cost comment
      • cc741f5 fixup Add segregated witness transaction serialization: comments
      • c28179f fixup Update p2p test framework with segwit support: fix comment
      • 6a5da9b fixup Witness commitment validation: list the mentioned BIPs
      • 88e0135 fixup Add command line options to loosen mempool acceptance rules: simplify comment
      • da60fae fixup Witness commitment validation: factor out GetWitnessCommitmentPos
      • 4f827be fixup Witness commitment validation: fix typo in comment
      • 058f495 fixup Implement block size/sigop cost rules, limits, and GBT support: improve GBT help
      • c1e7a95 fixup Add witness address RPCs (using P2SH): test hex & wallet available
      • dbe6391 fixup Add segregated witness transaction serialization: revert extformat-if-empty-vin
      • 97d7402 tidy up CInv::GetCommand
      • f0f6123 fixup Witness commitment validation: function UpdateUncommitedBlockStructures redefine
      • a613599 [qa] Use integer division, byte strings properly
      • 5d6b6e2 fixup Add segregated witness transaction serialization: add missing witness flags
    • fixups 2 (https://github.com/sipa/bitcoin/compare/77c613c...f38671f)
      • f889bec fixup Add segregated witness transaction serialization: add negative flag
      • 3ddabe4 fixup Add segregated witness transaction serialization: test with no inputs no longer possible
      • c744a1e fixup Add segregated witness transaction serialization: deal with overwrite and inconsistencies
      • 4709595 fixup Witness commitment validation: revert adding of commitment in IncrementExtraNonce
      • 122ca81 fixup Witness commitment validation: correct comment
      • 5df3e51 fixup Witness commitment validation: add comment about witness checking in miner
      • 36e1656 fixup Witness commitment validation: better validation error message
      • f726402 fixup Implement block size/sigop cost rules, limits, and GBT support: update comment
      • cf2c531 fixup Enable SCRIPT_VERIFY_WITNESS for mempool transactions: do not reject/punish invalid witness orphans
      • 0fb6e4e fixup Return witness data: don’t list nextblockhash twice
      • ccd0e6c [qa] mininode: Use hexlify wrapper from util
      • 3a62cb9 Implement RecursiveDynamicUsage for witness structures
      • a541f0b Use an enum for signature versions
    • fixups 3 (https://github.com/sipa/bitcoin/compare/f38671f...8708de8)
      • ba7e292 test: WITNESS flag must be used with P2SH flag
      • e7821e9 BIP9 parameters for testnet
      • 76142af Segwit script error unit tests
    • fixups 4 (https://github.com/sipa/bitcoin/compare/8708de8...306858f)
      • b846298 f cb9d4d34: typo fix
      • 38e3fcd f 3b1ff49f: remove redundant witness clean
      • fd85b77 f 1b6c6f16: Cleanup mapBlocksUnlinked in RewindBlockIndex
      • 433daf4 f ac6886d3: Get rid of leftover WITNESS_VERSION
      • 9590289 f 2c87be12: fix typo
      • 17277c9 f 39552185: GetBlockCost: Clarify comment description
      • 1e9cba2 f 39552185: provide both -maxblocksize and -maxblockcost
      • 8f50b96 [qa] segwit: Switch to py3
      • 75335ca f 1b6c6f16: pass CChainParams rather than Consensus::Params to RewindBlockIndex
    • fixups 5 (https://github.com/sipa/bitcoin/compare/306858f...869f26e)
      • 036fa47 Improve FindForkInGlobalIndex when locator contains chain tip
      • 7eb0d75 VerifyDB: don’t check blocks that have been pruned
      • 8adb03a Improve RewindBlockIndex when pruning
      • a1d1d0c Make sure upgraded nodes don’t ask for non-wit blocks
      • 019860e script_tests: witness tests can specify tx amount
      • c815c16 [Qt] Add support for NODE_WITNESS in formatServicesStr
      • f16067f bitcoinconsensus.h: Accept amount as int64_t
      • 059d4d1 Add GetTransactionSigOpCost unit tests
      • c1c38a2 segwit: fix gui wallet send transaction size calculation assertion failed
      • 0acd1dc segwit: txout dust threshold calculation update
    • fixups 6 (https://github.com/sipa/bitcoin/compare/869f26e...f98de5f)
      • 14d4d1d Extend the max witness program length to 40 bytes
      • 4840f6d Prevent witness addresses from being constructed before fork
      • 3dbf852 Remove positive SERIALIZE_TRANSACTION_WITNESS flag
    • fixups 7 (https://github.com/sipa/bitcoin/compare/f98de5f...7613bbb)
      • c06c40b Actually count the witness data in memusage of CTransaction
      • d8b5db9 Correctly count maximum size in mining
      • 57d4bd2 Rework -maxblocksize and -maxblockcost
      • 0bfbf60 Cache transaction cost in mempool
      • 496d8c0 Delete segnet
      • 7799a7c Do not send witnesses in response to bip37 blocks
      • 4c19c18 33 to 40 bytes push should now be considered a witness scriptPubKey
    • fixups 8 (https://github.com/sipa/bitcoin/compare/7613bbb...1b9893f)
      • a9bff09 Tests: add getblocktemplate/segwit test
      • 92ab64c Add test for getrawtransaction
      • 40f7829 p2p-segwit.py: more RPC coverage
      • b644339 Rename deployment (witness -> segwit)
      • 00bf5e1 Update p2p-segwit.py for new deployment name
      • c8f2fb2 BIP143 P2WSH examples
      • f3a7ed4 Fix unused variable in sigopcount test
    • fixups 9 (https://github.com/sipa/bitcoin/compare/1b9893f...b508f5b)
      • 3483e5c DEPLOYMENT_WITNESS -> DEPLOYMENT_SEGWIT
      • 2a6516f Behave as a non-witness node when start time is far away
      • c7b5de5 test: BIP143 examples fix and clarify
      • cc19adc Remove segnet from mininode
      • efc251d Tests: ensure that signrawtransaction failures are caught in segwit.py
      • 396f4b8 spelling fix: uncommited -> uncommitted
    • fixups 10 (https://github.com/sipa/bitcoin/compare/b508f5b...a6840e5)
      • 10433cf Fix direct fetching of blocks after 2a6516fc
      • 2948c02 Consistency in serialization flags
    • fixups 11 (https://github.com/sipa/bitcoin/compare/d7fe873...fb348c6)
      • 0e177a2 Don’t treat NODE_WITNESS as relevant before a fork is defined
      • c7795ee Revert “Don’t check the genesis block PoW” as segnet has been dropped.
    • merge (https://github.com/sipa/bitcoin/compare/fb348c6...e84733733d0ec3deee3b0d1cdcf643082b78ee1e)
      • e847337 Merge remote-tracking branch ‘upstream/master’ into segwit-master

    Code to generate this list:

    0PREV="$(git rev-parse HEAD)"; (git log --oneline upstream/master..HEAD; echo "$(git rev-parse upstream/master) --- [SEGWIT] begin: preparation ---") | while read C L; do if [ "d${L:0:13}" == "d--- [SEGWIT] " ]; then if [ "d$PREV" != "" ]; then echo "* ${L:20:-4} (https://github.com/sipa/bitcoin/compare/$C...$PREV)"; fi; PREV=$C; PREVL=$L; else echo "  * $C $L"; fi; done | tac
  8. Create segnet4 70ebe86f00
  9. Add segnet seed nodes f8bcd86d43
  10. qt: Work (don't crash) with -segnet 58847fe2c6
  11. --- [SEGWIT] begin: P2P/node/consensus --- bb4bb47d12
  12. Add segregated witness transaction serialization 
    Contains refactorings by Eric Lombrozo.
Contains fixup by Nicolas Dorier.
    3b1ff49f0c
  13. Removed ppszTypeName from protocol.cpp e35d0200eb
  14. getdata enum issue fix adb1c09b87
  15. Introduce and preferentially peer with NODE_WITNESS service bit 
    Service bit logic by Nicolas Dorier.
    ac6886d30d
  16. Witness commitment validation 
    Includes a fix by Suhas Daftuar
    cb9d4d34ca
  17. Script validation logic for witnesses 2c87be126f
  18. Enable SCRIPT_VERIFY_WITNESS for mempool transactions 1e478052ca
  19. Activate script consensus rules through BIP9 97f34f793f
  20. Only download blocks from witness peers after fork 6a8021fac7
  21. Observe input amounts: verification 271e45ef04
  22. Add signature version 1 with updated sighash 
    Includes simplifications by Eric Lombrozo.
    d374139b87
  23. Return witness data 
    Includes RPC field name changes by Luke-jr.
    92c6dc3ef8
  24. Implement block size/sigop cost rules, limits, and GBT support 
    Includes changes by Suhas Daftuar and Luke-jr.
    395521854e
  25. Add command line options to loosen mempool acceptance rules 5a47b98536
  26. bitcoinconsensus: add method that accepts amount, and return error when verify_script receives VERIFY_WITNESS flag 
    script_tests: always test bitcoinconsensus_verify_script_with_amount if VERIFY_WITNESS isn't set

Rename internal method + make it static

trim bitcoinconsensus_ prefix

Add SERIALIZE_TRANSACTION_WITNESS flag
    e5735719c4
  27. Increase MAX_PROTOCOL_MESSAGE_LENGTH 
    Witness blocks can be greater than 2MiB, but cannot be validly greater
than 4MB.
    a6386c264b
  28. Add rewind logic to deal with post-fork software updates 1b6c6f16b3
  29. --- [SEGWIT] begin: wallet --- 351a2feff7
  30. Witness script signing abbe085cae
  31. sipa force-pushed on Apr 19, 2016
  32. Add witness address RPCs (using P2SH) 
    Includes support for pushkeyhash wit v0 by Alex Morcos.
    c3fe53f364
  33. signrawtransaction can sign P2WSH ec385fa1d3
  34. --- [SEGWIT] begin: tests --- a2d6f8a5d9
  35. Signing tests 2ed1d1149a
  36. Add rpc test for segwit 
    Amended by Pieter Wuille to use multisig 1-of-1 for P2WSH tests, and BIP9
based switchover logic.
    3c0b16f487
  37. Add transaction tests for segwit 
    ...with the four types of segwit payment, as well as all sighash combinaisons.
    24746d28ef
  38. Add segwit support to script_tests 3409ad62b8
  39. Autogeneration support for witness in script_tests e5ffcc19ac
  40. Update p2p test framework with segwit support 
    mininode now supports witness transactions/blocks, blocktools
has a helper for adding witness commitments to blocks, and script
has a function to calculate hashes for signature under sigversion
1, used by segwit.
    5e6abe56db
  41. P2P test for segwit 
    Rebased by Pieter Wuille
    7b539f9bfe
  42. sipa force-pushed on Apr 19, 2016
  43. in src/rpc/misc.cpp: in 7b539f9bfe outdated 
    330+            "}\n"
331+        ;
332+        throw runtime_error(msg);
333+    }
334+
335+    std::vector<unsigned char> code = ParseHex(params[0].get_str());
    instagibbs commented at 6:02 pm on April 19, 2016:
    Probably should throw an error if it’s not hex. Currently accepts anything entered.
    sipa commented at 7:34 am on April 23, 2016:
    Agree.
  44. maaku commented at 6:51 pm on April 19, 2016: contributor

    Created pull request https://github.com/sipa/bitcoin/pull/75 on sipa’s repository against this branch. Explanation of the proposed change:

    The witness root is allowed to be placed at an arbitrary position up to seven layers deep in a Merkle tree structure. The witness nonce is now the branch through the commitment tree to the witness root, and a single byte is added to the commitment output specifying this path in compact form. This allows other consensus commitments to be added in the future with a minimal number of bytes and without committing at this time for a certain position for the segwit branch within the tree.

    In addition, switch to fast Merkle trees for witness. A fast Merkle branch uses midstate to perform a single SHA-256 compression per branch, and is not vulnerable to CVE-2012-2459. It produces different hashes though, so can only be used for new hash trees going forward.

  45. jonasschnelli added the label Consensus on Apr 19, 2016
  46. in src/wallet/rpcwallet.cpp: in 7b539f9bfe outdated 
    1132+        return false;
1133+    }
1134+};
1135+
1136+UniValue addwitnessaddress(const UniValue& params, bool fHelp)
1137+{
    instagibbs commented at 7:14 pm on April 19, 2016:
    nit: ensure wallet is available for more useful error than “not known to wallet”
    sipa commented at 10:57 am on April 23, 2016:
    Will do.
  47. morcos commented at 7:31 pm on April 19, 2016: member
    See sipa#76 for fix to CInv::GetCommand
  48. ghost commented at 1:03 am on April 20, 2016: none
    Ayayay… a third of the original client’s source base. I assume the items left to do will be committed to this PR #7910 ?
  49. in src/main.cpp: in 7b539f9bfe outdated 
    3353+    return (VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_WITNESS, versionbitscache) == THRESHOLD_ACTIVE);
3354+}
3355+
3356+void UpdateUncommitedBlockStructures(CBlock& block, const CBlockIndex* pindexPrev, const Consensus::Params& consensusParams)
3357+{
3358+    int commitpos = -1;
    gubatron commented at 6:44 am on April 20, 2016:
    this little commitpos check could use a refactor, it appears 3 times. #DRY.
    sipa commented at 9:07 am on April 20, 2016:
    Agree!
  50. LongShao007 commented at 11:10 am on April 20, 2016: contributor

    function UpdateUncommitedBlockStructures redefine in main.h

    +void UpdateUncommitedBlockStructures(CBlock& block, const CBlockIndex* pindexPrev, const Consensus::Params& consensusParams); + +/** Update uncommitted block structures (currently: only the witness nonce). This is safe for submitted blocks. / +void UpdateUncommitedBlockStructures(CBlock& block, const CBlockIndex pindexPrev, const Consensus::Params& consensusParams); +

    see sipa#78

  51. paveljanik commented at 3:18 pm on April 20, 2016: contributor
    I can sometimes repeat failed test (https://travis-ci.org/bitcoin/bitcoin/jobs/124242358#L4152) even on master - wallet.py, so it seems to be irrelevant to this PR.
  52. MarcoFalke commented at 3:21 pm on April 20, 2016: member
    Jup, since we are aware of the issue, this might be a good time to disable this specific test.
  53. theuni commented at 4:02 pm on April 20, 2016: member
    @paveljanik Thanks, that was driving me crazy yesterday while trying to figure out wtf it has to do with segwit. Great to know it’s not related.
  54. sipa commented at 4:10 pm on April 20, 2016: member
    @theuni 3rd retry of Travis, and now it succeeded. Perhaps just bad luck…
  55. --- [SEGWIT] begin: fixups --- cfb0a83d40
  56. fixup Implement block size/sigop cost rules, limits, and GBT support: use int64_t for sigopcost everywhere bb2bfcadd8
  57. fixup Implement block size/sigop cost rules, limits, and GBT support: update block cost comment 64c15279f5
  58. fixup Add segregated witness transaction serialization: comments cc741f5250
  59. fixup Update p2p test framework with segwit support: fix comment c28179f4a7
  60. fixup Witness commitment validation: list the mentioned BIPs 6a5da9bd0b
  61. fixup Add command line options to loosen mempool acceptance rules: simplify comment 88e0135601
  62. fixup Witness commitment validation: factor out GetWitnessCommitmentPos da60faea77
  63. fixup Witness commitment validation: fix typo in comment 4f827beaf8
  64. fixup Implement block size/sigop cost rules, limits, and GBT support: improve GBT help 058f495882
  65. fixup Add witness address RPCs (using P2SH): test hex & wallet available c1e7a95ce5
  66. fixup Add segregated witness transaction serialization: revert extformat-if-empty-vin dbe6391922
  67. tidy up CInv::GetCommand 97d7402c2c
  68. fixup Witness commitment validation: function UpdateUncommitedBlockStructures redefine 
    function UpdateUncommitedBlockStructures defined twice.
    f0f61237d6
  69. [qa] Use integer division, byte strings properly 
    Also refactor some code and add segwit.py to the pulltester
    a6135995d4
  70. fixup Add segregated witness transaction serialization: add missing witness flags 5d6b6e288d
  71. in src/main.cpp: in 7b539f9bfe outdated 
    1167@@ -1144,8 +1168,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
1168         if (fRequireStandard && !AreInputsStandard(tx, view))
1169             return state.Invalid(false, REJECT_NONSTANDARD, "bad-txns-nonstandard-inputs");
1170 
1171-        unsigned int nSigOps = GetLegacySigOpCount(tx);
1172-        nSigOps += GetP2SHSigOpCount(tx, view);
1173+        unsigned int nSigOpsCost = GetTransactionSigOpCost(tx, view, STANDARD_SCRIPT_VERIFY_FLAGS);
    SergioDemianLerner commented at 5:40 pm on April 20, 2016:
    GetTransactionSigOpCost() could return a value close to 2^29 (4M * 20 (multisig) * 4) so to be safe nSigOps should be an int64_t. That can prevent overflows on future witness size extensions.
    jl2012 commented at 6:32 pm on April 20, 2016:
    Why not (4M * 20 (multisig))? You may have 4MB only if it is a segwit tx, and cost of segwit sigops does not need to * 4
    sipa commented at 8:34 pm on April 20, 2016:
    I agree with jl2012, and I think an int64_t is not necessary at this point. It won’t hurt though, so I’ll switch the type.
    dcousens commented at 3:32 am on April 21, 2016:
    ACK switch to uint64_t, its what is returned by GetTransactionSigOpCost any way… explicit casts should be the rule, not the exception (IMHO).
  72. in src/main.cpp: in 7b539f9bfe outdated 
    2368@@ -2326,7 +2369,7 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
2369     std::vector<int> prevheights;
2370     CAmount nFees = 0;
2371     int nInputs = 0;
2372-    unsigned int nSigOps = 0;
2373+    unsigned int nSigOpsCost = 0;
    SergioDemianLerner commented at 5:43 pm on April 20, 2016:
    same here. nSigOps -> int64_t
    sipa commented at 10:55 am on April 23, 2016:
    Thanks, will do.
  73. in src/primitives/block.cpp: in 7b539f9bfe outdated 
    35+int64_t GetBlockCost(const CBlock& block)
36+{
37+    // The intended approximate formula is: cost = base_size * 4 + witness_size.
38+    // We can only serialize base or base+witness, so the formula
39+    // becomes: cost = base_size * 3 + total_size.
40+    return ::GetSerializeSize(block, SER_NETWORK, 0) * (WITNESS_SCALE_FACTOR - 1) + ::GetSerializeSize(block, SER_NETWORK, SERIALIZE_TRANSACTION_WITNESS);
    SergioDemianLerner commented at 6:06 pm on April 20, 2016:
    GetBlockCost() is part of consensus-critical code. Why is the phrase “approximate formula” used in a comment here? Either the formula is exact (and matches the BIP) or it is incorrect.
    sipa commented at 8:40 pm on April 20, 2016:

    The formula is exact.

    It’s trying to explain the rationale for this rule (basesize * 3 + totalsize) by showing that it corresponds to (basesize * 4 + witsize). But perhaps this rationale belongs in the BIP and not in the code.

    dooglus commented at 7:56 pm on April 21, 2016:
    So just remove the word ‘approximate’? I think it is helpful to keep the rest of the explanation here.
    sipa commented at 10:44 am on April 23, 2016:
    Will do.
  74. in src/consensus/consensus.h: in 7b539f9bfe outdated 
     5@@ -6,10 +6,14 @@
 6 #ifndef BITCOIN_CONSENSUS_CONSENSUS_H
 7 #define BITCOIN_CONSENSUS_CONSENSUS_H
 8 
 9-/** The maximum allowed size for a serialized block, in bytes (network rule) */
10-static const unsigned int MAX_BLOCK_SIZE = 1000000;
11+/** The maximum allowed size for a serialized block, in bytes (only for buffer size limits) */
12+static const unsigned int MAX_BLOCK_SERIALIZED_SIZE = 4000000;
13+/** The maximum allowed cost for a block, see BIP 141 (network rule) */
    dcousens commented at 2:48 am on April 21, 2016:
    What was the reasoning behind separating this from being derived from the network rule?
    sipa commented at 10:31 am on April 23, 2016:
    It is not derived from the network rule because it is the (only) network rule. What would you derive it from?
    dcousens commented at 3:40 am on April 26, 2016:
    All good, I forgot I made that comment before finishing reviewing the majority of the PR, it makes sense now :+1:
  75. in src/main.cpp: in 7b539f9bfe outdated 
    1051@@ -1033,6 +1052,11 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
1052         return state.DoS(0, false, REJECT_NONSTANDARD, "premature-version2-tx");
1053     }
1054 
1055+    // Don't accept witness transactions before the final threshold passes
    dcousens commented at 3:16 am on April 21, 2016:

    Reject witness transactions if segregated witness is disabled [and a witness exists]; override with -prematurewitness to bypass

    IMHO, reads easier, less negations to do in my head

    sipa commented at 10:55 am on April 23, 2016:
    Will fix.
  76. in src/main.cpp: in 7b539f9bfe outdated 
    1201@@ -1179,9 +1202,9 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
1202         // itself can contain sigops MAX_STANDARD_TX_SIGOPS is less than
1203         // MAX_BLOCK_SIGOPS; we still consider this an invalid rather than
1204         // merely non-standard transaction.
1205-        if ((nSigOps > MAX_STANDARD_TX_SIGOPS) || (nBytesPerSigOp && nSigOps > nSize / nBytesPerSigOp))
1206+        if ((nSigOpsCost > MAX_STANDARD_TX_SIGOPS_COST) || (nBytesPerSigOp && nSigOpsCost > nSize * WITNESS_SCALE_FACTOR / nBytesPerSigOp))
    dcousens commented at 3:24 am on April 21, 2016:
    Could this be bracketed such that precedence rules don’t need to be remembered?
    sipa commented at 10:37 am on April 23, 2016:
    I don’t think that’s necessary here.
  77. in src/consensus/consensus.h: in 7b539f9bfe outdated 
     5@@ -6,10 +6,14 @@
 6 #ifndef BITCOIN_CONSENSUS_CONSENSUS_H
 7 #define BITCOIN_CONSENSUS_CONSENSUS_H
 8 
 9-/** The maximum allowed size for a serialized block, in bytes (network rule) */
10-static const unsigned int MAX_BLOCK_SIZE = 1000000;
11+/** The maximum allowed size for a serialized block, in bytes (only for buffer size limits) */
12+static const unsigned int MAX_BLOCK_SERIALIZED_SIZE = 4000000;
13+/** The maximum allowed cost for a block, see BIP 141 (network rule) */
14+static const unsigned int MAX_BLOCK_COST = 4000000;
    dcousens commented at 3:33 am on April 21, 2016:
    Less implicit casts if this is also uint64_t
    sipa commented at 10:54 am on April 23, 2016:
    Will turn all sigops cost variables into int64_t
  78. in src/main.cpp: in 7b539f9bfe outdated 
    3493+            }
3494+            fHaveWitness = true;
3495+        }
3496+    }
3497+
3498+    // No witness data is allowed in blocks that don't commit to witness data, as this would otherwise leave room from spam
    dooglus commented at 7:48 pm on April 21, 2016:
    You mean “room for spam”?
    sipa commented at 10:56 am on April 23, 2016:
    I do!
  79. in src/main.cpp: in 7b539f9bfe outdated 
    3355+
3356+void UpdateUncommitedBlockStructures(CBlock& block, const CBlockIndex* pindexPrev, const Consensus::Params& consensusParams)
3357+{
3358+    int commitpos = -1;
3359+    for (size_t o = 0; o < block.vtx[0].vout.size(); o++) {
3360+        if (block.vtx[0].vout[o].scriptPubKey.size() >= 38 && block.vtx[0].vout[o].scriptPubKey[0] == OP_RETURN && block.vtx[0].vout[o].scriptPubKey[1] == 0x24 && block.vtx[0].vout[o].scriptPubKey[2] == 0xaa && block.vtx[0].vout[o].scriptPubKey[3] == 0x21 && block.vtx[0].vout[o].scriptPubKey[4] == 0xa9 && block.vtx[0].vout[o].scriptPubKey[5] == 0xed) {
    kanzure commented at 8:06 pm on April 21, 2016:

    would be nice to shorten this line, plus it’s essentially repeated soon after a few lines later….

    0if (block.vtx[0].vout[o].scriptPubKey.size() >= 38 && block.vtx[0].vout[o].scriptPubKey[0] == OP_RETURN && block.vtx[0].vout[o].scriptPubKey[1] == 0x24 && block.vtx[0].vout[o].scriptPubKey[2] == 0xaa && block.vtx[0].vout[o].scriptPubKey[3] == 0x21 && block.vtx[0].vout[o].scriptPubKey[4] == 0xa9 && block.vtx[0].vout[o].scriptPubKey[5] == 0xed) {
    sipa commented at 10:42 am on April 23, 2016:
    Yes, will factor it out to a separate function.
  80. in src/main.cpp: in 7b539f9bfe outdated 
    2668@@ -2629,7 +2669,7 @@ void static UpdateTip(CBlockIndex *pindexNew) {
2669 }
2670 
2671 /** Disconnect chainActive's tip. You probably want to call mempool.removeForReorg and manually re-limit mempool size after this, with cs_main held. */
2672-bool static DisconnectTip(CValidationState& state, const Consensus::Params& consensusParams)
2673+bool static DisconnectTip(CValidationState& state, const Consensus::Params& consensusParams, bool fBare = false)
    MarcoFalke commented at 9:50 am on April 22, 2016:
    Needs merge conflict solved.
  81. in qa/rpc-tests/test_framework/mininode.py: in 7b539f9bfe outdated 
    22@@ -23,7 +23,7 @@
23 import time
24 import sys
25 import random
26-from binascii import hexlify, unhexlify
27+from util import *
    MarcoFalke commented at 9:50 am on April 22, 2016:
    Nit: Let’s not do wildcard imports in mininode.
  82. in qa/rpc-tests/test_framework/blocktools.py: in 7b539f9bfe outdated 
    21@@ -22,6 +22,29 @@ def create_block(hashprev, coinbase, nTime=None):
22     block.calc_sha256()
23     return block
24 
25+# From BIP141
26+WITNESS_COMMITMENT_HEADER = "\xaa\x21\xa9\xed"
27+
28+# According to BIP141, nVersion=5 blocks must commit to the
    btcdrak commented at 10:17 am on April 22, 2016:
    This comment doesn’t look right, since we’re using version bits deployment.
    sipa commented at 10:51 am on April 23, 2016:
    Will fix.
  83. in src/consensus/params.h: in 7b539f9bfe outdated 
    15@@ -16,6 +16,7 @@ enum DeploymentPos
16 {
17     DEPLOYMENT_TESTDUMMY,
18     DEPLOYMENT_CSV, // Deployment of BIP68, BIP112, and BIP113.
19+    DEPLOYMENT_WITNESS,
    btcdrak commented at 10:41 am on April 22, 2016:
    Add comment Deployment of BIP141 and BIP143
    sipa commented at 10:55 am on April 23, 2016:
    Will do.
  84. in src/chainparams.cpp: in 7b539f9bfe outdated 
    92@@ -92,6 +93,11 @@ class CMainParams : public CChainParams {
93         consensus.vDeployments[Consensus::DEPLOYMENT_CSV].nStartTime = 1462060800; // May 1st, 2016
94         consensus.vDeployments[Consensus::DEPLOYMENT_CSV].nTimeout = 1493596800; // May 1st, 2017
95 
96+        // Deployment of SegWit.
    btcdrak commented at 10:42 am on April 22, 2016:

    add the BIP numbers being deployed.

    Deployment of SegWit, BIP141 and BIP143

    sipa commented at 10:51 am on April 23, 2016:
    Will fix.
  85. in src/rpc/mining.cpp: in 7b539f9bfe outdated 
    331@@ -330,13 +332,15 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp)
332             "  \"transactions\" : [                (array) contents of non-coinbase transactions that should be included in the next block\n"
333             "      {\n"
334             "         \"data\" : \"xxxx\",          (string) transaction data encoded in hexadecimal (byte-for-byte)\n"
335-            "         \"hash\" : \"xxxx\",          (string) hash/id encoded in little-endian hexadecimal\n"
336+            "         \"txid\" : \"xxxx\",          (string) transaction id encoded in little-endian hexadecimal\n"
337+            "         \"hash\" : \"xxxx\",          (string) hash encoded in little-endian hexadecimal\n"
    instagibbs commented at 10:03 pm on April 22, 2016:
    Rename to “witnesshash”, or mention hash is based on witnesshash in description.
    sipa commented at 7:33 am on April 23, 2016:
    This is from BIP145, though I agree it’s helpful to document.
  86. sipa commented at 12:17 pm on April 23, 2016: member

    I have included fixup commits for almost all comments, as well as a merge commit at the end to keep the result testable and show what changes are needed to rebase on master.

    I intend to only overwrite the merge commit at the end, and otherwise only append fixup commits (and where possible, list which commit they modify). I will also keep the broken-down commit list on #7910 (comment) up to date with every push.

  87. in src/main.cpp: in 81b3d67eac outdated 
    3468+    // * We compute the witness hash (which is the hash including witnesses) of all the block's transactions, except the
3469+    //   coinbase (where 0x0000....0000 is used instead).
3470+    // * The coinbase scriptWitness is a stack of a single 32-byte vector, containing a witness nonce (unconstrained).
3471+    // * We build a merkle tree with all those witness hashes as leaves (similar to the hashMerkleRoot in the block header).
3472+    // * The must be at least one output whose scriptPubKey is a single 36-byte push, the first 4 bytes of which are
3473+    //   {0xaa, 0x21, 0xa9, 0xed}, and the following 32 bytes are SHA256(witness root, witness nonce). In case there are
    jl2012 commented at 3:22 pm on April 25, 2016:
    Double SHA256?
  88. in src/main.cpp: in 81b3d67eac outdated 
    1409+            // SCRIPT_VERIFY_CLEANSTACK requires SCRIPT_VERIFY_WITNESS, so we
1410+            // need to turn both off, and compare against just turning off CLEANSTACK
1411+            // to see if the failure is specifically due to witness validation.
1412+            if (CheckInputs(tx, state, view, true, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true) &&
1413+                !CheckInputs(tx, state, view, true, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true)) {
1414+                // Only the witness is wrong, so the transaction itself may be fine.
    instagibbs commented at 3:27 pm on April 25, 2016:

    what does “may be fine” mean here?

    edit: nevermind, I see the explanation here: https://github.com/bitcoin/bitcoin/commit/1e478052ca3222d13ce3646dd152ebcf27f07e76#diff-7ec3c68a81efff79b6ca22ac1f1eabbaR5221

  89. morcos commented at 3:30 pm on April 25, 2016: member
    I think there are a couple of more places that need the SERIALIZE_TRANSACTION_WITNESS flag: rest_tx and rest_block in rest.cpp, CDB:Rewrite in db.cpp, and I wasn’t sure about CWalletDB::Recover in walletdb.cpp.
  90. in src/primitives/transaction.h: in 81b3d67eac outdated 
    281+        /* Try to read the vin. In case the dummy is there, this will be read as an empty vector. */
282+        READWRITE(*const_cast<std::vector<CTxIn>*>(&tx.vin));
283+        if (tx.vin.size() == 0 && (nVersion & SERIALIZE_TRANSACTION_WITNESS)) {
284+            /* We read a dummy or an empty vin. */
285+            READWRITE(flags);
286+            if (flags != 0) {
    morcos commented at 4:59 pm on April 25, 2016:

    Do we need to handle the case that flags == 0? For instance, the serialization of a transaction with no inputs and no outputs, maybe we should explicitly make sure tx.vout is set empty?

    (i put this same comment somewhere wrong initially)

    sipa commented at 5:33 pm on April 25, 2016:
    That is intentional. If a transaction starting with “[version] 0x00 0x00 …” is seen, it is treated as one with no inputs and no outputs.
  91. in src/test/data/tx_invalid.json: in 81b3d67eac outdated 
    31@@ -32,7 +32,7 @@
32 ["Tests for CheckTransaction()"],
33 ["No inputs"],
34 [[["0000000000000000000000000000000000000000000000000000000000000100", 0, "HASH160 0x14 0x7a052c840ba73af26755de42cf01cc9e0a49fef0 EQUAL"]],
35-"0100000000010000000000000000015100000000", "P2SH"],
36+"01000000000000010000000000000000015100000000", "P2SH"],
    morcos commented at 5:29 pm on April 25, 2016:
    I think after your fixup commit this doesn’t deserialize properly, which probably isn’t what we’re supposed to be testing here.
  92. in src/miner.cpp: in cb9d4d34ca outdated 
    185@@ -183,6 +186,9 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const CScript& s
186 
187             const CTransaction& tx = iter->GetTx();
188 
189+            if (!fIncludeWitness && !tx.wit.IsNull())
    morcos commented at 9:34 pm on April 25, 2016:
    This isn’t necessary right? Perhaps we need a better way to identify which rules for block assembly are enforced by the mempool, but I’d rather that we keep redundant checks out to make future work on block assembly easier. At the very least this should be commented that unless there is a reorg to a lower height more work chain immediately at the segwit activation border, then this check is superfluous.
    sipa commented at 9:56 pm on April 25, 2016:
    Agree, this is a hack.
    sipa commented at 10:58 pm on April 27, 2016:
    Going to replace it with an extra delay before witness transactions are accepted to the mempool.
    morcos commented at 11:17 pm on April 27, 2016:
    @sipa I’m not sure it’s necessary to replace it with anything. Given the way versionbits soft forks activate, its not really possible to have it unactivate without an enormous reorg
  93. in src/miner.cpp: in 81b3d67eac outdated 
    93+    pblocktemplate->vTxSigOpsCost.push_back(-1); // updated at end
94 
95     // Largest block you're willing to create:
96-    unsigned int nBlockMaxSize = GetArg("-blockmaxsize", DEFAULT_BLOCK_MAX_SIZE);
97+    unsigned int nBlockMaxCost = GetArg("-blockmaxsize", DEFAULT_BLOCK_MAX_SIZE) * WITNESS_SCALE_FACTOR;
98     // Limit to between 1K and MAX_BLOCK_SIZE-1K for sanity:
    instagibbs commented at 2:12 pm on April 26, 2016:
    nit: update comment to cost/4k
  94. in src/main.cpp: in 81b3d67eac outdated 
    3480+            uint256 hashWitness = BlockWitnessMerkleRoot(block, &malleated);
3481+            // The malleation check is ignored; as the transaction tree itself
3482+            // already does not permit it, it is impossible to trigger in the
3483+            // witness tree.
3484+            if (block.vtx[0].wit.vtxinwit.size() != 1 || block.vtx[0].wit.vtxinwit[0].scriptWitness.stack.size() != 1 || block.vtx[0].wit.vtxinwit[0].scriptWitness.stack[0].size() != 32) {
3485+                return state.DoS(100, error("%s : invalid witness commitment size", __func__), REJECT_INVALID, "bad-witness-merkle-size", true);
    morcos commented at 2:58 pm on April 26, 2016:
    This error message might be clearer if it indicated that it was the “nonce” that was the wrong size.
  95. in src/main.cpp: in 1e478052ca outdated 
    5184@@ -5176,8 +5185,10 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5185             if (nEvicted > 0)
5186                 LogPrint("mempool", "mapOrphan overflow, removed %u tx\n", nEvicted);
5187         } else {
5188-            assert(recentRejects);
5189-            recentRejects->insert(tx.GetHash());
5190+            if (!state.CorruptionPossible()) {
5191+                assert(recentRejects);
5192+                recentRejects->insert(tx.GetHash());
    sdaftuar commented at 7:27 pm on April 26, 2016:
    This is minor, but shouldn’t we put a similar guard up above, where failed-orphan transactions get added to recentRejects? Presumably failed orphans should also only be added to recentRejects if the corruption field is not set?
    sipa commented at 11:34 pm on April 27, 2016:
    Indeed, and likewise for the misbehaviour assignment.
  96. in src/script/interpreter.cpp: in d374139b87 outdated 
    1109@@ -1106,8 +1110,64 @@ class CTransactionSignatureSerializer {
1110 
1111 } // anon namespace
1112 
1113-uint256 SignatureHash(const CScript& scriptCode, const CTransaction& txTo, unsigned int nIn, int nHashType)
1114+uint256 SignatureHash(const CScript& scriptCode, const CTransaction& txTo, unsigned int nIn, int nHashType, const CAmount& amount, int sigversion)
1115 {
1116+    if (sigversion == 1) {
    morcos commented at 5:02 pm on April 27, 2016:
    nit: would it be easier to read if any time we had a sigversion we used an enum?
    sipa commented at 11:54 pm on April 27, 2016:
    Oh yes!
  97. in src/rpc/blockchain.cpp: in 81b3d67eac outdated 
    332@@ -331,6 +333,7 @@ UniValue getblockheader(const UniValue& params, bool fHelp)
333             "  \"previousblockhash\" : \"hash\",  (string) The hash of the previous block\n"
334             "  \"nextblockhash\" : \"hash\",      (string) The hash of the next block\n"
335             "  \"chainwork\" : \"0000...1f3\"     (string) Expected number of hashes required to produce the current chain (in hex)\n"
336+            "  \"nextblockhash\" : \"hash\"       (string) The hash of the next block\n"
    morcos commented at 5:36 pm on April 27, 2016:
    maybe rebase error? nextblockhash is repeated. to keep the order the same as output, chainwork should go before previousblockhash and nextblockhash.
    instagibbs commented at 5:52 pm on April 27, 2016:
    the ordering has always been wrong: #7194
  98. --- [SEGWIT] begin: fixups 2 --- 77c613c3b7
  99. fixup Add segregated witness transaction serialization: add negative flag 
    This is the beginning of a transition to using an opt-out NO_WITNESS flag
rather than an opt-in WITNESS flag. Having them temporary both at the same
time means we can do sanity checking in every transaction serialization to
be sure exactly one of both flags is set.

Most of the changes in this commit will be undone when the positive flag is
removed.
    f889bec471
  100. fixup Add segregated witness transaction serialization: test with no inputs no longer possible 
    The base commit for segwit serialization uses extended format for transactions without inputs.
In "fixup Add segregated witness transaction serialization: revert extformat-if-empty-vin" this
was changed, but the test for CheckTransaction with no inputs was not removed. Do that now.
    3ddabe4185
  101. fixup Add segregated witness transaction serialization: deal with overwrite and inconsistencies c744a1ef13
  102. fixup Witness commitment validation: revert adding of commitment in IncrementExtraNonce 47095950fd
  103. fixup Witness commitment validation: correct comment 122ca81fc7
  104. fixup Witness commitment validation: add comment about witness checking in miner 5df3e51530
  105. fixup Witness commitment validation: better validation error message 36e1656823
  106. fixup Implement block size/sigop cost rules, limits, and GBT support: update comment f7264020d7
  107. fixup Enable SCRIPT_VERIFY_WITNESS for mempool transactions: do not reject/punish invalid witness orphans cf2c531123
  108. fixup Return witness data: don't list nextblockhash twice 0fb6e4e293
  109. [qa] mininode: Use hexlify wrapper from util ccd0e6ce23
  110. Implement RecursiveDynamicUsage for witness structures 3a62cb95e9
  111. Use an enum for signature versions a541f0b86f
  112. sipa force-pushed on Apr 28, 2016
  113. sipa commented at 12:32 pm on April 28, 2016: member
    Pushed a new batch of fixups.
  114. in src/primitives/transaction.h: in 588da1e3a3 outdated 
    295+            }
296+        } else {
297+            /* We read a non-empty vin. Assume a normal vout follows. */
298+            READWRITE(*const_cast<std::vector<CTxOut>*>(&tx.vout));
299+        }
300+        const_cast<CTxWitness*>(&tx.wit)->SetNull();
    morcos commented at 1:31 pm on April 28, 2016:
    this is redundant to line 286 now
  115. in src/main.cpp: in 588da1e3a3 outdated 
    4102+            pindexIter->nChainTx = 0;
4103+            pindexIter->nSequenceId = 0;
4104+            // Make sure it gets written.
4105+            setDirtyBlockIndex.insert(pindexIter);
4106+            // Update setBlockIndexCandidates
4107+            setBlockIndexCandidates.erase(pindexIter);
    morcos commented at 2:41 pm on April 28, 2016:
    do we also need to remove it from mapBlocksUnlinked?
  116. in src/version.h: in 588da1e3a3 outdated 
    38@@ -39,4 +39,7 @@ static const int SENDHEADERS_VERSION = 70012;
39 //! "feefilter" tells peers to filter invs to you by fee starts with this version
40 static const int FEEFILTER_VERSION = 70013;
41 
42+//! Version after which witness support potentially exists
43+static const int WITNESS_VERSION = 70013;
    sdaftuar commented at 0:06 am on April 29, 2016:
    This variable doesn’t appear to be used by anything; I assume this was introduced before switching to the service bit, not sure if you intended to leave it here as a useful marker, or remove?
  117. in src/main.cpp: in 588da1e3a3 outdated 
    4073+
4074+    // nHeight is now the height of the first insufficiently-validated block, or tipheight + 1
4075+    CValidationState state;
4076+    CBlockIndex* pindex = chainActive.Tip();
4077+    while (chainActive.Height() >= nHeight) {
4078+        if (!DisconnectTip(state, params, true)) {
    sdaftuar commented at 0:14 am on April 29, 2016:
    This logic will cause pruning nodes to fail (un-gracefully) if they try to upgrade after activation and the activation block has been pruned (which could be as short as two days). I propose that for pruning nodes, we don’t bother trying to rewind past their HAVE_DATA point.
    sipa commented at 0:26 am on May 11, 2016:
    Sounds good, can you provide a patch?
  118. in src/script/interpreter.h: in 588da1e3a3 outdated 
    85@@ -86,16 +86,30 @@ enum
86     //
87     // See BIP112 for details
88     SCRIPT_VERIFY_CHECKSEQUENCEVERIFY = (1U << 10),
89+
90+    // Support segregated witness
91+    //
92+    SCRIPT_VERIFY_WITNESS = (1U << 11),
93+
94+    // Making v2-v16 witness program non-standard
    jl2012 commented at 8:03 pm on April 29, 2016:
    // Making v1-v16 witness program non-standard
  119. theuni commented at 8:16 pm on May 2, 2016: member

    As requested in the last two IRC meetings, here’s the coverage data for segwit (at 588da1e3a35c75c1d4cbe50b0224cca810db0bf0): https://dev.bitcoincore.org/cfields/post-segwit.coverage/

    It’s missing branching data, and inlining throws off a few things, but I think it gives a good idea of what’s being tested. This data includes tests from test_bitcoin, the java pull-tester, and regular+extended rpc tests (minus pruning).

    A few interesting untested lines that jumped out at a glance: src/primitives/transaction.h:253 src/primitives/transaction.h:309

    Edit: Updated with much more accurate info that now includes branching and auto-inlined funcs.

  120. NicolasDorier commented at 3:54 am on May 3, 2016: contributor
    @theuni @sipa This cover both (to squash with my test commit): https://github.com/NicolasDorier/bitcoin/commit/ad366c56536019c180901194baa376d46052fb4a
  121. jonasnick commented at 1:59 am on May 4, 2016: contributor
    https://github.com/sipa/bitcoin/pull/81/ increases coverage by causing a SCRIPT_ERR_WITNESS_MALLEATED_P2SH error.
  122. jtimon commented at 3:38 am on May 5, 2016: contributor

    I’m sorry, I still cannot read this. That doesn’t mean that I think the PR is not well written or it’s incomplete. It’s just too much to review for me personally. Completely selfishly, I will promote a division of the PR in 2:

    1. do the most tested still-not-activate consensus changes in 0.12.99 as soon that part is ready. I don’t think we’re too far away from that, people have been really testing this since last time I wrote I line on it (which has been re-written a hundred times already). I’m not trying to claim any credit I don’t deserve (that would be the only “sin” I couldn’t tollerate in any culture, I am really really tolerant for “mew” cultures I don’t know about): I will review that.

    What about consensus -about-ready and the rest we merge next?

    There shouldn’t be many more fixes, specially in the consensus part, let’s just separate review and belief. At this point, I bealieve…|

    TL:DR;

    If this is not going to be merged soon, let’s get the inoucuous consensus part firt, now, or…whenever, the timing is not important. That’ my nit, I think.

    I simply cannot review consensus-ready changes and the future at the same time. Simple consensus with bip9 it’s the review I’ll do. Backport consensus rules, and then the actually useful stuff can join later too.

    EDIT Please, let’s just do the consensus commits first without activation and ley’s not encourage anyone to upgrade. Than we can review much more easily

  123. --- [SEGWIT] begin: fixups 3 --- f38671f3d0
  124. test: WITNESS flag must be used with P2SH flag 
    WITNESS flag must be used with P2SH flag. If not, and if the script passes, the test will crash.
    ba7e292ba6
  125. BIP9 parameters for testnet e7821e910d
  126. Segwit script error unit tests 76142afd28
  127. sipa force-pushed on May 10, 2016
  128. sipa commented at 1:58 am on May 10, 2016: member
    @jtimon All the consensus logic is already combined into a single group of commits (see #7910 (comment) which I maintain for every push). As mentioned several times in the weekly meetings, I’m also not rebasing anymore, so you don’t have to worry about review going stale.
  129. --- [SEGWIT] begin: fixups 4 --- 8708de8a74
  130. f cb9d4d34: typo fix b846298163
  131. f 3b1ff49f: remove redundant witness clean 38e3fcd2dc
  132. f 1b6c6f16: Cleanup mapBlocksUnlinked in RewindBlockIndex fd85b77404
  133. f ac6886d3: Get rid of leftover WITNESS_VERSION 433daf4eef
  134. f 2c87be12: fix typo 9590289f4f
  135. f 39552185: GetBlockCost: Clarify comment description 17277c9f81
  136. f 39552185: provide both -maxblocksize and -maxblockcost 1e9cba2873
  137. [qa] segwit: Switch to py3 8f50b96c82
  138. f 1b6c6f16: pass CChainParams rather than Consensus::Params to RewindBlockIndex 75335ca109
  139. in src/main.cpp: in ad4f9c71d0 outdated 
    3460@@ -3350,6 +3461,53 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIn
3461         }
3462     }
3463 
3464+    // Validation for witness commitments.
3465+    // * We compute the witness hash (which is the hash including witnesses) of all the block's transactions, except the
3466+    //   coinbase (where 0x0000....0000 is used instead).
3467+    // * The coinbase scriptWitness is a stack of a single 32-byte vector, containing a witness nonce (unconstrained).
3468+    // * We build a merkle tree with all those witness hashes as leaves (similar to the hashMerkleRoot in the block header).
3469+    // * The must be at least one output whose scriptPubKey is a single 36-byte push, the first 4 bytes of which are
    wallclockbuilder commented at 2:39 am on May 10, 2016:
    • // * TheThere must be at least one output whose scriptPubKey is a single 36-byte push, the first 4 bytes of which are
  140. sipa force-pushed on May 11, 2016
  141. sipa commented at 7:42 am on May 11, 2016: member
    More comments and nits addressed, testnet activation included, separated -maxblocksize and -maxblockcost (by @luke-jr).
  142. mruddy commented at 1:19 pm on May 12, 2016: contributor
    Since signature/witness data gets a 75% discount in the new cost calculation, I’m wondering if the constant of 148u in GetDustThreshold in transaction.h should be updated too? Should that be changed to 108*0.25 + 32 + 4 + 4 = 67 ?
  143. sipa commented at 0:40 am on May 15, 2016: member
    I added a commit section “commentary” that contains comments on the changes (and not on the resulting code). Maybe they’re useful for people asking “why is X being changed?”.
  144. in src/script/bitcoinconsensus.cpp: in e5735719c4 outdated 
    100 
101+int bitcoinconsensus_verify_script_with_amount(const unsigned char *scriptPubKey, unsigned int scriptPubKeyLen, uint64_t amount,
102+                                    const unsigned char *txTo        , unsigned int txToLen,
103+                                    unsigned int nIn, unsigned int flags, bitcoinconsensus_error* err)
104+{
105+    CAmount am(amount);
    theuni commented at 11:13 am on May 20, 2016:

    This casts uint64_t -> int64_t. Should probably test the uint64_t directly against MAX_MONEY and fail immediately here as necessary.

    Maybe bitcoinconsensus_ERR_TX_AMOUNT_VALUE?

    afk11 commented at 12:28 pm on May 20, 2016:
    Or just accept int64_t, as amounts are never interpreted as unsigned..
    theuni commented at 12:39 pm on May 20, 2016:
    I assumed that (self-documentation) was the reason for the uint64_t.
  145. in src/test/script_tests.cpp: in abbe085cae outdated 
    163 #if defined(HAVE_CONSENSUS_LIB)
164     CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
165     stream << tx2;
166     if (flags & bitcoinconsensus_SCRIPT_FLAGS_VERIFY_WITNESS) {
167-        BOOST_CHECK_MESSAGE(bitcoinconsensus_verify_script_with_amount(begin_ptr(scriptPubKey), scriptPubKey.size(), amountZero, (const unsigned char*)&stream[0], stream.size(), 0, flags, NULL) == expect, message);
168+        BOOST_CHECK_MESSAGE(bitcoinconsensus_verify_script_with_amount(begin_ptr(scriptPubKey), scriptPubKey.size(), txCredit.vout[0].nValue, (const unsigned char*)&stream[0], stream.size(), 0, flags, NULL) == expect, message);
    sdaftuar commented at 11:30 am on May 20, 2016:
    It looks like none of the tests actually use a non-zero value – seems like we should add a way to exercise the code in the non-zero value case (as the zero case is special).
  146. in qa/rpc-tests/maxuploadtarget.py: in 5b0cd48d9f outdated 
    182 
183-        # 144MB will be reserved for relaying new blocks, so expect this to
184-        # succeed for ~70 tries.
185+        # 576MB will be reserved for relaying new blocks, so expect this to
186+        # succeed for ~235 tries.
187         for i in range(success_count):
    nicola commented at 7:32 pm on May 20, 2016:
    do you mind explaining this calculation?
    rebroad commented at 10:21 am on August 25, 2016:
    at least 4 people so far waiting for an answer to this one, @sipa curious minds want to know!
    MarcoFalke commented at 10:28 am on August 25, 2016:
    @rebroad and other 3: Explanation should be in OP of #8559
  147. in src/bitcoin-tx.cpp: in 5b0cd48d9f outdated 
    357@@ -358,6 +358,18 @@ vector<unsigned char> ParseHexUO(map<string,UniValue>& o, string strKey)
358     return ParseHexUV(o[strKey], strKey);
359 }
360 
361+static CAmount AmountFromValue(const UniValue& value)
    jonasschnelli commented at 8:46 am on May 21, 2016:
    nit: I think we should place this in a utility class and use the same implementation also for bitcoin-txs (static CAmount AmountFromValue(const UniValue& value)). Can be done post this PR.
  148. in src/main.cpp: in 5b0cd48d9f outdated 
    3373+        }
3374+    }
3375+    return commitpos;
3376+}
3377+
3378+void UpdateUncommitedBlockStructures(CBlock& block, const CBlockIndex* pindexPrev, const Consensus::Params& consensusParams)
    sdaftuar commented at 8:47 am on May 21, 2016:
    nit: “Uncommited” -> “Uncommitted”
  149. in src/chainparamsbase.cpp: in 5b0cd48d9f outdated 
    21@@ -21,6 +22,7 @@ void AppendParamsHelpMessages(std::string& strUsage, bool debugHelp)
22     if (debugHelp) {
23         strUsage += HelpMessageOpt("-regtest", "Enter regression test mode, which uses a special chain in which blocks can be solved instantly. "
24                                    "This is intended for regression testing tools and app development.");
25+        strUsage += HelpMessageOpt("-segnet", "Enter segregated witness test mode. ");
    jonasschnelli commented at 8:51 am on May 21, 2016:
    nit: s/mode. “)/mode.”)/
  150. in src/consensus/consensus.h: in 5b0cd48d9f outdated 
    11+#include <stdint.h>
12+
13+/** The maximum allowed size for a serialized block, in bytes (only for buffer size limits) */
14+static const unsigned int MAX_BLOCK_SERIALIZED_SIZE = 4000000;
15+/** The maximum allowed cost for a block, see BIP 141 (network rule) */
16+static const unsigned int MAX_BLOCK_COST = 4000000;
    sdaftuar commented at 8:53 am on May 21, 2016:
    This may be too much bikeshedding, but I think this would be clearer as MAX_BLOCK_SIZE_COST (and eg GetBlockSizeCost() elsewhere) to distinguish from the MAX_BLOCK_SIGOPS_COST.
    rebroad commented at 10:22 am on August 25, 2016:
    Oooh! We’ve lost the 1MB block limit rule?! ;)
  151. in src/script/standard.cpp: in 5b0cd48d9f outdated 
    299@@ -282,3 +300,26 @@ CScript GetScriptForMultisig(int nRequired, const std::vector<CPubKey>& keys)
300     script << CScript::EncodeOP_N(keys.size()) << OP_CHECKMULTISIG;
301     return script;
302 }
303+
304+CScript GetScriptForWitness(const CScript& redeemscript)
    instagibbs commented at 9:08 am on May 21, 2016:

    I think “GetWitnessProgramForScript” is more direct and clear.

    I had a misunderstanding of “witness program” definition somehow.

    “GetScriptForWitnessScript” isn’t much clearer either. Hm.

  152. in src/script/interpreter.cpp: in 5b0cd48d9f outdated 
    1298@@ -1239,28 +1299,106 @@ bool TransactionSignatureChecker::CheckSequence(const CScriptNum& nSequence) con
1299     return true;
1300 }
1301 
1302-bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* serror)
1303+static bool VerifyWitnessProgram(const CScriptWitness& witness, int witversion, const std::vector<unsigned char>& program, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* serror)
    instagibbs commented at 9:23 am on May 21, 2016:

    WitnessProgram has a specific meaning, which is defined in https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#witness-program

    Therefore, this function would be better named as “VerifyWitness”.

  153. in src/script/interpreter.cpp: in 5b0cd48d9f outdated 
    1343+
1344+    if (!EvalScript(stack, scriptPubKey, flags, checker, SIGVERSION_WITNESS_V0, serror)) {
1345+        return false;
1346+    }
1347+
1348+    // Scripts inside witness implicitly require cleanstack behaviour
    instagibbs commented at 9:36 am on May 21, 2016:

    Phrasing bothering me. The cleanstackness is implicit(not using the flag) but it requiring some new behavior is surely explicit here. Perhaps rewrite as:

    // Scripts inside witness require implicit cleanstack behavior as a consensus rule.

  154. in src/script/interpreter.cpp: in 5b0cd48d9f outdated 
    1394+                return set_error(serror, SCRIPT_ERR_WITNESS_MALLEATED);
1395+            }
1396+            if (!VerifyWitnessProgram(*witness, witnessversion, witnessprogram, flags, checker, serror)) {
1397+                return false;
1398+            }
1399+            // Bypass the cleanstack check at the end. The actual stack is obviously not clean
    instagibbs commented at 9:49 am on May 21, 2016:
    s/actual/base/ ?
  155. in src/script/interpreter.cpp: in 5b0cd48d9f outdated 
    1437+                    return set_error(serror, SCRIPT_ERR_WITNESS_MALLEATED_P2SH);
1438+                }
1439+                if (!VerifyWitnessProgram(*witness, witnessversion, witnessprogram, flags, checker, serror)) {
1440+                    return false;
1441+                }
1442+                // Bypass the cleanstack check at the end. The actual stack is obviously not clean
    instagibbs commented at 9:51 am on May 21, 2016:
    s/actual/base/
  156. in src/primitives/block.cpp: in 5b0cd48d9f outdated 
    30@@ -31,3 +31,12 @@ std::string CBlock::ToString() const
31     }
32     return s.str();
33 }
34+
35+int64_t GetBlockCost(const CBlock& block)
36+{
37+    // This implements the cost = (stripped_size * 4) + witness_size formula,
    instagibbs commented at 2:36 pm on May 21, 2016:
    nit: This comment would also be useful in GetTransactionCost, since the reasoning appears identical.
  157. in src/rpc/mining.cpp: in cb9d4d34ca outdated 
    511@@ -511,7 +512,7 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp)
512     UniValue transactions(UniValue::VARR);
513     map<uint256, int64_t> setTxIndex;
514     int i = 0;
515-    BOOST_FOREACH (const CTransaction& tx, pblock->vtx) {
516+    BOOST_FOREACH (CTransaction& tx, pblock->vtx) {
    jonasschnelli commented at 9:31 am on May 22, 2016:
    Why can’t this still be const?
    sipa commented at 9:42 am on May 22, 2016:
    No clue!
    dcousens commented at 10:53 am on May 22, 2016:

    No clue!

    As in, compiler error or is this a mistaken edit?

    earonesty commented at 3:39 pm on May 31, 2016:
    must be a mistaken edit: all references in that loop are const
  158. in qa/rpc-tests/p2p-segwit.py: in 5b0cd48d9f outdated 
    257+        # until the future, making synchronization here difficult.
258+        #assert_equal(self.test_node.last_reject.reason, "unexpected-witness")
259+
260+        # But it should not be permanently marked bad...
261+        # Resend without witness information.
262+        self.test_node.send_message(msg_block(block))
    instagibbs commented at 9:49 am on May 22, 2016:
    self.test_node_test_witness_block with withWitness=False will do this fine
  159. in src/policy/policy.h: in 1e9cba2873 outdated 
    18@@ -19,6 +19,8 @@ static const unsigned int DEFAULT_BLOCK_MAX_SIZE = 750000;
19 static const unsigned int DEFAULT_BLOCK_MIN_SIZE = 0;
20 /** Default for -blockprioritysize, maximum space for zero/low-fee transactions **/
21 static const unsigned int DEFAULT_BLOCK_PRIORITY_SIZE = 0;
22+/** Default for -blockmaxcost, which control the range of block costs the mining code will create **/
23+static const unsigned int DEFAULT_BLOCK_MAX_COST = 3000000;
    sdaftuar commented at 2:51 pm on May 24, 2016:

    I suggest we change this to 4000000 so that pre-activation, miners who are setting only -blockmaxsize don’t see a potential behavior change.

    FYI this is what is currently breaking qa/rpc-tests/pruning.py.

    sdaftuar commented at 4:49 pm on June 6, 2016:
    @sipa I think this still needs to be addressed?
  160. in qa/rpc-tests/p2p-segwit.py: in 5b0cd48d9f outdated 
    581+        block.solve()
582+
583+        self.test_node.test_witness_block(block, accepted=True)
584+
585+        # Now try extra witness/signature data on an input that DOES require a
586+        # witness
    instagibbs commented at 2:03 pm on May 25, 2016:
    nit: mention that this violates consensus cleanstack
  161. in qa/rpc-tests/p2p-segwit.py: in 5b0cd48d9f outdated 
    854+
855+
856+    # After segwit activates, verify that mempool:
857+    # - rejects transactions with unnecessary/extra witnesses
858+    # - accepts transactions with valid witnesses
859+    # and that witness transactions are relayed to non-upgraded peers.
    instagibbs commented at 2:38 pm on May 25, 2016:
    only the announcements are relayed in this code?
    sdaftuar commented at 2:42 pm on May 25, 2016:
    Good observation; we should verify that sending the two different kinds of getdata give the expected results.
  162. in qa/rpc-tests/p2p-segwit.py: in 5b0cd48d9f outdated 
    1105+        self.update_witness_block_with_transactions(block2, [spend_tx])
1106+        self.test_node.test_witness_block(block2, accepted=True)
1107+        sync_blocks(self.nodes)
1108+
1109+
1110+    def test_signature_version_1(self):
    instagibbs commented at 5:17 pm on May 25, 2016:
    Version 0 (and all references below)
    sdaftuar commented at 5:48 pm on May 25, 2016:
    For clarity, we should probably change this to SIGVERSION_WITNESS_V0 to match the enum that was added to the code, but that enum has value 1 (which is where “signature version 1” comes from).
  163. instagibbs commented at 5:35 pm on May 25, 2016: member
    nits aside, p2p-segwit.py code review ACK https://github.com/bitcoin/bitcoin/pull/7910/commits/5b0cd48d9f29f4a839640819230d4e88e077ce40
  164. --- [SEGWIT] begin: fixups 5 --- 306858f70f
  165. Improve FindForkInGlobalIndex when locator contains chain tip 036fa47b7d
  166. VerifyDB: don't check blocks that have been pruned 7eb0d75a75
  167. Improve RewindBlockIndex when pruning 
    Pruning nodes that upgrade after segwit activation may not HAVE_DATA for all
blocks back to the activation point.  Since pruning nodes won't be serving such
blocks to peers anyway, don't try to rewind past the HAVE_DATA point, as that
would fail and force pruning nodes to redownload the blockchain.
    8adb03a24d
  168. Make sure upgraded nodes don't ask for non-wit blocks a1d1d0ce40
  169. script_tests: witness tests can specify tx amount 
    Add tests that witness signatures cover value
    019860e835
  170. [Qt] Add support for NODE_WITNESS in formatServicesStr c815c1604e
  171. bitcoinconsensus.h: Accept amount as int64_t f16067fa80
  172. Add GetTransactionSigOpCost unit tests 059d4d1757
  173. segwit: fix gui wallet send transaction size calculation assertion failed c1c38a2088
  174. segwit: txout dust threshold calculation update 0acd1dc449
  175. in qa/rpc-tests/p2p-segwit.py: in 5b0cd48d9f outdated 
    1443+
1444+        tx2 = CTransaction()
1445+        # If we try to spend the first n-1 outputs from tx, that should be
1446+        # too many sigops.
1447+        total_value = 0
1448+
    instagibbs commented at 7:04 pm on May 25, 2016:
    Appears unused
  176. sipa force-pushed on May 31, 2016
  177. --- [SEGWIT] begin: fixups 6 --- 869f26e026
  178. Extend the max witness program length to 40 bytes 
    Amended by Pieter Wuille (max limit 40 instead of 33).
    14d4d1d23a
  179. in qa/rpc-tests/maxuploadtarget.py: in e82f2fc388 outdated 
    174@@ -175,13 +175,13 @@ def run_test(self):
175         getdata_request = msg_getdata()
176         getdata_request.inv.append(CInv(2, big_old_block))
177 
178-        max_bytes_per_day = 200*1024*1024
179-        daily_buffer = 144 * MAX_BLOCK_SIZE
180+        max_bytes_per_day = 800*1024*1024
181+        daily_buffer = 144 * 4000000
    JDutil commented at 7:09 am on June 3, 2016:
    Why is the max arbitrarily set here?
    sipa commented at 7:16 am on June 3, 2016:
    This is in test code. It has to test a specific scenario.
    MarcoFalke commented at 7:20 am on June 3, 2016:
    Though, it could help to name constants that have more than 4 trailing zeros and put them in test_framework/ e.g. /mininode.py.
    JDutil commented at 7:25 am on June 3, 2016:
    Okay so this is test code, but the test code used to be against the MAX_BLOCK_SIZE why is the max block size no longer important? Sorry if I sound stupid here being ruby dev not python, but I’d like to understand this seemingly arbitrary change that IMHO is way worse than being arbitrary it is changing the TEST ITSELF.
    sipa commented at 10:05 am on June 4, 2016:
    Oh, I misunderstood your comment. Sure, I should add a constant for the new max block size on the python side.
    JDutil commented at 10:12 am on June 4, 2016:
    @sipa 👍 on a new constant to make things readable, but sorry I’m just starting to learn the source code here so I still have simple questions. Is the reason your setting this different than the MAX_BLOCK_SIZE because segwit is supposed to help double the amount of data per block? I would think this would be 4MB not 4_000_000 which is close… Makes me think the test would be MAX_BLOCK_SIZE * 2. My main misunderstanding is why this value is changing from max constant when the test appears to be testing mining full blocks… Hopefully that helps clear up my confusion.
  180. sipa force-pushed on Jun 3, 2016
  181. sipa force-pushed on Jun 3, 2016
  182. Prevent witness addresses from being constructed before fork 4840f6dc83
  183. Remove positive SERIALIZE_TRANSACTION_WITNESS flag 3dbf852286
  184. sipa force-pushed on Jun 4, 2016
  185. sipa commented at 8:51 pm on June 4, 2016: member

    I’ve created a rebased/squash branch against master, whose tip tree is identical to the tree in this PR: https://github.com/sipa/bitcoin/compare/segwit-master2-base...segwit-master2

    EDIT: moved to a separate PR #8149.

  186. in src/test/sigopcount_tests.cpp: in 3cb46c1a4a outdated 
    71+ */
72+ScriptError VerifyWithFlag(const CTransaction& output, const CMutableTransaction& input, int flags)
73+{
74+    ScriptError error;
75+    CTransaction inputi(input);
76+    bool ret = VerifyScript(inputi.vin[0].scriptSig, output.vout[0].scriptPubKey, inputi.wit.vtxinwit.size() > 0 ? &inputi.wit.vtxinwit[0].scriptWitness : NULL, flags, TransactionSignatureChecker(&inputi, 0, output.vout[0].nValue), &error);
    paveljanik commented at 8:14 am on June 7, 2016:
    ret is unused here.
  187. sdaftuar commented at 7:32 pm on June 7, 2016: member

    As brought up in https://github.com/sipa/bitcoin/issues/95 (and subsequent IRC discussion), we currently will respond to a MSG_FILTERED_BLOCK with transactions that will be serialized with witness, even though the peer may not support it.

    We should change this to returning transactions without witness, and in the future we could add support for filtered block requests to indicate whether responses should include witnesses or not (eg by setting MSG_FILTERED_BLOCK | MSG_WITNESS_FLAG in the getdata request).

    Mental note: look into adding a p2p test to cover filtered blocks.

  188. --- [SEGWIT] begin: fixups 7 --- f98de5fe5d
  189. Actually count the witness data in memusage of CTransaction c06c40b152
  190. Correctly count maximum size in mining d8b5db9449
  191. Rework -maxblocksize and -maxblockcost 57d4bd232f
  192. Cache transaction cost in mempool 0bfbf60b47
  193. Delete segnet 496d8c0d51
  194. Do not send witnesses in response to bip37 blocks 7799a7c2d4
  195. 33 to 40 bytes push should now be considered a witness scriptPubKey 4c19c18ecb
  196. sipa force-pushed on Jun 12, 2016
  197. sipa commented at 8:31 pm on June 12, 2016: member

    Updates:

    • Removed segnet
    • Changes meaning of -maxblocksize and -maxblockcost: if only of one them is given, that becomes the only constraint
    • Avoid recomputing transaction sizes/costs in mining code
    • Bugfix: count serialized bytes rather than vsize for -maxblockcost
    • Bugfix: don’t send witnesses in response to getdata MSG_FILTERED_BLOCK
    • New merge with master (on top of the mempool/relay refactors, and bip9/bip145 changes)

    #8149 has been updated with a rebase/squash of the above.

  198. --- [SEGWIT] begin: fixups 8 --- 7613bbbd3c
  199. Tests: add getblocktemplate/segwit test 
    Ensure that we set the versionbit for segwit before lockin
iff the client has indicated segwit-readiness.
    a9bff09d1d
  200. Add test for getrawtransaction 92ab64c510
  201. p2p-segwit.py: more RPC coverage 40f782975e
  202. Rename deployment (witness -> segwit) b6443393fd
  203. Update p2p-segwit.py for new deployment name 00bf5e1d68
  204. BIP143 P2WSH examples c8f2fb2c0b
  205. Fix unused variable in sigopcount test f3a7ed4978
  206. sipa force-pushed on Jun 13, 2016
  207. sipa commented at 8:28 pm on June 13, 2016: member

    Updates:

    • Removed commentary
    • Fix inconsistent naming ‘witness’ vs ‘segwit’
    • Added more tests by @jl2012 and @sdaftuar
    • New merge with master (on top of #7598 and #7749).

    There have been significant changes in master since this PR was branched off, and as a result, many things are now hidden under the merge commit here (including adapting for the tx relay/mempool changes from #8082 #8080 #8059 #7840 , the shared_ptr changed from #8126, the new CNB code from #7598, and the GBT changes from #7935). Especially for the new CNB code, it is probably easier to review the relevant commit from #8149 (which is rebased and squashed and completely identical to this PR).

    I would like to see some final reviews.

  208. in src/primitives/block.cpp: in a29e8991ad outdated 
    36+{
37+    // This implements the cost = (stripped_size * 4) + witness_size formula,
38+    // using only serialization with and without witness data. As witness_size
39+    // is equal to total_size - stripped_size, this formula is identical to:
40+    // cost = (stripped_size * 3) + total_size.
41+    return ::GetSerializeSize(block, SER_NETWORK, SERIALIZE_TRANSACTION_NO_WITNESS) * (WITNESS_SCALE_FACTOR - 1) + ::GetSerializeSize(block, SER_NETWORK, PROTOCOL_VERSION);
    sdaftuar commented at 1:19 am on June 14, 2016:

    Shouldn’t this first call be ::GetSerializeSize(block, SER_NETWORK, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS)? I guess PROTOCOL_VERSION isn’t actually used, but I assume we should be consistent…

    EDIT: maybe I’m wrong? I see both in the code actually…

  209. sipa commented at 7:19 am on June 14, 2016: member

    PROTOCOL_VERSION is indeed unused, so it doesn’t matter. I’ll change it for consistency.

    Longer term, I think the nSerType/nVersion should be replaced by a context object that encapsulates serialization parameters (for example, there could be a TransactionSerializationContext with a boolean fWitness, and a BlockSerializationContext that inherits from TransactionSerializationContext etc).

  210. kanzure commented at 2:29 pm on June 14, 2016: contributor
    ACK 3cb46c1. ACK 17389dc. ACK #8149 having same git tree hash for 3cb46c1 and 17389dc. Also, other reviewers might benefit from some segwit code review notes from https://bitcoincore.org/logs/2016-05-zurich-meeting-notes.html although I believe code review is possible in absence of looking at those notes.
  211. in qa/rpc-tests/test_framework/mininode.py: in a29e8991ad outdated 
    1350     MAGIC_BYTES = {
1351         "mainnet": b"\xf9\xbe\xb4\xd9",   # mainnet
1352         "testnet3": b"\x0b\x11\x09\x07",  # testnet3
1353-        "regtest": b"\xfa\xbf\xb5\xda"    # regtest
1354+        "regtest": b"\xfa\xbf\xb5\xda",   # regtest
1355+        "segnet": b"\x2e\x96\xea\xca"     # segnet
    instagibbs commented at 3:52 pm on June 14, 2016:
    nit: Vestigial segnet reference
    sipa commented at 3:55 pm on June 14, 2016:
    Please send a small PR so this is not forgotten.
    instagibbs commented at 3:59 pm on June 14, 2016:
    done
  212. instagibbs commented at 3:52 pm on June 14, 2016: member
    tACK wallet section plus fixups https://github.com/sipa/bitcoin/compare/3f989b9...be976b7 tACK p2p-segwit.py tests plus fixups https://github.com/bitcoin/bitcoin/commit/da0c46e1bbee0413bb5fb107c21a31fd4bbe1f42 code review ACK for the rest minus unit tests through beginning of merge section: https://github.com/bitcoin/bitcoin/pull/7910/commits/2311cf6ab879b352da39d89b959c735d06cbbf59
  213. jl2012 commented at 6:49 pm on June 14, 2016: contributor
    tACK 2311cf6 : consensus behavior matches descriptions in BIP141 and BIP143
  214. --- [SEGWIT] begin: fixups 9 --- 1b9893fb55
  215. DEPLOYMENT_WITNESS -> DEPLOYMENT_SEGWIT 3483e5cfdc
  216. Behave as a non-witness node when start time is far away 2a6516fcf7
  217. test: BIP143 examples fix and clarify c7b5de55b1
  218. Remove segnet from mininode cc19adc86b
  219. Tests: ensure that signrawtransaction failures are caught in segwit.py efc251d1f3
  220. spelling fix: uncommited -> uncommitted 396f4b8828
  221. sipa force-pushed on Jun 16, 2016
  222. sipa commented at 0:58 am on June 16, 2016: member

    A few more changes:

    • Spelling and naming consistency nits (by @sdaftuar and @instagibbs)
    • Test improvements (by @jl2012 and @sdaftuar)
    • Make a node fully behave as a non-witness node as long as there is no softfork defined
  223. --- [SEGWIT] begin: fixups 10 --- b508f5bde0
  224. Fix direct fetching of blocks after 2a6516fc 10433cfde7
  225. Consistency in serialization flags 2948c022d4
  226. sipa force-pushed on Jun 16, 2016
  227. --- [SEGWIT] begin: fixups 11 --- d7fe873677
  228. Don't treat NODE_WITNESS as relevant before a fork is defined 0e177a2577
  229. Revert "Don't check the genesis block PoW" as segnet has been dropped. c7795ee6db
  230. sipa force-pushed on Jun 16, 2016
  231. sipa force-pushed on Jun 16, 2016
  232. sipa commented at 6:35 pm on June 16, 2016: member
    New merge on top of #7600.
  233. in src/wallet/rpcwallet.cpp: in e84733733d outdated 
    1070+        throw runtime_error(msg);
1071+    }
1072+
1073+    {
1074+        LOCK(cs_main);
1075+        if (!IsWitnessEnabled(chainActive.Tip(), Params().GetConsensus()) && !GetBoolArg("-walletprematurewitness", false)) {
    instagibbs commented at 8:31 pm on June 16, 2016:
    Do we want to do a similar thing for createwitnessaddress?
  234. in src/script/sign.cpp: in e84733733d outdated 
    186+    }
187+    else if (solved && whichType == TX_WITNESS_V0_SCRIPTHASH)
188+    {
189+        CScript witnessscript(result[0].begin(), result[0].end());
190+        txnouttype subType;
191+        solved = solved && SignStep(creator, witnessscript, result, subType, SIGVERSION_WITNESS_V0) && subType != TX_SCRIPTHASH && subType != TX_WITNESS_V0_SCRIPTHASH && subType != TX_WITNESS_V0_KEYHASH;
    dcousens commented at 0:23 am on June 17, 2016:
    Rather than reassign solved, maybe just return false? Why are we waiting until the end of the scope?
    sipa commented at 5:00 pm on June 17, 2016:
    We want to push the potentially partial result to sigdata.scriptSig, which is done in line 179-182.
  235. sdaftuar commented at 5:04 am on June 17, 2016: member

    ACK e84733733d0ec3deee3b0d1cdcf643082b78ee1e

    (Will ACK in #8149 as well)

  236. --- [SEGWIT] begin: merge --- c708c3a4cf
  237. Merge remote-tracking branch 'upstream/master' into segwit-master c4e3c755d7
  238. in src/main.cpp: in e84733733d outdated 
    2429+        // * legacy (always)
2430+        // * p2sh (when P2SH enabled in flags and excludes coinbase)
2431+        // * witness (when witness enabled in flags and excludes coinbase)
2432+        nSigOpsCost += GetTransactionSigOpCost(tx, view, flags);
2433+        if (nSigOpsCost > MAX_BLOCK_SIGOPS_COST)
2434+            return state.DoS(100, error("ConnectBlock(): too many sigops"),
    instagibbs commented at 1:15 am on June 18, 2016:
    Upgraded nodes don’t ask for blocks from non-upgraded?
    kazcw commented at 1:29 am on June 18, 2016:
    Oh, right. Nevermind..
  239. sipa force-pushed on Jun 22, 2016
  240. sipa commented at 2:13 pm on June 22, 2016: member
    New merge after #8068 and #8179. See the details in #8149 (comment).
  241. NicolasDorier commented at 8:20 pm on June 23, 2016: contributor
    I udpated the sig cache PR (https://github.com/sipa/bitcoin/pull/101)
  242. sipa commented at 4:20 pm on June 24, 2016: member
    Merged as #8149.
  243. sipa closed this on Jun 24, 2016
  244. zkbot referenced this in commit 1d7ed06174 on Aug 13, 2021
  245. zkbot referenced this in commit 56b5f95897 on Aug 17, 2021
  246. MarcoFalke locked this on Sep 8, 2021


sipa instagibbs maaku morcos ghost gubatron LongShao007 paveljanik MarcoFalke theuni SergioDemianLerner jl2012 dcousens dooglus kanzure btcdrak sdaftuar NicolasDorier jonasnick jtimon wallclockbuilder mruddy afk11 nicola rebroad jonasschnelli earonesty JDutil kazcw

Labels
Consensus

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-12-22 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me