New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bugfix] Do not send witnesses in cmpctblock #8271

Merged
merged 1 commit into from Jul 14, 2016

Conversation

Projects
None yet
5 participants
@sipa
Member

sipa commented Jun 26, 2016

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

@jonasschnelli jonasschnelli added the P2P label Jun 26, 2016

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jun 27, 2016

Contributor

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

Contributor

TheBlueMatt commented Jun 27, 2016

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 27, 2016

Member
Member

sipa commented Jun 27, 2016

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jun 27, 2016

Contributor

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.

Contributor

TheBlueMatt commented Jun 27, 2016

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.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 27, 2016

Member
Member

sipa commented Jun 27, 2016

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jun 28, 2016

Contributor

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?

Contributor

TheBlueMatt commented Jun 28, 2016

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?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 28, 2016

Member
Member

sipa commented Jun 28, 2016

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jun 28, 2016

Contributor

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.

Contributor

TheBlueMatt commented Jun 28, 2016

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.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

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

Member

sipa commented Jun 28, 2016

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.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

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

Member

sipa commented Jul 8, 2016

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.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jul 8, 2016

Member

utACK. We should merge this for 0.13.0.

Member

sdaftuar commented Jul 8, 2016

utACK. We should merge this for 0.13.0.

@laanwj laanwj added this to the 0.13.0 milestone Jul 11, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 14, 2016

Member

utACK 252675e

Member

laanwj commented Jul 14, 2016

utACK 252675e

@laanwj laanwj merged commit 252675e into bitcoin:master Jul 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 14, 2016

Merge #8271: [bugfix] Do not send witnesses in cmpctblock
252675e Do not send witnesses in cmpctblock (Pieter Wuille)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment