Segregated witness rebased #8149

Merged
merged 27 commits into from Jun 24, 2016

Projects

Done in Segwit

@sipa
Member
sipa commented Jun 6, 2016 edited

This PR is a rebased and squashed version of #7910. As this is the form (in pieces or in whole) that we expect it to be merged in, I'm opening a separate pull request for it. I will leave the old one open for discussion and history.

The tree here is identical to the resulting tree there:

$ git show -s --format="%T" 3cb46c1a4ac94f4a7f25368bc2ba3c784c901b89
8ddfe56cfedba64667c63dd0fef6ee9584889719
$ git show -s --format="%T" 17389dc466f2acf8bfa64ce0416f3b5281445a5c
8ddfe56cfedba64667c63dd0fef6ee9584889719

Where 3cb46c1 is #7910's tip commit, and 17389dc is this PR's tip commit.

Please make comments on #7910, so the history can be tracked, and everything stays in one place.

@sipa
Member
sipa commented Jun 6, 2016 edited

Here is a categorized list of the commits:

  • P2P/node/consensus (sipa@4182520...7080d47)
    • 8199125 BIP144: Serialization, hashes, relay (sender side)
    • 04dd13a BIP141: Witness program
    • 1fff664 BIP141: Commitment structure and deployment
    • 902c279 BIP144: Handshake and relay (receiver side)
    • 113b3e5 Refactor script validation to observe amounts
    • 1f5bb93 BIP143: Verification logic
    • 87252e9 [RPC] Return witness data in blockchain RPCs
    • 94c2abb BIP141: Other consensus critical limits, and BIP145
    • f0b33a5 [libconsensus] Script verification API with amounts
    • 76cb63b Add rewind logic to deal with post-fork software updates
  • wallet (sipa@7080d47...8a5665a)
    • b344e52 BIP143: Signing logic
    • ddb6682 [RPC] Add wallet support for witness transactions (using P2SH)
    • 0c57081 [RPC] signrawtransaction can sign P2WSH
  • tests (sipa@8a5665a...37916d3)
    • f7d6e0a [qa] Witness version 0 signing unit tests
    • 2cbf540 [qa] Add transaction tests for segwit
    • 29344db [qa] Add segwit support to script_tests
    • ec00dc9 [qa] Autogeneration support for witness in script_tests
    • e711429 [qa] Add rpc test for segwit
    • 7d828f1 [qa] p2p segwit tests
    • 0612ad6 [qa] script_tests: witness tests can specify tx amount
    • 89179b0 [qa] Add GetTransactionSigOpCost unit tests
  • deployment (sipa@37916d3...74300b9)
    • 74300b9 BIP9 parameters for testnet
@kanzure
Contributor
kanzure commented Jun 6, 2016

ACK for same git tree hash for 3cb46c1 and 17389dc.

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

Segregated witness #7910

5 of 7 tasks complete
@sipa sipa 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);
@sipa
sipa Jun 7, 2016 Member

Please comment in #7910.

@sipa
Member
sipa commented Jun 12, 2016

Updated with a rebased/squashed version of the changes in #7910. The resulting tree should still be identical.

@sipa
Member
sipa commented Jun 13, 2016

Updated again.

@MarcoFalke MarcoFalke added this to the 0.13.0 milestone Jun 15, 2016
@sdaftuar
Contributor

ACK 74300b9a5ed24e9f7d80ecc39d4e19690732ccbe

@jonasschnelli
Member

Reviewed everything. Tested different scenarios. Mostly focused on the wallet.
ACK 74300b9a5ed24e9f7d80ecc39d4e19690732ccbe

@NicolasDorier
Member
NicolasDorier commented Jun 17, 2016 edited

ACK 74300b9a5ed24e9f7d80ecc39d4e19690732ccbe (focused more on the scripting side, and tx signature v2, lightly reviewed the rest)

