Segregated witness #7910

Closed
wants to merge 128 commits into
from

Projects

Done in Segwit

@sipa
Member
sipa commented Apr 19, 2016 edited

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 bitcoin/bips#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 sipa#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).

@sipa
Member
sipa commented Apr 19, 2016 edited

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

  • preparation (sipa@fb0ac48...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 (sipa@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 (sipa@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 (sipa@351a2fe...a2d6f8a)
    • abbe085 Witness script signing
    • c3fe53f Add witness address RPCs (using P2SH)
    • ec385fa signrawtransaction can sign P2WSH
  • tests (sipa@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 (sipa@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 (sipa@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 (sipa@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 (sipa@8708de8...306858f)
  • fixups 5 (sipa@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 (sipa@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 (sipa@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 (sipa@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 (sipa@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 (sipa@b508f5b...a6840e5)
  • fixups 11 (sipa@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 (sipa@fb348c6...e847337)
    • e847337 Merge remote-tracking branch 'upstream/master' into segwit-master

Code to generate this list:

PREV="$(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
sipa and others added some commits Dec 31, 2015
@sipa sipa Create segnet4 70ebe86
@sipa sipa Add segregated witness transaction serialization
Contains refactorings by Eric Lombrozo.
Contains fixup by Nicolas Dorier.
3b1ff49
@theuni @sipa theuni qt: Work (don't crash) with -segnet 58847fe
@sipa sipa Add segnet seed nodes f8bcd86
@sipa sipa --- [SEGWIT] begin: P2P/node/consensus --- bb4bb47
@sipa sipa Witness commitment validation
Includes a fix by Suhas Daftuar
cb9d4d3
@sipa sipa Script validation logic for witnesses 2c87be1
@sipa sipa Introduce and preferentially peer with NODE_WITNESS service bit
Service bit logic by Nicolas Dorier.
ac6886d
@sipa sipa Enable SCRIPT_VERIFY_WITNESS for mempool transactions 1e47805
@sipa sipa Activate script consensus rules through BIP9 97f34f7
@sipa sipa Add signature version 1 with updated sighash
Includes simplifications by Eric Lombrozo.
d374139
@sipa sipa Only download blocks from witness peers after fork 6a8021f
@CodeShark @sipa CodeShark Removed ppszTypeName from protocol.cpp e35d020
@CodeShark @sipa CodeShark getdata enum issue fix adb1c09
@sipa sipa Observe input amounts: verification 271e45e
@sipa sipa Implement block size/sigop cost rules, limits, and GBT support
Includes changes by Suhas Daftuar and Luke-jr.
3955218
@jl2012 @sipa jl2012 Return witness data
Includes RPC field name changes by Luke-jr.
92c6dc3
@morcos @sipa morcos Add command line options to loosen mempool acceptance rules 5a47b98
@sdaftuar @sipa sdaftuar Increase MAX_PROTOCOL_MESSAGE_LENGTH
Witness blocks can be greater than 2MiB, but cannot be validly greater
than 4MB.
a6386c2
@afk11 @sipa afk11 bitcoinconsensus: add method that accepts amount, and return error wh…
…en 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
e573571
@sipa sipa Add rewind logic to deal with post-fork software updates 1b6c6f1
@sipa sipa --- [SEGWIT] begin: wallet --- 351a2fe
@sipa sipa Witness script signing abbe085
@sipa sipa Add witness address RPCs (using P2SH)
Includes support for pushkeyhash wit v0 by Alex Morcos.
c3fe53f
@sipa sipa Signing tests 2ed1d11
@sipa sipa --- [SEGWIT] begin: tests --- a2d6f8a
@NicolasDorier @sipa NicolasDorier signrawtransaction can sign P2WSH ec385fa
@morcos @sipa morcos Add rpc test for segwit
Amended by Pieter Wuille to use multisig 1-of-1 for P2WSH tests, and BIP9
based switchover logic.
3c0b16f
@NicolasDorier @sipa NicolasDorier Add transaction tests for segwit
...with the four types of segwit payment, as well as all sighash combinaisons.
24746d2
@sipa sipa Add segwit support to script_tests 3409ad6
@sipa sipa Autogeneration support for witness in script_tests e5ffcc1
@sdaftuar @sipa sdaftuar 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.
5e6abe5
@sdaftuar @sipa sdaftuar P2P test for segwit
Rebased by Pieter Wuille
7b539f9
@instagibbs instagibbs and 1 other commented on an outdated diff Apr 19, 2016
src/rpc/misc.cpp
+ "\nCreates a witness address for a particular script.\n"
+ "It returns a json object with the address and witness script.\n"
+
+ "\nArguments:\n"
+ "1. \"script\" (string, required) A hex encoded script\n"
+
+ "\nResult:\n"
+ "{\n"
+ " \"address\":\"multisigaddress\", (string) The value of the new address (P2SH of witness script).\n"
+ " \"witnessScript\":\"script\" (string) The string value of the hex-encoded witness script.\n"
+ "}\n"
+ ;
+ throw runtime_error(msg);
+ }
+
+ std::vector<unsigned char> code = ParseHex(params[0].get_str());
@instagibbs
instagibbs Apr 19, 2016 Contributor

Probably should throw an error if it's not hex. Currently accepts anything entered.

@sipa
sipa Apr 23, 2016 Member

Agree.

@maaku
Contributor
maaku commented Apr 19, 2016 edited

Created pull request sipa#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.

@instagibbs instagibbs commented on the diff Apr 19, 2016
src/wallet/rpcwallet.cpp
+ std::vector<unsigned char> witprog;
+ if (subscript.IsWitnessProgram(witnessversion, witprog)) {
+ result = scriptID;
+ return true;
+ }
+ CScript witscript = GetScriptForWitness(subscript);
+ pwalletMain->AddCScript(witscript);
+ result = CScriptID(witscript);
+ return true;
+ }
+ return false;
+ }
+};
+
+UniValue addwitnessaddress(const UniValue& params, bool fHelp)
+{
@instagibbs
instagibbs Apr 19, 2016 Contributor

nit: ensure wallet is available for more useful error than "not known to wallet"

@sipa
sipa Apr 23, 2016 Member

Will do.

@morcos
Contributor
morcos commented Apr 19, 2016

See sipa#76 for fix to CInv::GetCommand

@21E14
Contributor
21E14 commented Apr 20, 2016

Ayayay... a third of the original client's source base. I assume the items left to do will be committed to this PR #7910 ?

@gubatron gubatron and 1 other commented on an outdated diff Apr 20, 2016
@@ -3301,6 +3347,68 @@ static bool CheckIndexAgainstCheckpoint(const CBlockIndex* pindexPrev, CValidati
return true;
}
+bool IsWitnessEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& params)
+{
+ LOCK(cs_main);
+ return (VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_WITNESS, versionbitscache) == THRESHOLD_ACTIVE);
+}
+
+void UpdateUncommitedBlockStructures(CBlock& block, const CBlockIndex* pindexPrev, const Consensus::Params& consensusParams)
+{
+ int commitpos = -1;
@gubatron
gubatron Apr 20, 2016 Contributor

this little commitpos check could use a refactor, it appears 3 times. #DRY.

@sipa
sipa Apr 20, 2016 Member

Agree!

@LongShao007
Contributor
LongShao007 commented Apr 20, 2016 edited

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

@paveljanik
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.

@MarcoFalke
Member

Jup, since we are aware of the issue, this might be a good time to disable this specific test.

@theuni
Member
theuni commented Apr 20, 2016

@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.

@sipa
Member
sipa commented Apr 20, 2016

@theuni 3rd retry of Travis, and now it succeeded. Perhaps just bad luck...

@SergioDemianLerner SergioDemianLerner and 3 others commented on an outdated diff Apr 20, 2016
@@ -1144,8 +1168,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
if (fRequireStandard && !AreInputsStandard(tx, view))
return state.Invalid(false, REJECT_NONSTANDARD, "bad-txns-nonstandard-inputs");
- unsigned int nSigOps = GetLegacySigOpCount(tx);
- nSigOps += GetP2SHSigOpCount(tx, view);
+ unsigned int nSigOpsCost = GetTransactionSigOpCost(tx, view, STANDARD_SCRIPT_VERIFY_FLAGS);
@SergioDemianLerner
SergioDemianLerner Apr 20, 2016 Contributor

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
jl2012 Apr 20, 2016 Member

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
sipa Apr 20, 2016 Member

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
dcousens Apr 21, 2016 Contributor

ACK switch to uint64_t, its what is returned by GetTransactionSigOpCost any way... explicit casts should be the rule, not the exception (IMHO).

@SergioDemianLerner SergioDemianLerner and 1 other commented on an outdated diff Apr 20, 2016
@@ -2326,7 +2369,7 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
std::vector<int> prevheights;
CAmount nFees = 0;
int nInputs = 0;
- unsigned int nSigOps = 0;
+ unsigned int nSigOpsCost = 0;
@SergioDemianLerner
SergioDemianLerner Apr 20, 2016 Contributor

same here. nSigOps -> int64_t

@sipa
sipa Apr 23, 2016 Member

Thanks, will do.

@SergioDemianLerner SergioDemianLerner and 2 others commented on an outdated diff Apr 20, 2016
src/primitives/block.cpp
@@ -31,3 +31,11 @@ std::string CBlock::ToString() const
}
return s.str();
}
+
+int64_t GetBlockCost(const CBlock& block)
+{
+ // The intended approximate formula is: cost = base_size * 4 + witness_size.
+ // We can only serialize base or base+witness, so the formula
+ // becomes: cost = base_size * 3 + total_size.
+ return ::GetSerializeSize(block, SER_NETWORK, 0) * (WITNESS_SCALE_FACTOR - 1) + ::GetSerializeSize(block, SER_NETWORK, SERIALIZE_TRANSACTION_WITNESS);
@SergioDemianLerner
SergioDemianLerner Apr 20, 2016 Contributor

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
sipa Apr 20, 2016 Member

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
dooglus Apr 21, 2016 Contributor

So just remove the word 'approximate'? I think it is helpful to keep the rest of the explanation here.

@sipa
sipa Apr 23, 2016 Member

Will do.

@dcousens dcousens and 1 other commented on an outdated diff Apr 21, 2016
src/consensus/consensus.h
@@ -6,10 +6,14 @@
#ifndef BITCOIN_CONSENSUS_CONSENSUS_H
#define BITCOIN_CONSENSUS_CONSENSUS_H
-/** The maximum allowed size for a serialized block, in bytes (network rule) */
-static const unsigned int MAX_BLOCK_SIZE = 1000000;
+/** The maximum allowed size for a serialized block, in bytes (only for buffer size limits) */
+static const unsigned int MAX_BLOCK_SERIALIZED_SIZE = 4000000;
+/** The maximum allowed cost for a block, see BIP 141 (network rule) */
@dcousens
dcousens Apr 21, 2016 Contributor

What was the reasoning behind separating this from being derived from the network rule?

@sipa
sipa Apr 23, 2016 Member

It is not derived from the network rule because it is the (only) network rule. What would you derive it from?

@dcousens
dcousens Apr 26, 2016 Contributor

All good, I forgot I made that comment before finishing reviewing the majority of the PR, it makes sense now 👍

@dcousens dcousens and 1 other commented on an outdated diff Apr 21, 2016
@@ -1033,6 +1052,11 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
return state.DoS(0, false, REJECT_NONSTANDARD, "premature-version2-tx");
}
+ // Don't accept witness transactions before the final threshold passes
@dcousens
dcousens Apr 21, 2016 edited Contributor

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
sipa Apr 23, 2016 Member

Will fix.

@dcousens dcousens commented on the diff Apr 21, 2016
src/main.cpp
@@ -1179,9 +1202,9 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
// itself can contain sigops MAX_STANDARD_TX_SIGOPS is less than
// MAX_BLOCK_SIGOPS; we still consider this an invalid rather than
// merely non-standard transaction.
- if ((nSigOps > MAX_STANDARD_TX_SIGOPS) || (nBytesPerSigOp && nSigOps > nSize / nBytesPerSigOp))
+ if ((nSigOpsCost > MAX_STANDARD_TX_SIGOPS_COST) || (nBytesPerSigOp && nSigOpsCost > nSize * WITNESS_SCALE_FACTOR / nBytesPerSigOp))
@dcousens
dcousens Apr 21, 2016 Contributor

Could this be bracketed such that precedence rules don't need to be remembered?

@sipa
sipa Apr 23, 2016 Member

I don't think that's necessary here.

@dcousens dcousens and 1 other commented on an outdated diff Apr 21, 2016
src/consensus/consensus.h
@@ -6,10 +6,14 @@
#ifndef BITCOIN_CONSENSUS_CONSENSUS_H
#define BITCOIN_CONSENSUS_CONSENSUS_H
-/** The maximum allowed size for a serialized block, in bytes (network rule) */
-static const unsigned int MAX_BLOCK_SIZE = 1000000;
+/** The maximum allowed size for a serialized block, in bytes (only for buffer size limits) */
+static const unsigned int MAX_BLOCK_SERIALIZED_SIZE = 4000000;
+/** The maximum allowed cost for a block, see BIP 141 (network rule) */
+static const unsigned int MAX_BLOCK_COST = 4000000;
@dcousens
dcousens Apr 21, 2016 Contributor

Less implicit casts if this is also uint64_t

@sipa
sipa Apr 23, 2016 Member

Will turn all sigops cost variables into int64_t

@dooglus dooglus and 1 other commented on an outdated diff Apr 21, 2016
+ uint256 hashWitness = BlockWitnessMerkleRoot(block, &malleated);
+ // The malleation check is ignored; as the transaction tree itself
+ // already does not permit it, it is impossible to trigger in the
+ // witness tree.
+ 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) {
+ return state.DoS(100, error("%s : invalid witness commitment size", __func__), REJECT_INVALID, "bad-witness-merkle-size", true);
+ }
+ CHash256().Write(hashWitness.begin(), 32).Write(&block.vtx[0].wit.vtxinwit[0].scriptWitness.stack[0][0], 32).Finalize(hashWitness.begin());
+ if (memcmp(hashWitness.begin(), &block.vtx[0].vout[commitpos].scriptPubKey[6], 32)) {
+ return state.DoS(100, error("%s : witness merkle commitment mismatch", __func__), REJECT_INVALID, "bad-witness-merkle-match", true);
+ }
+ fHaveWitness = true;
+ }
+ }
+
+ // No witness data is allowed in blocks that don't commit to witness data, as this would otherwise leave room from spam
@dooglus
dooglus Apr 21, 2016 Contributor

You mean "room for spam"?

@sipa
sipa Apr 23, 2016 Member

I do!

@kanzure kanzure and 1 other commented on an outdated diff Apr 21, 2016
@@ -3301,6 +3347,68 @@ static bool CheckIndexAgainstCheckpoint(const CBlockIndex* pindexPrev, CValidati
return true;
}
+bool IsWitnessEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& params)
+{
+ LOCK(cs_main);
+ return (VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_WITNESS, versionbitscache) == THRESHOLD_ACTIVE);
+}
+
+void UpdateUncommitedBlockStructures(CBlock& block, const CBlockIndex* pindexPrev, const Consensus::Params& consensusParams)
+{
+ int commitpos = -1;
+ for (size_t o = 0; o < block.vtx[0].vout.size(); o++) {
+ 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
kanzure Apr 21, 2016 Contributor

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

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) {
@sipa
sipa Apr 23, 2016 Member

Yes, will factor it out to a separate function.

@instagibbs

Why no const here? I'm sure there's a reason, but above comment contradicts this line.

Member
sipa replied Apr 21, 2016

The other items are const because they are required to be immutable (modifying them would invalidate the cached hash), the witness however does not affect the hash.

Contributor

Makes perfect sense, but wasn't completely obvious to me. nit:update comment to mention the cache invalidation point, and that wtxid isn't important here.

@instagibbs
Contributor

Why a vector wrapper and not a CScript?

Member
sipa replied Apr 21, 2016

Because it is not script, it's a list of byte vectors. It does not contain opcodes, it is just the list of data types that would be pushed.

@MarcoFalke MarcoFalke commented on an outdated diff Apr 22, 2016
@@ -2629,7 +2669,7 @@ void static UpdateTip(CBlockIndex *pindexNew) {
}
/** Disconnect chainActive's tip. You probably want to call mempool.removeForReorg and manually re-limit mempool size after this, with cs_main held. */
-bool static DisconnectTip(CValidationState& state, const Consensus::Params& consensusParams)
+bool static DisconnectTip(CValidationState& state, const Consensus::Params& consensusParams, bool fBare = false)
@MarcoFalke
MarcoFalke Apr 22, 2016 Member

Needs merge conflict solved.

@MarcoFalke MarcoFalke commented on an outdated diff Apr 22, 2016
qa/rpc-tests/test_framework/mininode.py
@@ -23,7 +23,7 @@
import time
import sys
import random
-from binascii import hexlify, unhexlify
+from util import *
@MarcoFalke
MarcoFalke Apr 22, 2016 Member

Nit: Let's not do wildcard imports in mininode.

@btcdrak btcdrak and 1 other commented on an outdated diff Apr 22, 2016
qa/rpc-tests/test_framework/blocktools.py
@@ -22,6 +22,29 @@ def create_block(hashprev, coinbase, nTime=None):
block.calc_sha256()
return block
+# From BIP141
+WITNESS_COMMITMENT_HEADER = "\xaa\x21\xa9\xed"
+
+# According to BIP141, nVersion=5 blocks must commit to the
@btcdrak
btcdrak Apr 22, 2016 Member

This comment doesn't look right, since we're using version bits deployment.

@sipa
sipa Apr 23, 2016 Member

Will fix.

@btcdrak btcdrak and 1 other commented on an outdated diff Apr 22, 2016
src/consensus/params.h
@@ -16,6 +16,7 @@ enum DeploymentPos
{
DEPLOYMENT_TESTDUMMY,
DEPLOYMENT_CSV, // Deployment of BIP68, BIP112, and BIP113.
+ DEPLOYMENT_WITNESS,
@btcdrak
btcdrak Apr 22, 2016 Member

Add comment Deployment of BIP141 and BIP143

@sipa
sipa Apr 23, 2016 Member

Will do.

@btcdrak btcdrak and 1 other commented on an outdated diff Apr 22, 2016
src/chainparams.cpp
@@ -92,6 +93,11 @@ class CMainParams : public CChainParams {
consensus.vDeployments[Consensus::DEPLOYMENT_CSV].nStartTime = 1462060800; // May 1st, 2016
consensus.vDeployments[Consensus::DEPLOYMENT_CSV].nTimeout = 1493596800; // May 1st, 2017
+ // Deployment of SegWit.
@btcdrak
btcdrak Apr 22, 2016 Member

add the BIP numbers being deployed.

Deployment of SegWit, BIP141 and BIP143

@sipa
sipa Apr 23, 2016 Member

Will fix.

@instagibbs instagibbs and 1 other commented on an outdated diff Apr 22, 2016
src/rpc/mining.cpp
@@ -330,13 +332,15 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp)
" \"transactions\" : [ (array) contents of non-coinbase transactions that should be included in the next block\n"
" {\n"
" \"data\" : \"xxxx\", (string) transaction data encoded in hexadecimal (byte-for-byte)\n"
- " \"hash\" : \"xxxx\", (string) hash/id encoded in little-endian hexadecimal\n"
+ " \"txid\" : \"xxxx\", (string) transaction id encoded in little-endian hexadecimal\n"
+ " \"hash\" : \"xxxx\", (string) hash encoded in little-endian hexadecimal\n"
@instagibbs
instagibbs Apr 22, 2016 Contributor

Rename to "witnesshash", or mention hash is based on witnesshash in description.

@sipa
sipa Apr 23, 2016 Member

This is from BIP145, though I agree it's helpful to document.

sipa and others added some commits Apr 23, 2016
@sipa sipa --- [SEGWIT] begin: fixups --- cfb0a83
@sipa sipa fixup Implement block size/sigop cost rules, limits, and GBT support:…
… use int64_t for sigopcost everywhere
bb2bfca
@sipa sipa fixup Implement block size/sigop cost rules, limits, and GBT support:…
… update block cost comment
64c1527
@sipa sipa fixup Add segregated witness transaction serialization: comments cc741f5
@sipa sipa fixup Update p2p test framework with segwit support: fix comment c28179f
@sipa sipa fixup Witness commitment validation: list the mentioned BIPs 6a5da9b
@sipa sipa fixup Add command line options to loosen mempool acceptance rules: si…
…mplify comment
88e0135
@sipa sipa fixup Witness commitment validation: factor out GetWitnessCommitmentPos da60fae
@sipa sipa fixup Witness commitment validation: fix typo in comment 4f827be
@sipa sipa fixup Implement block size/sigop cost rules, limits, and GBT support:…
… improve GBT help
058f495
@sipa sipa fixup Add witness address RPCs (using P2SH): test hex & wallet available c1e7a95
@sipa sipa fixup Add segregated witness transaction serialization: revert extfor…
…mat-if-empty-vin
dbe6391
@morcos @sipa morcos tidy up CInv::GetCommand 97d7402
@LongShao007 @sipa LongShao007 fixup Witness commitment validation: function UpdateUncommitedBlockSt…
…ructures redefine

function UpdateUncommitedBlockStructures defined twice.
f0f6123
@MarcoFalke @sipa MarcoFalke [qa] Use integer division, byte strings properly
Also refactor some code and add segwit.py to the pulltester
a613599
@sipa sipa fixup Add segregated witness transaction serialization: add missing w…
…itness flags
5d6b6e2
@sipa
Member
sipa commented Apr 23, 2016 edited

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.

@jl2012 jl2012 commented on an outdated diff Apr 25, 2016
@@ -3353,6 +3464,53 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIn
}
}
+ // Validation for witness commitments.
+ // * We compute the witness hash (which is the hash including witnesses) of all the block's transactions, except the
+ // coinbase (where 0x0000....0000 is used instead).
+ // * The coinbase scriptWitness is a stack of a single 32-byte vector, containing a witness nonce (unconstrained).
+ // * We build a merkle tree with all those witness hashes as leaves (similar to the hashMerkleRoot in the block header).
+ // * The must be at least one output whose scriptPubKey is a single 36-byte push, the first 4 bytes of which are
+ // {0xaa, 0x21, 0xa9, 0xed}, and the following 32 bytes are SHA256(witness root, witness nonce). In case there are
@jl2012
jl2012 Apr 25, 2016 Member

Double SHA256?

@instagibbs instagibbs commented on the diff Apr 25, 2016
src/main.cpp
// Check against previous transactions
// This is done last to help prevent CPU exhaustion denial-of-service attacks.
- if (!CheckInputs(tx, state, view, true, STANDARD_SCRIPT_VERIFY_FLAGS, true))
- return false; // state filled in by CheckInputs
+ if (!CheckInputs(tx, state, view, true, scriptVerifyFlags, true)) {
+ // SCRIPT_VERIFY_CLEANSTACK requires SCRIPT_VERIFY_WITNESS, so we
+ // need to turn both off, and compare against just turning off CLEANSTACK
+ // to see if the failure is specifically due to witness validation.
+ if (CheckInputs(tx, state, view, true, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true) &&
+ !CheckInputs(tx, state, view, true, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true)) {
+ // Only the witness is wrong, so the transaction itself may be fine.
@instagibbs
instagibbs Apr 25, 2016 edited Contributor

what does "may be fine" mean here?

edit: nevermind, I see the explanation here: 1e47805#diff-7ec3c68a81efff79b6ca22ac1f1eabbaR5221

@morcos
Contributor
morcos commented Apr 25, 2016

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.

@morcos morcos and 1 other commented on an outdated diff Apr 25, 2016
src/primitives/transaction.h
+ * - std::vector<CTxOut> vout
+ * - if (flags & 1):
+ * - CTxWitness wit;
+ * - uint32_t nLockTime
+ */
+template<typename Stream, typename Operation, typename TxType>
+inline void SerializeTransaction(TxType& tx, Stream& s, Operation ser_action, int nType, int nVersion) {
+ READWRITE(*const_cast<int32_t*>(&tx.nVersion));
+ unsigned char flags = 0;
+ if (ser_action.ForRead()) {
+ /* Try to read the vin. In case the dummy is there, this will be read as an empty vector. */
+ READWRITE(*const_cast<std::vector<CTxIn>*>(&tx.vin));
+ if (tx.vin.size() == 0 && (nVersion & SERIALIZE_TRANSACTION_WITNESS)) {
+ /* We read a dummy or an empty vin. */
+ READWRITE(flags);
+ if (flags != 0) {
@morcos
morcos Apr 25, 2016 Contributor

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
sipa Apr 25, 2016 edited Member

That is intentional. If a transaction starting with "[version] 0x00 0x00 ..." is seen, it is treated as one with no inputs and no outputs.

@morcos morcos commented on an outdated diff Apr 25, 2016
src/test/data/tx_invalid.json
@@ -32,7 +32,7 @@
["Tests for CheckTransaction()"],
["No inputs"],
[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "HASH160 0x14 0x7a052c840ba73af26755de42cf01cc9e0a49fef0 EQUAL"]],
-"0100000000010000000000000000015100000000", "P2SH"],
+"01000000000000010000000000000000015100000000", "P2SH"],
@morcos
morcos Apr 25, 2016 Contributor

I think after your fixup commit this doesn't deserialize properly, which probably isn't what we're supposed to be testing here.

@morcos morcos and 1 other commented on an outdated diff Apr 25, 2016
src/miner.cpp
@@ -183,6 +186,9 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const CScript& s
const CTransaction& tx = iter->GetTx();
+ if (!fIncludeWitness && !tx.wit.IsNull())
@morcos
morcos Apr 25, 2016 Contributor

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
sipa Apr 27, 2016 Member

Going to replace it with an extra delay before witness transactions are accepted to the mempool.

@morcos
morcos Apr 27, 2016 Contributor

@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

@sipa sipa commented on an outdated diff Apr 25, 2016
src/miner.cpp
@@ -183,6 +186,9 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const CScript& s
const CTransaction& tx = iter->GetTx();
+ if (!fIncludeWitness && !tx.wit.IsNull())
@sipa
sipa Apr 25, 2016 Member

Agree, this is a hack.

@instagibbs instagibbs commented on an outdated diff Apr 26, 2016
src/miner.cpp
// Largest block you're willing to create:
- unsigned int nBlockMaxSize = GetArg("-blockmaxsize", DEFAULT_BLOCK_MAX_SIZE);
+ unsigned int nBlockMaxCost = GetArg("-blockmaxsize", DEFAULT_BLOCK_MAX_SIZE) * WITNESS_SCALE_FACTOR;
// Limit to between 1K and MAX_BLOCK_SIZE-1K for sanity:
@instagibbs
instagibbs Apr 26, 2016 Contributor

nit: update comment to cost/4k

@morcos morcos commented on an outdated diff Apr 26, 2016
+ // * The coinbase scriptWitness is a stack of a single 32-byte vector, containing a witness nonce (unconstrained).
+ // * We build a merkle tree with all those witness hashes as leaves (similar to the hashMerkleRoot in the block header).
+ // * The must be at least one output whose scriptPubKey is a single 36-byte push, the first 4 bytes of which are
+ // {0xaa, 0x21, 0xa9, 0xed}, and the following 32 bytes are SHA256(witness root, witness nonce). In case there are
+ // multiple, the last one is used.
+ bool fHaveWitness = false;
+ if (IsWitnessEnabled(pindexPrev, consensusParams)) {
+ int commitpos = GetWitnessCommitmentIndex(block);
+ if (commitpos != -1) {
+ bool malleated = false;
+ uint256 hashWitness = BlockWitnessMerkleRoot(block, &malleated);
+ // The malleation check is ignored; as the transaction tree itself
+ // already does not permit it, it is impossible to trigger in the
+ // witness tree.
+ 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) {
+ return state.DoS(100, error("%s : invalid witness commitment size", __func__), REJECT_INVALID, "bad-witness-merkle-size", true);
@morcos
morcos Apr 26, 2016 Contributor

This error message might be clearer if it indicated that it was the "nonce" that was the wrong size.

@sdaftuar sdaftuar commented on the diff Apr 26, 2016
src/main.cpp
@@ -5176,8 +5185,10 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
if (nEvicted > 0)
LogPrint("mempool", "mapOrphan overflow, removed %u tx\n", nEvicted);
} else {
- assert(recentRejects);
- recentRejects->insert(tx.GetHash());
+ if (!state.CorruptionPossible()) {
+ assert(recentRejects);
+ recentRejects->insert(tx.GetHash());
@sdaftuar
sdaftuar Apr 26, 2016 Contributor

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
sipa Apr 27, 2016 Member

Indeed, and likewise for the misbehaviour assignment.

@NicolasDorier

A bug can be triggered by the caller:

You are checking if the witnesss need to be serialized on 3b1ff49#diff-5cb8d9decaa15620a8f98b0c6c44da9bR307

However, this resize can turn tx.wit.IsNull() to false, which mean the witness should not be serialized anymore.

Step to produce a bug:

  1. Create a CMutableTransaction with 5 inputs, last one with a witness.
  2. Remove the last input.
  3. Serialize
  4. Deserialization should crash on 3b1ff49#diff-5cb8d9decaa15620a8f98b0c6c44da9bR287

A way to fix this problem is to resize vtxinwit on 3b1ff49#diff-5cb8d9decaa15620a8f98b0c6c44da9bR306

Contributor

@NicolasDorier ah ok, i understand what you mean. yes i agree this should be cleaned up, we don't want to try to serialize a tx that is invalid (more witnesses than inputs) and have the serialization succeed but potentially have some later deserialization fail.
I'd suggest that just before this resize we add assert(tx.wit.vtxinwit.size() <= tx.vin.size()) as I assume this resize is just meant to pad extra empty witnesses if the last added inputs didn't have them and it should never be shrinking vtxinwit.

Member
NicolasDorier replied Apr 27, 2016 edited

I think an assert should be there indeed, but that also the resize should be done before the call to IsNull() so if the caller removed an input but forgot to remove the witness, it would not explose in his face.

Or maybe you are right and it is better it explodes as it could eventually lead to none obvious bug later...

The way I avoid such mistake in NBitcoin is by adding a new field Witness to TxIn. Then serialization code and deserialization code is responsible to write/read those TxIn's witnesses at the end of the transaction. (so a transaction in NBitcoin don't have a vtxinwit field exposed which can possibly be out of sync with TxIn)

I fear it is too late to implement such way to handle witnesses now, but if there is interest I can work on it.

@morcos morcos and 1 other commented on an outdated diff Apr 27, 2016
src/script/interpreter.cpp
{
+ if (sigversion == 1) {
@morcos
morcos Apr 27, 2016 edited Contributor

nit: would it be easier to read if any time we had a sigversion we used an enum?

@sipa
sipa Apr 27, 2016 Member

Oh yes!

@morcos morcos and 1 other commented on an outdated diff Apr 27, 2016
src/rpc/blockchain.cpp
@@ -331,6 +333,7 @@ UniValue getblockheader(const UniValue& params, bool fHelp)
" \"previousblockhash\" : \"hash\", (string) The hash of the previous block\n"
" \"nextblockhash\" : \"hash\", (string) The hash of the next block\n"
" \"chainwork\" : \"0000...1f3\" (string) Expected number of hashes required to produce the current chain (in hex)\n"
+ " \"nextblockhash\" : \"hash\" (string) The hash of the next block\n"
@morcos
morcos Apr 27, 2016 Contributor

maybe rebase error? nextblockhash is repeated. to keep the order the same as output, chainwork should go before previousblockhash and nextblockhash.

@instagibbs
instagibbs Apr 27, 2016 Contributor

the ordering has always been wrong: #7194

@sipa sipa 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.
f889bec
@sipa
Member
sipa commented May 15, 2016 edited

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?".

@mruddy mruddy referenced this pull request in sipa/bitcoin May 16, 2016
Closed

segwit: txout dust threshold calculation update #86

@theuni theuni and 1 other commented on an outdated diff May 20, 2016
src/script/bitcoinconsensus.cpp
} catch (const std::exception&) {
return set_error(err, bitcoinconsensus_ERR_TX_DESERIALIZE); // Error deserializing
}
}
+int bitcoinconsensus_verify_script_with_amount(const unsigned char *scriptPubKey, unsigned int scriptPubKeyLen, uint64_t amount,
+ const unsigned char *txTo , unsigned int txToLen,
+ unsigned int nIn, unsigned int flags, bitcoinconsensus_error* err)
+{
+ CAmount am(amount);
@theuni
theuni May 20, 2016 Member

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
afk11 May 20, 2016 Contributor

Or just accept int64_t, as amounts are never interpreted as unsigned..

@theuni
theuni May 20, 2016 Member

I assumed that (self-documentation) was the reason for the uint64_t.

@sdaftuar sdaftuar commented on an outdated diff May 20, 2016
src/test/script_tests.cpp
BOOST_CHECK_MESSAGE(err == scriptError, std::string(FormatScriptError(err)) + " where " + std::string(FormatScriptError((ScriptError_t)scriptError)) + " expected: " + message);
#if defined(HAVE_CONSENSUS_LIB)
CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
stream << tx2;
if (flags & bitcoinconsensus_SCRIPT_FLAGS_VERIFY_WITNESS) {
- 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);
+ 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
sdaftuar May 20, 2016 Contributor

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).

@nicola nicola commented on the diff May 20, 2016
qa/rpc-tests/maxuploadtarget.py
max_bytes_available = max_bytes_per_day - daily_buffer
success_count = max_bytes_available // old_block_size
- # 144MB will be reserved for relaying new blocks, so expect this to
- # succeed for ~70 tries.
+ # 576MB will be reserved for relaying new blocks, so expect this to
+ # succeed for ~235 tries.
for i in range(success_count):
@nicola
nicola May 20, 2016

do you mind explaining this calculation?

@rebroad
rebroad Aug 25, 2016 Contributor

at least 4 people so far waiting for an answer to this one, @sipa curious minds want to know!

@MarcoFalke
MarcoFalke Aug 25, 2016 Member

@rebroad and other 3: Explanation should be in OP of #8559

@jonasschnelli jonasschnelli commented on the diff May 21, 2016
src/bitcoin-tx.cpp
@@ -358,6 +358,18 @@ vector<unsigned char> ParseHexUO(map<string,UniValue>& o, string strKey)
return ParseHexUV(o[strKey], strKey);
}
+static CAmount AmountFromValue(const UniValue& value)
@jonasschnelli
jonasschnelli May 21, 2016 Member

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.

@sdaftuar sdaftuar commented on an outdated diff May 21, 2016
+}
+
+// Compute at which vout of the block's coinbase transaction the witness
+// commitment occurs, or -1 if not found.
+static int GetWitnessCommitmentIndex(const CBlock& block)
+{
+ int commitpos = -1;
+ for (size_t o = 0; o < block.vtx[0].vout.size(); o++) {
+ 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) {
+ commitpos = o;
+ }
+ }
+ return commitpos;
+}
+
+void UpdateUncommitedBlockStructures(CBlock& block, const CBlockIndex* pindexPrev, const Consensus::Params& consensusParams)
@sdaftuar
sdaftuar May 21, 2016 Contributor

nit: "Uncommited" -> "Uncommitted"

@jonasschnelli jonasschnelli commented on an outdated diff May 21, 2016
src/chainparamsbase.cpp
@@ -21,6 +22,7 @@ void AppendParamsHelpMessages(std::string& strUsage, bool debugHelp)
if (debugHelp) {
strUsage += HelpMessageOpt("-regtest", "Enter regression test mode, which uses a special chain in which blocks can be solved instantly. "
"This is intended for regression testing tools and app development.");
+ strUsage += HelpMessageOpt("-segnet", "Enter segregated witness test mode. ");
@jonasschnelli
jonasschnelli May 21, 2016 Member

nit: s/mode. ")/mode.")/

@sdaftuar sdaftuar commented on the diff May 21, 2016
src/consensus/consensus.h
@@ -6,10 +6,16 @@
#ifndef BITCOIN_CONSENSUS_CONSENSUS_H
#define BITCOIN_CONSENSUS_CONSENSUS_H
-/** The maximum allowed size for a serialized block, in bytes (network rule) */
-static const unsigned int MAX_BLOCK_SIZE = 1000000;
+#include <stdint.h>
+
+/** The maximum allowed size for a serialized block, in bytes (only for buffer size limits) */
+static const unsigned int MAX_BLOCK_SERIALIZED_SIZE = 4000000;
+/** The maximum allowed cost for a block, see BIP 141 (network rule) */
+static const unsigned int MAX_BLOCK_COST = 4000000;
@sdaftuar
sdaftuar May 21, 2016 Contributor

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
rebroad Aug 25, 2016 Contributor

Oooh! We've lost the 1MB block limit rule?! ;)

@instagibbs instagibbs commented on the diff May 21, 2016
src/script/standard.cpp
@@ -282,3 +300,26 @@ CScript GetScriptForMultisig(int nRequired, const std::vector<CPubKey>& keys)
script << CScript::EncodeOP_N(keys.size()) << OP_CHECKMULTISIG;
return script;
}
+
+CScript GetScriptForWitness(const CScript& redeemscript)
@instagibbs
instagibbs May 21, 2016 edited Contributor

I think "GetWitnessProgramForScript" is more direct and clear.

I had a misunderstanding of "witness program" definition somehow.

"GetScriptForWitnessScript" isn't much clearer either. Hm.

@instagibbs instagibbs commented on the diff May 21, 2016
src/script/interpreter.cpp
@@ -1239,28 +1299,106 @@ bool TransactionSignatureChecker::CheckSequence(const CScriptNum& nSequence) con
return true;
}
-bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* serror)
+static bool VerifyWitnessProgram(const CScriptWitness& witness, int witversion, const std::vector<unsigned char>& program, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* serror)
@instagibbs
instagibbs May 21, 2016 edited Contributor

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".

@instagibbs instagibbs commented on the diff May 21, 2016
src/script/interpreter.cpp
+ } else {
+ // Higher version witness scripts return true for future softfork compatibility
+ return set_success(serror);
+ }
+
+ // Disallow stack item size > MAX_SCRIPT_ELEMENT_SIZE in witness stack
+ for (unsigned int i = 0; i < stack.size(); i++) {
+ if (stack.at(i).size() > MAX_SCRIPT_ELEMENT_SIZE)
+ return set_error(serror, SCRIPT_ERR_PUSH_SIZE);
+ }
+
+ if (!EvalScript(stack, scriptPubKey, flags, checker, SIGVERSION_WITNESS_V0, serror)) {
+ return false;
+ }
+
+ // Scripts inside witness implicitly require cleanstack behaviour
@instagibbs
instagibbs May 21, 2016 Contributor

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.

@instagibbs instagibbs commented on the diff May 21, 2016
src/script/interpreter.cpp
// serror is set
return false;
if (stack.empty())
return set_error(serror, SCRIPT_ERR_EVAL_FALSE);
if (CastToBool(stack.back()) == false)
return set_error(serror, SCRIPT_ERR_EVAL_FALSE);
+ // Bare witness programs
+ int witnessversion;
+ std::vector<unsigned char> witnessprogram;
+ if (flags & SCRIPT_VERIFY_WITNESS) {
+ if (scriptPubKey.IsWitnessProgram(witnessversion, witnessprogram)) {
+ hadWitness = true;
+ if (scriptSig.size() != 0) {
+ // The scriptSig must be _exactly_ CScript(), otherwise we reintroduce malleability.
+ return set_error(serror, SCRIPT_ERR_WITNESS_MALLEATED);
+ }
+ if (!VerifyWitnessProgram(*witness, witnessversion, witnessprogram, flags, checker, serror)) {
+ return false;
+ }
+ // Bypass the cleanstack check at the end. The actual stack is obviously not clean
@instagibbs
instagibbs May 21, 2016 Contributor

s/actual/base/ ?

@instagibbs instagibbs commented on the diff May 21, 2016
src/script/interpreter.cpp
// serror is set
return false;
if (stack.empty())
return set_error(serror, SCRIPT_ERR_EVAL_FALSE);
if (!CastToBool(stack.back()))
return set_error(serror, SCRIPT_ERR_EVAL_FALSE);
+
+ // P2SH witness program
+ if (flags & SCRIPT_VERIFY_WITNESS) {
+ if (pubKey2.IsWitnessProgram(witnessversion, witnessprogram)) {
+ hadWitness = true;
+ if (scriptSig != CScript() << std::vector<unsigned char>(pubKey2.begin(), pubKey2.end())) {
+ // The scriptSig must be _exactly_ a single push of the redeemScript. Otherwise we
+ // reintroduce malleability.
+ return set_error(serror, SCRIPT_ERR_WITNESS_MALLEATED_P2SH);
+ }
+ if (!VerifyWitnessProgram(*witness, witnessversion, witnessprogram, flags, checker, serror)) {
+ return false;
+ }
+ // Bypass the cleanstack check at the end. The actual stack is obviously not clean
@instagibbs
instagibbs May 21, 2016 Contributor

s/actual/base/

@sdaftuar

This enum might be better named "ScriptVersion". This is passed to EvalScript, which in turn passes it on to the signature verification. But in the future to make upgrades to the script system with new witness versions, we'd be translating witness versions to SigVersion's in VerifyScript (and then switching on this value in EvalScript) which could have behavior outside of just changing signatures or signature hashing (eg turning on a new op code).

Not sure we need to do this in this PR, but if we don't, I'd suggest we add a comment to help understand how this should work in the future.

@sdaftuar
Contributor

nit: The help string here should indicate that the "hash" is the hash including witness. (also below at line 450 in the decoderawtransaction help)

@sdaftuar
Contributor

nit: return the block cost here as well

@sdaftuar
Contributor

nit: Perhaps add a comment pointing out that P2SH sig op counting and legacy sip op counting is done differently from CountWitnessSigOps for historical reasons.

@instagibbs instagibbs commented on the diff May 21, 2016
src/primitives/block.cpp
@@ -31,3 +31,12 @@ std::string CBlock::ToString() const
}
return s.str();
}
+
+int64_t GetBlockCost(const CBlock& block)
+{
+ // This implements the cost = (stripped_size * 4) + witness_size formula,
@instagibbs
instagibbs May 21, 2016 Contributor

nit: This comment would also be useful in GetTransactionCost, since the reasoning appears identical.

@sdaftuar
Contributor

We also need to do this above, in RecursiveDynamicUsage(CTransaction).

Contributor
sdaftuar replied Jun 6, 2016 edited

@sipa This is still outstanding.

@ongygy2000

NU STIU PRIETENE POATE MA INVETI TU
MULTUMESC

@jonasschnelli jonasschnelli commented on the diff May 22, 2016
src/rpc/mining.cpp
@@ -511,7 +512,7 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp)
UniValue transactions(UniValue::VARR);
map<uint256, int64_t> setTxIndex;
int i = 0;
- BOOST_FOREACH (const CTransaction& tx, pblock->vtx) {
+ BOOST_FOREACH (CTransaction& tx, pblock->vtx) {
@jonasschnelli
jonasschnelli May 22, 2016 Member

Why can't this still be const?

@sipa
sipa May 22, 2016 Member

No clue!

@dcousens
dcousens May 22, 2016 edited Contributor

No clue!

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

@earonesty
earonesty May 31, 2016

must be a mistaken edit: all references in that loop are const

@instagibbs instagibbs commented on the diff May 22, 2016
qa/rpc-tests/p2p-segwit.py
+ assert(tx.sha256 != tx.calc_sha256(with_witness=True))
+
+ # Construct a segwit-signaling block that includes the transaction.
+ block = self.build_next_block(nVersion=(VB_TOP_BITS|(1 << VB_WITNESS_BIT)))
+ self.update_witness_block_with_transactions(block, [tx])
+ # Sending witness data before activation is not allowed (anti-spam
+ # rule).
+ self.test_node.test_witness_block(block, accepted=False)
+ # TODO: fix synchronization so we can test reject reason
+ # Right now, bitcoind delays sending reject messages for blocks
+ # until the future, making synchronization here difficult.
+ #assert_equal(self.test_node.last_reject.reason, "unexpected-witness")
+
+ # But it should not be permanently marked bad...
+ # Resend without witness information.
+ self.test_node.send_message(msg_block(block))
@instagibbs
instagibbs May 22, 2016 Contributor

self.test_node_test_witness_block with withWitness=False will do this fine

@sdaftuar sdaftuar commented on the diff May 24, 2016
src/policy/policy.h
@@ -19,6 +19,8 @@ static const unsigned int DEFAULT_BLOCK_MAX_SIZE = 750000;
static const unsigned int DEFAULT_BLOCK_MIN_SIZE = 0;
/** Default for -blockprioritysize, maximum space for zero/low-fee transactions **/
static const unsigned int DEFAULT_BLOCK_PRIORITY_SIZE = 0;
+/** Default for -blockmaxcost, which control the range of block costs the mining code will create **/
+static const unsigned int DEFAULT_BLOCK_MAX_COST = 3000000;
@sdaftuar
sdaftuar May 24, 2016 Contributor

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
sdaftuar Jun 6, 2016 edited Contributor

@sipa I think this still needs to be addressed?

@instagibbs instagibbs commented on the diff May 25, 2016
qa/rpc-tests/p2p-segwit.py
+ self.update_witness_block_with_transactions(block, [tx])
+
+ # Extra witness data should not be allowed.
+ self.test_node.test_witness_block(block, accepted=False)
+
+ # Try extra signature data. Ok if we're not spending a witness output.
+ block.vtx[1].wit.vtxinwit = []
+ block.vtx[1].vin[0].scriptSig = CScript([OP_0])
+ block.vtx[1].rehash()
+ add_witness_commitment(block)
+ block.solve()
+
+ self.test_node.test_witness_block(block, accepted=True)
+
+ # Now try extra witness/signature data on an input that DOES require a
+ # witness
@instagibbs
instagibbs May 25, 2016 Contributor

nit: mention that this violates consensus cleanstack

@instagibbs instagibbs commented on the diff May 25, 2016
qa/rpc-tests/p2p-segwit.py
+ tx.rehash()
+ self.test_node.announce_tx_and_wait_for_getdata(tx)
+ assert(self.test_node.last_getdata.inv[0].type == 1|MSG_WITNESS_FLAG)
+
+ # Cleanup: mine the first transaction and update utxo
+ self.nodes[0].generate(1)
+ assert_equal(len(self.nodes[0].getrawmempool()), 0)
+
+ self.utxo.pop(0)
+ self.utxo.append(UTXO(tx_hash, 0, tx_value))
+
+
+ # After segwit activates, verify that mempool:
+ # - rejects transactions with unnecessary/extra witnesses
+ # - accepts transactions with valid witnesses
+ # and that witness transactions are relayed to non-upgraded peers.
@instagibbs
instagibbs May 25, 2016 Contributor

only the announcements are relayed in this code?

@sdaftuar
sdaftuar May 25, 2016 Contributor

Good observation; we should verify that sending the two different kinds of getdata give the expected results.

@instagibbs instagibbs commented on the diff May 25, 2016
qa/rpc-tests/p2p-segwit.py
+ # Now test a premature spend.
+ self.nodes[0].generate(98)
+ sync_blocks(self.nodes)
+ block2 = self.build_next_block()
+ self.update_witness_block_with_transactions(block2, [spend_tx])
+ self.test_node.test_witness_block(block2, accepted=False)
+
+ # Advancing one more block should allow the spend.
+ self.nodes[0].generate(1)
+ block2 = self.build_next_block()
+ self.update_witness_block_with_transactions(block2, [spend_tx])
+ self.test_node.test_witness_block(block2, accepted=True)
+ sync_blocks(self.nodes)
+
+
+ def test_signature_version_1(self):
@instagibbs
instagibbs May 25, 2016 edited Contributor

Version 0 (and all references below)

@sdaftuar
sdaftuar May 25, 2016 Contributor

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).

@instagibbs
Contributor

nits aside, p2p-segwit.py code review ACK 5b0cd48

@instagibbs instagibbs commented on an outdated diff May 25, 2016
qa/rpc-tests/p2p-segwit.py
+ tx.vin.append(CTxIn(COutPoint(self.utxo[0].sha256, self.utxo[0].n), b""))
+ for i in range(outputs):
+ tx.vout.append(CTxOut(split_value, scriptPubKey))
+ tx.vout[-2].scriptPubKey = scriptPubKey_toomany
+ tx.vout[-1].scriptPubKey = scriptPubKey_justright
+ tx.rehash()
+
+ block_1 = self.build_next_block()
+ self.update_witness_block_with_transactions(block_1, [tx])
+ self.test_node.test_witness_block(block_1, accepted=True)
+
+ tx2 = CTransaction()
+ # If we try to spend the first n-1 outputs from tx, that should be
+ # too many sigops.
+ total_value = 0
+ for i in range(outputs-1):
@instagibbs
instagibbs May 25, 2016 Contributor

Appears unused

@JDutil JDutil commented on the diff Jun 3, 2016
qa/rpc-tests/maxuploadtarget.py
@@ -175,13 +175,13 @@ def run_test(self):
getdata_request = msg_getdata()
getdata_request.inv.append(CInv(2, big_old_block))
- max_bytes_per_day = 200*1024*1024
- daily_buffer = 144 * MAX_BLOCK_SIZE
+ max_bytes_per_day = 800*1024*1024
+ daily_buffer = 144 * 4000000
@JDutil
JDutil Jun 3, 2016

Why is the max arbitrarily set here?

@sipa
sipa via email Jun 3, 2016 Member
@MarcoFalke
MarcoFalke via email Jun 3, 2016 Member
@JDutil
JDutil Jun 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
sipa Jun 4, 2016 Member

Oh, I misunderstood your comment. Sure, I should add a constant for the new max block size on the python side.

@JDutil
JDutil Jun 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.

@sipa
Member
sipa commented Jun 4, 2016 edited

I've created a rebased/squash branch against master, whose tip tree is identical to the tree in this PR: sipa@segwit-master2-base...segwit-master2

EDIT: moved to a separate PR #8149.

@sipa sipa referenced this pull request Jun 6, 2016
Merged

Segregated witness rebased #8149

@paveljanik paveljanik commented on the diff Jun 7, 2016
src/test/sigopcount_tests.cpp
@@ -64,4 +65,179 @@ BOOST_AUTO_TEST_CASE(GetSigOpCount)
BOOST_CHECK_EQUAL(p2sh.GetSigOpCount(scriptSig2), 3U);
}
+/**
+ * Verifies script execution of the zeroth scriptPubKey of tx output and
+ * zeroth scriptSig and witness of tx input.
+ */
+ScriptError VerifyWithFlag(const CTransaction& output, const CMutableTransaction& input, int flags)
+{
+ ScriptError error;
+ CTransaction inputi(input);
+ 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
paveljanik Jun 7, 2016 Contributor

ret is unused here.

@sdaftuar
Contributor
sdaftuar commented Jun 7, 2016

As brought up in sipa#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.

@sipa
Member
sipa commented Jun 12, 2016

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.

@sipa
Member
sipa commented Jun 13, 2016

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.

@sdaftuar sdaftuar commented on an outdated diff Jun 14, 2016
src/primitives/block.cpp
@@ -31,3 +31,12 @@ std::string CBlock::ToString() const
}
return s.str();
}
+
+int64_t GetBlockCost(const CBlock& block)
+{
+ // This implements the cost = (stripped_size * 4) + witness_size formula,
+ // using only serialization with and without witness data. As witness_size
+ // is equal to total_size - stripped_size, this formula is identical to:
+ // cost = (stripped_size * 3) + total_size.
+ return ::GetSerializeSize(block, SER_NETWORK, SERIALIZE_TRANSACTION_NO_WITNESS) * (WITNESS_SCALE_FACTOR - 1) + ::GetSerializeSize(block, SER_NETWORK, PROTOCOL_VERSION);
@sdaftuar
sdaftuar Jun 14, 2016 edited Contributor

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...

@sipa
Member
sipa commented Jun 14, 2016

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).

@kanzure
Contributor
kanzure commented Jun 14, 2016

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.

@instagibbs instagibbs and 1 other commented on an outdated diff Jun 14, 2016
qa/rpc-tests/test_framework/mininode.py
}
MAGIC_BYTES = {
"mainnet": b"\xf9\xbe\xb4\xd9", # mainnet
"testnet3": b"\x0b\x11\x09\x07", # testnet3
- "regtest": b"\xfa\xbf\xb5\xda" # regtest
+ "regtest": b"\xfa\xbf\xb5\xda", # regtest
+ "segnet": b"\x2e\x96\xea\xca" # segnet
@instagibbs
instagibbs Jun 14, 2016 edited Contributor

nit: Vestigial segnet reference

@sipa
sipa via email Jun 14, 2016 Member
@instagibbs
instagibbs Jun 14, 2016 Contributor

done

@instagibbs
Contributor

tACK wallet section plus fixups sipa@3f989b9...be976b7
tACK p2p-segwit.py tests plus fixups da0c46e
code review ACK for the rest minus unit tests through beginning of merge section: 2311cf6

@jl2012
Member
jl2012 commented Jun 14, 2016

tACK 2311cf6 : consensus behavior matches descriptions in BIP141 and BIP143

@sipa
Member
sipa commented Jun 16, 2016 edited

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
@sipa
Member
sipa commented Jun 16, 2016

New merge on top of #7600.

@instagibbs instagibbs commented on the diff Jun 16, 2016
src/wallet/rpcwallet.cpp
+ "\nAdd a witness address for a script (with pubkey or redeemscript known).\n"
+ "It returns the witness script.\n"
+
+ "\nArguments:\n"
+ "1. \"address\" (string, required) An address known to the wallet\n"
+
+ "\nResult:\n"
+ "\"witnessaddress\", (string) The value of the new address (P2SH of witness script).\n"
+ "}\n"
+ ;
+ throw runtime_error(msg);
+ }
+
+ {
+ LOCK(cs_main);
+ if (!IsWitnessEnabled(chainActive.Tip(), Params().GetConsensus()) && !GetBoolArg("-walletprematurewitness", false)) {
@instagibbs
instagibbs Jun 16, 2016 Contributor

Do we want to do a similar thing for createwitnessaddress?

@dcousens dcousens commented on the diff Jun 17, 2016
src/script/sign.cpp
txnouttype subType;
- bool fSolved =
- SignStep(creator, subscript, scriptSig, subType) && subType != TX_SCRIPTHASH;
- // Append serialized subscript whether or not it is completely signed:
- scriptSig << valtype(subscript.begin(), subscript.end());
- if (!fSolved) return false;
+ solved = solved && SignStep(creator, witnessscript, result, subType, SIGVERSION_WITNESS_V0);
+ sigdata.scriptWitness.stack = result;
+ result.clear();
+ }
+ else if (solved && whichType == TX_WITNESS_V0_SCRIPTHASH)
+ {
+ CScript witnessscript(result[0].begin(), result[0].end());
+ txnouttype subType;
+ solved = solved && SignStep(creator, witnessscript, result, subType, SIGVERSION_WITNESS_V0) && subType != TX_SCRIPTHASH && subType != TX_WITNESS_V0_SCRIPTHASH && subType != TX_WITNESS_V0_KEYHASH;
@dcousens
dcousens Jun 17, 2016 edited Contributor

Rather than reassign solved, maybe just return false?
Why are we waiting until the end of the scope?

@sipa
sipa Jun 17, 2016 Member

We want to push the potentially partial result to sigdata.scriptSig, which is done in line 179-182.

@sdaftuar
Contributor

ACK e847337

(Will ACK in #8149 as well)

@instagibbs instagibbs commented on the diff Jun 18, 2016
src/main.cpp
- {
- // Add in sigops done by pay-to-script-hash inputs;
- // this is to prevent a "rogue miner" from creating
- // an incredibly-expensive-to-validate block.
- nSigOps += GetP2SHSigOpCount(tx, view);
- if (nSigOps > MAX_BLOCK_SIGOPS)
- return state.DoS(100, error("ConnectBlock(): too many sigops"),
- REJECT_INVALID, "bad-blk-sigops");
- }
+ // GetTransactionSigOpCost counts 3 types of sigops:
+ // * legacy (always)
+ // * p2sh (when P2SH enabled in flags and excludes coinbase)
+ // * witness (when witness enabled in flags and excludes coinbase)
+ nSigOpsCost += GetTransactionSigOpCost(tx, view, flags);
+ if (nSigOpsCost > MAX_BLOCK_SIGOPS_COST)
+ return state.DoS(100, error("ConnectBlock(): too many sigops"),
@instagibbs
instagibbs Jun 18, 2016 Contributor

Upgraded nodes don't ask for blocks from non-upgraded?

@kazcw
kazcw Jun 18, 2016 Contributor

Oh, right. Nevermind..

@sipa
Member
sipa commented Jun 22, 2016

New merge after #8068 and #8179. See the details in #8149 (comment).

@NicolasDorier
Member

I udpated the sig cache PR (sipa#101)

@sipa
Member
sipa commented Jun 24, 2016

Merged as #8149.

@sipa sipa closed this Jun 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment