Skip to content
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

check that a separator is found for psbt inputs, outputs, and global map #14377

Merged
merged 1 commit into from Nov 1, 2018

Conversation

Projects
None yet
8 participants
@achow101
Copy link
Member

commented Oct 3, 2018

Currently it doesn't make sure that a separator was found so PSBTs missing a trailing separator would still pass. This fixes that and adds a test case for it.

It really only makes sense to check for the separator for the output maps as if an input or global map was missing a separator, the fields following it would be interpreted as belonging to the previous input or global map. However I have added the check for those two anyways to be consistent.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

Is this for 0.17.1 backport?

if (key.empty()) return;
if (key.empty()) {
found_sep = true;
break;

This comment has been minimized.

Copy link
@jimmysong

jimmysong Oct 3, 2018

Contributor

What's the rationale for breaking here instead of throwing the error right away?

This comment has been minimized.

Copy link
@achow101

achow101 Oct 3, 2018

Author Member

We want to throw the error if we don't get here.

@@ -300,14 +300,18 @@ struct PSBTInput
template <typename Stream>
inline void Unserialize(Stream& s) {
// Read loop
bool found_sep = false;
while(!s.empty()) {
// Read
std::vector<unsigned char> key;
s >> key;

// the key is empty if that was actually a separator byte
// This is a special case for key lengths 0 as those are not allowed (except for separator)

This comment has been minimized.

Copy link
@jimmysong

jimmysong Oct 3, 2018

Contributor

Comment may need updating?

This comment has been minimized.

Copy link
@achow101

achow101 Oct 3, 2018

Author Member

No

while(!s.empty()) {
// Read
std::vector<unsigned char> key;
s >> key;

// the key is empty if that was actually a separator byte
// This is a special case for key lengths 0 as those are not allowed (except for separator)
if (key.empty()) return;

This comment has been minimized.

Copy link
@jimmysong

jimmysong Oct 3, 2018

Contributor

same as above, comment may need updating?

while(!s.empty()) {
// Read
std::vector<unsigned char> key;
s >> key;

// the key is empty if that was actually a separator byte
// This is a special case for key lengths 0 as those are not allowed (except for separator)
if (key.empty()) break;

This comment has been minimized.

Copy link
@jimmysong

jimmysong Oct 3, 2018

Contributor

comment may need updating?

@sipa

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

utACK 4fb3388. I think it should go into 0.17.1.

@MarcoFalke MarcoFalke added this to the 0.17.1 milestone Oct 4, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2018

Coverage Change (pull 14377) Reference (master)
Lines -0.0117 % 87.0471 %
Functions -0.0154 % 84.1130 %
Branches +0.0018 % 51.5403 %
@meshcollider

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

utACK 4fb3388

@laanwj laanwj merged commit 4fb3388 into bitcoin:master Nov 1, 2018

2 checks passed

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

laanwj added a commit that referenced this pull request Nov 1, 2018

Merge #14377: check that a separator is found for psbt inputs, output…
…s, and global map

4fb3388 check that a separator is found for psbt inputs, outputs, and global map (Andrew Chow)

Pull request description:

  Currently it doesn't make sure that a separator was found so PSBTs missing a trailing separator would still pass. This fixes that and adds a test case for it.

  It really only makes sense to check for the separator for the output maps as if an input or global map was missing a separator, the fields following it would be interpreted as belonging to the previous input or global map. However I have added the check for those two anyways to be consistent.

Tree-SHA512: 50c0c08e201ba02494b369a4d36ddb73e6634eb5a4e4e201c4ef38fd2dbeea2c642b8a04d50c91615da61ecbfade37309e47431368f4b1064539c42015766b50

@sipa sipa referenced this pull request Nov 22, 2018

Merged

PSBT backports to 0.17 #14780

@fanquake

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

This will be backported in #14780.

sipa added a commit to sipa/bitcoin that referenced this pull request Dec 3, 2018

MarcoFalke added a commit that referenced this pull request Dec 5, 2018

Merge #14780: PSBT backports to 0.17
7bee414 Add test for conversion from non-witness to witness UTXO (Pieter Wuille)
ff56bb9 Add regression test for PSBT signing bug #14473 (Glenn Willen)
db445d4 Refactor PSBTInput signing to enforce invariant (Glenn Willen)
ad94165 Simplify arguments to SignPSBTInput (Glenn Willen)
39ece4f Add bool PSBTInputSigned (Glenn Willen)
70ee1f8 New PartiallySignedTransaction constructor from CTransction (Glenn Willen)
a9eab08 Remove redundant txConst parameter to FillPSBT (Glenn Willen)
cfdd6b2 More concise conversion of CDataStream to string (Glenn Willen)
a3fe125 check that a separator is found for psbt inputs, outputs, and global map (Andrew Chow)

Pull request description:

  This is a backport of #14588, #14377, and #14197's test to 0.17.

Tree-SHA512: 07535ec69a878a63b549e5e463345e233f34662dff805202614cf2ffc896c6d1981363e6d06d02db2e02d815075ad8ebdc5f93f637052cff8c8cbe6c8dfa096a

bvbfan added a commit to bvbfan/bitcoin that referenced this pull request Feb 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.