sipa and others added some commits Nov 6, 2015
@sipa sipa BIP144: Serialization, hashes, relay (sender side)
Contains refactorings by Eric Lombrozo.
Contains fixup by Nicolas Dorier.
Contains cleanup of CInv::GetCommand by Alex Morcos
7030d9e
@sipa sipa BIP141: Commitment structure and deployment
Includes a fix by Suhas Daftuar and LongShao007
8b49040
@sipa sipa BIP141: Witness program 449f9b8
@sipa sipa BIP144: Handshake and relay (receiver side)
Service bit logic by Nicolas Dorier.

Only download blocks from witness peers after fork.
b8a9749
@sipa sipa --- [SEGWIT] begin: P2P/node/consensus --- ecacfd9
@sipa sipa BIP143: Verification logic
Includes simplifications by Eric Lombrozo.
3dd4102
@sipa sipa BIP141: Other consensus critical limits, and BIP145
Includes changes by Suhas Daftuar, Luke-jr, and mruddy.
2b1f6f9
@jl2012 @sipa jl2012 [RPC] Return witness data in blockchain RPCs
Includes RPC field name changes by Luke-jr.
7c4bf77
@afk11 @sipa afk11 [libconsensus] Script verification API with amounts
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
b7dbeb2
@sipa sipa Add rewind logic to deal with post-fork software updates
Includes logic for dealing with pruning by Suhas Daftuar.
6032f69
@sipa sipa --- [SEGWIT] begin: wallet --- 9757b57
@sipa sipa Refactor script validation to observe amounts
This is a preparation for BIP143 support.
0ef1dd3
@sipa sipa Do not use compact blocks when segwit is enabled af87a67
@sipa sipa [qa] Witness version 0 signing unit tests 0aa9207
@sipa sipa [RPC] Add wallet support for witness transactions (using P2SH)
Includes support for pushkeyhash wit v0 by Alex Morcos.
f4691ab
@morcos @sipa morcos [qa] Add rpc test for segwit
Amended by Pieter Wuille to use multisig 1-of-1 for P2WSH tests, and BIP9
based switchover logic.

Fixes and py3 conversion by Marco Falke.
4f7ff00
@sipa sipa --- [SEGWIT] begin: tests --- 978e200
@sipa sipa BIP143: Signing logic 605e847
@NicolasDorier @sipa NicolasDorier [qa] Add transaction tests for segwit
Including BIP143 P2WSH examples by jl2012.
00f46cb
@sipa sipa [qa] Add segwit support to script_tests
Contains fix by Johnson Lau.
06d3805
@sipa sipa [qa] Autogeneration support for witness in script_tests 66cca79
@NicolasDorier @sipa NicolasDorier [RPC] signrawtransaction can sign P2WSH 745eb67
@sdaftuar @sipa sdaftuar [qa] p2p segwit tests
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.

Py3 conversion by Marco Falke

Test to make sure upgraded nodes don't ask for non-wit blocks by
Gregory Sanders.
330b0f3
@jl2012 @sipa jl2012 BIP9 parameters for testnet f852813
@sdaftuar @sipa sdaftuar [qa] script_tests: witness tests can specify tx amount
Add tests that witness signatures cover value
d846e02
@jonasnick @sipa jonasnick [qa] Add GetTransactionSigOpCost unit tests fdb43df
@sipa sipa --- [SEGWIT] begin: deployment --- 070dbc4
@sipa
Member
sipa commented Jun 22, 2016 edited

Rebased on top of #8068 and #8179.

Note that since there is currently no written-down specification for how segwit should interact with compact blocks, I've taken the conservative approach of disabling compact blocks whenever the segwit softfork is defined (in practice this means that compact blocks won't be used on testnet, but will be on mainnet).

Defining how segwit + compact blocks work together should be trivial, but it shouldn't burden this PR and testing thereof.

@sneurlax

will run #8149 node! finally!

@laanwj
Member
laanwj commented Jun 24, 2016

ACK f852813

