Skip to content

Conversation

@kodslav
Copy link

@kodslav kodslav commented Jan 10, 2019

Fixes commit 6b25f29 where opcodes where probably lost in translation.

Made two versions, the looped test using the '<<' operator also (stream api).

Fixes commit 6b25f29 where opcodes where lost in translation.
@laanwj laanwj added the Tests label Jan 10, 2019
BOOST_CHECK(CScript(direct.begin(), direct.end()).IsPayToScriptHash());

//---------><----- cut here
// orig version fixed by re-adding OP_PUSHDATA2/4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what cut here and orig version means? I think those are confusion to test readers.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry my commit comment was a bit short.

I've made two versions of the fix since I didn't know what you prefer.

The orig version is clearer in what it fixes.
The block below that is perhaps less code, though not perhaps the quickest to understand at-a-glance.

@maflcko
Copy link
Member

maflcko commented Feb 13, 2019

@domob1812

BOOST_CHECK(!CScript(script.begin(), script.end()).IsPayToScriptHash());
}

//---------><----- cut here
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you either please remove the comments here or remove the whole second hunk. I think the fixup should be standalone and minimal.

@sipa
Copy link
Member

sipa commented Feb 13, 2019

utACK first version

@maflcko
Copy link
Member

maflcko commented Feb 13, 2019

ACK, sorry this was my oversight

@domob1812
Copy link
Contributor

Good catch, sorry for missing the OP_PUSHDATA2/4 ones in my change! utACK to the first version of the fix. Perhaps you can do the second change in a follow up, but I agree it should not be mixed with the fix.

Also, please squash the two commits.

@laanwj
Copy link
Member

laanwj commented Oct 25, 2019

This has been waiting for the author for more than half a year. Closing with "up for grabs".

@laanwj laanwj closed this Oct 25, 2019
maflcko pushed a commit that referenced this pull request Nov 1, 2019
5710dad test: fix script_p2sh_tests OP_PUSHBACK2/4 missing (kodslav)

Pull request description:

  Cleans up #15140 which fixes commit 6b25f29 where opcodes were lost in translation.

ACKs for top commit:
  laanwj:
    code review ACK 5710dad

Tree-SHA512: 3f7fbcaf0dd199626d9ec9fdf3c5b5c5c2a91c4cfe81fae5b1d5662a48e52cf4bd27c94f8f42ebdfe7a076c5d600ada5661a6902b03eb5dc3dc953f4524345ac
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 2, 2019
5710dad test: fix script_p2sh_tests OP_PUSHBACK2/4 missing (kodslav)

Pull request description:

  Cleans up bitcoin#15140 which fixes commit 6b25f29 where opcodes were lost in translation.

ACKs for top commit:
  laanwj:
    code review ACK 5710dad

Tree-SHA512: 3f7fbcaf0dd199626d9ec9fdf3c5b5c5c2a91c4cfe81fae5b1d5662a48e52cf4bd27c94f8f42ebdfe7a076c5d600ada5661a6902b03eb5dc3dc953f4524345ac
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
5710dad test: fix script_p2sh_tests OP_PUSHBACK2/4 missing (kodslav)

Pull request description:

  Cleans up bitcoin#15140 which fixes commit 6b25f29 where opcodes were lost in translation.

ACKs for top commit:
  laanwj:
    code review ACK 5710dad

Tree-SHA512: 3f7fbcaf0dd199626d9ec9fdf3c5b5c5c2a91c4cfe81fae5b1d5662a48e52cf4bd27c94f8f42ebdfe7a076c5d600ada5661a6902b03eb5dc3dc953f4524345ac
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
5710dad test: fix script_p2sh_tests OP_PUSHBACK2/4 missing (kodslav)

Pull request description:

  Cleans up bitcoin#15140 which fixes commit 6b25f29 where opcodes were lost in translation.

ACKs for top commit:
  laanwj:
    code review ACK 5710dad

Tree-SHA512: 3f7fbcaf0dd199626d9ec9fdf3c5b5c5c2a91c4cfe81fae5b1d5662a48e52cf4bd27c94f8f42ebdfe7a076c5d600ada5661a6902b03eb5dc3dc953f4524345ac
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 14, 2021
5710dad test: fix script_p2sh_tests OP_PUSHBACK2/4 missing (kodslav)

Pull request description:

  Cleans up bitcoin#15140 which fixes commit 6b25f29 where opcodes were lost in translation.

ACKs for top commit:
  laanwj:
    code review ACK 5710dad

Tree-SHA512: 3f7fbcaf0dd199626d9ec9fdf3c5b5c5c2a91c4cfe81fae5b1d5662a48e52cf4bd27c94f8f42ebdfe7a076c5d600ada5661a6902b03eb5dc3dc953f4524345ac
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants