[bugfix] Do not send witnesses in cmpctblock #8271

1. 
   sipa commented at 6:20 PM on June 26, 2016: member

   This is a small bugfix for #8149: we should not sent witnesses inside cmpctblock without negotiating whether the peers support them.

2. Do not send witnesses in cmpctblock 252675efc6
3. jonasschnelli added the label P2P on Jun 26, 2016
4. 
   TheBlueMatt commented at 8:39 PM on June 27, 2016: member

   The BIP specifies you should encode transactions exactly as they would be in "tx" messages, so this does not really fix the issue - ultimately we need to inform the compact block stuff whether to include the witnesses (though its probably easier/better to just change the spec to require witness-inclusion).

5. 
   sipa commented at 11:38 PM on June 27, 2016: member

   I'm working on a spec for making witness + compactblocks work (using sendcmpct version 2 to indicate that blocktxn, cmpctblock should include witnesses, and that short ids should be based on wtxids), which seems to work quite well.

   This is just a minimal change to prevent things from breaking when witness nodes start talking to older compactblocks-aware nodes.

6. 
   TheBlueMatt commented at 11:44 PM on June 27, 2016: member

   Correct me if I'm wrong, but transaction encoding/hash with witness for a non-witness transaction and transaction encoding/hash without witness are the same, no? I'd think no one would ship code with compact blocks before we fix the witness/compact combination, so might as well just remove the code for non-witness compact with version 1...it'll only break current master nodes after segwit activates, so I'm not too worried.

7. 
   sipa commented at 11:49 PM on June 27, 2016: member

   0.13.0 will ship with compact blocks, but very likely without segwit activation. A potential future 0.13.1 with segwit activation can't send witnesses to 0.13.0 nodes, as they would consider those blocks invalid and/or the shorttixds would mismatch.

   You need to negotiate which parts of transactions are relevant unless you want relay between old and new nodes to be severely impacted any time a new piece of data is defined in transactions.

   The code is quite small, see my segwitcb branch.

8. 
   TheBlueMatt commented at 2:13 AM on June 28, 2016: member

   I mean you can still define version 1 to include witnesses, it doesnt effect the current protocol for non-witness transactions and continues to work fine, no?

9. 
   sipa commented at 6:39 AM on June 28, 2016: member

   Yes, you could. But it would mean 0.13.1 can't send to 0.13.0 nodes.

10. 
    TheBlueMatt commented at 6:48 AM on June 28, 2016: member

    Sure they could! 0.13.0 will know how to deserialize with witnesses, even if the block-processing-with-witnesses is not enabled. I think you meant to say 0.13.0 wont be able to send to 0.13.1, because it would drop the witnesses from processing, which I'd think is true?

    On 06/28/16 06:39, Pieter Wuille wrote:

    Yes, you could. But it would mean 0.13.1 can't send to 0.13.0 nodes.

    — You are receiving this because you commented. Reply to this email directly, view it on GitHub #8271 (comment), or mute the thread https://github.com/notifications/unsubscribe/AAnoHqS1W2YWU0XIBATMqAzGSoiBp9geks5qQMGZgaJpZM4I-m8y.

11. 
    sipa commented at 8:37 AM on June 28, 2016: member

    To 0.13.0 nodes these blocks should not have witnesses, as there has not been a softfork to define them. Sure it could strip them off before processing when received from a 0.13.1 node, but that is inefficient (especially for a protocol that tries to minimize bandwidth) and does not solve the problem for other potential future data extensions to the serialization.

    0.13.1 will never request from 0.13.0 after segwit activation, as blocks are only downloaded from other peers that can provide witnesses.

12. 
    sipa commented at 1:15 PM on July 8, 2016: member

    The discussion above should probably happen elsewhere.

    The PR here is a pure bugfix, though one that would only occur on testnet, and if there is ever a client on the network which does support BIP152 but not segwit.

13. 
    sdaftuar commented at 5:10 PM on July 8, 2016: member

    utACK. We should merge this for 0.13.0.

14. laanwj added this to the milestone 0.13.0 on Jul 11, 2016
15. 
    laanwj commented at 6:17 AM on July 14, 2016: member

    utACK 252675e

16. laanwj merged this on Jul 14, 2016
17. laanwj closed this on Jul 14, 2016

    ---

18. laanwj referenced this in commit 1bc9c8085f on Jul 14, 2016
19. DrahtBot locked this on Sep 8, 2021

Milestone
0.13.0