@laanwj laanwj merged commit f852813 into bitcoin:master Jun 24, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laanwj laanwj added a commit that referenced this pull request Jun 24, 2016
@laanwj laanwj Merge #8149: Segregated witness rebased
f852813 BIP9 parameters for testnet (Johnson Lau)
070dbc4 --- [SEGWIT] begin: deployment --- (Pieter Wuille)
fdb43df [qa] Add GetTransactionSigOpCost unit tests (Jonas Nick)
d846e02 [qa] script_tests: witness tests can specify tx amount (Suhas Daftuar)
330b0f3 [qa] p2p segwit tests (Suhas Daftuar)
4f7ff00 [qa] Add rpc test for segwit (Alex Morcos)
66cca79 [qa] Autogeneration support for witness in script_tests (Pieter Wuille)
06d3805 [qa] Add segwit support to script_tests (Pieter Wuille)
00f46cb [qa] Add transaction tests for segwit (NicolasDorier)
0aa9207 [qa] Witness version 0 signing unit tests (Pieter Wuille)
978e200 --- [SEGWIT] begin: tests --- (Pieter Wuille)
745eb67 [RPC] signrawtransaction can sign P2WSH (NicolasDorier)
f4691ab [RPC] Add wallet support for witness transactions (using P2SH) (Pieter Wuille)
605e847 BIP143: Signing logic (Pieter Wuille)
9757b57 --- [SEGWIT] begin: wallet --- (Pieter Wuille)
af87a67 Do not use compact blocks when segwit is enabled (Pieter Wuille)
6032f69 Add rewind logic to deal with post-fork software updates (Pieter Wuille)
b7dbeb2 [libconsensus] Script verification API with amounts (Thomas Kerin)
2b1f6f9 BIP141: Other consensus critical limits, and BIP145 (Pieter Wuille)
7c4bf77 [RPC] Return witness data in blockchain RPCs (Johnson Lau)
3dd4102 BIP143: Verification logic (Pieter Wuille)
0ef1dd3 Refactor script validation to observe amounts (Pieter Wuille)
b8a9749 BIP144: Handshake and relay (receiver side) (Pieter Wuille)
8b49040 BIP141: Commitment structure and deployment (Pieter Wuille)
449f9b8 BIP141: Witness program (Pieter Wuille)
7030d9e BIP144: Serialization, hashes, relay (sender side) (Pieter Wuille)
ecacfd9 --- [SEGWIT] begin: P2P/node/consensus --- (Pieter Wuille)
d612837
@skaag
skaag commented Jun 24, 2016

Exciting release! Great job :)

@dcousens
Contributor
dcousens commented Jun 26, 2016 edited

utACK 070dbc4 (w/ non-intensive testing done)

@petertodd
Member

utACK consensus critical parts, 070dbc4

I wrote up an in-depth review here: https://petertodd.org/2016/segwit-consensus-critical-code-review

Note that I think we may have an issue with the P2P code with the (non-consensus) handling of transactions; see the last part of my writeup: https://petertodd.org/2016/segwit-consensus-critical-code-review#peer-to-peer-networking

@ysangkok ysangkok referenced this pull request in bitcoin/bips Jul 20, 2016
@jl2012 jl2012 BIP141 clarifications and formatting
Add rationale of block cost
Change the name of "witness nonce" to "witness reserved value"
Update link to reference implementation
Formatting
ab4f511
@gmaxwell gmaxwell referenced this pull request in bitcoinclassic/bitcoinclassic Aug 14, 2016
Closed

Implement segregated witness. #185

@rebroad rebroad commented on the diff Aug 25, 2016
src/main.cpp
@@ -470,6 +473,10 @@ void UpdateBlockAvailability(NodeId nodeid, const uint256 &hash) {
}
void MaybeSetPeerAsAnnouncingHeaderAndIDs(const CNodeState* nodestate, CNode* pfrom) {
+ if (nLocalServices & NODE_WITNESS) {
@rebroad
rebroad Aug 25, 2016 Contributor

How does SegWit impact on the usefulness of Compact Blocks?

@sipa
sipa via email Aug 25, 2016 Member
@rebroad rebroad commented on the diff Aug 25, 2016
src/main.cpp
@@ -4684,6 +4940,14 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam
}
}
+uint32_t GetFetchFlags(CNode* pfrom, CBlockIndex* pprev, const Consensus::Params& chainparams) {
+ uint32_t nFetchFlags = 0;
+ if (IsWitnessEnabled(pprev, chainparams) && State(pfrom->GetId())->fHaveWitness) {
@rebroad
rebroad Aug 25, 2016 Contributor

Ok, I have to ask (as I've been wondering about this for a while), but what is the purpose of this GetId() function? Why use it instead of the variable id?

@rebroad rebroad commented on the diff Aug 26, 2016
src/main.cpp
@@ -4790,6 +5054,12 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
pfrom->fClient = !(pfrom->nServices & NODE_NETWORK);
+ if((pfrom->nServices & NODE_WITNESS))
+ {
+ LOCK(cs_main);
+ State(pfrom->GetId())->fHaveWitness = true;
@rebroad
rebroad Aug 26, 2016 edited Contributor

How is this useful? (pfrom->nServices & NODE_WITNESS) is 33 characters, State(pfrom->GetId())->fHaveWitness is 35 characters. It's easier and quicker and uses up less memory to type the thing this variable returns.

@rebroad rebroad commented on the diff Aug 26, 2016
src/main.cpp
@@ -5016,8 +5292,10 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
pfrom->PushMessage(NetMsgType::GETHEADERS, chainActive.GetLocator(pindexBestHeader), inv.hash);
CNodeState *nodestate = State(pfrom->GetId());
if (CanDirectFetch(chainparams.GetConsensus()) &&
- nodestate->nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
- if (nodestate->fProvidesHeaderAndIDs)
+ nodestate->nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER &&
+ (!IsWitnessEnabled(chainActive.Tip(), chainparams.GetConsensus()) || State(pfrom->GetId())->fHaveWitness)) {
@rebroad
rebroad Aug 26, 2016 Contributor

Maybe I'm misreading the code, but once SegWit activates then where is the code that downloads blocks upon receiving a block inv? Block invs are no longer responded to?

@sipa
sipa Aug 26, 2016 Member

Read the part of the line after ||.

@rebroad
rebroad Aug 26, 2016 Contributor

Sorry, it's taking me a while to get my head around this - I keep making the assumption that most nodes won't be running SegWit for some reason!!

@gmaxwell gmaxwell referenced this pull request in BitcoinUnlimited/BitcoinUnlimited Sep 9, 2016
Open

Segregated witness. #91

+ nLocalServices = ServiceFlags(nLocalServices | NODE_WITNESS);
+ // Only care about others providing witness capabilities if there is a softfork
+ // defined.
+ nRelevantServices = ServiceFlags(nRelevantServices | NODE_WITNESS);
@rebroad
rebroad Sep 16, 2016 Contributor

So witness nodes won't make outbound connections to non-witness nodes? This puts a reliance on witness nodes that can receive inbound connections as these will be the only witness nodes talking to the non-witness nodes.

@@ -655,6 +662,9 @@ CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& loc
CBlockIndex* pindex = (*mi).second;
if (chain.Contains(pindex))
return pindex;
+ if (pindex->GetAncestor(chain.Height()) == chain.Tip()) {
+ return chain.Tip();
@xenog
xenog Nov 2, 2016

I understand this as: if we have a block hash that the caller sent us in our mapBlockIndex that is not in the main chain, but is a descendant of our best block, then send our best block. Is there a case where our mapBlockIndex knows about a block that is higher than the tip of our chain? Are these data structures not kept in sync so that all the blocks in mapBlockIndex are also in chain?

@sipa
Member
sipa commented Nov 2, 2016
@sdaftuar
Contributor
sdaftuar commented Nov 2, 2016

@sipa @xenog: FYI the specific change to FindForkInGlobalIndex() was precipitated by the introduction of RewindBlockIndex(), which causes nodes that upgrade after segwit activation to disconnect blocks back to the activation point, so that the witness-version of those blocks could then be redownloaded and reconnected. During that window, mapBlockIndex will be (far) ahead of chainActive.Tip().

If I remember correctly, the wallet rescan logic uses FindForkInGlobalIndex, and prior to this change, it might return a block older than was necessary (which could be a problem for pruning nodes).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment