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
tests: Add unit testing for the CompressScript function #17220
Conversation
src/test/compress_tests.cpp
Outdated
CScript script; | ||
script.clear(); | ||
script << OP_DUP << OP_HASH160 << ToByteVector(pubkey.GetID()) << OP_EQUALVERIFY << OP_CHECKSIG; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This could be put in one line by immediately assigning the script on the declaration: CScript script = CScript() << OP_DUP ..... << OP_CHECKSIG
(also applies to the other three functions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't assigning the script on the declaration remove the opportunity to call .clear()
on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that calling .clear()
is needed here at all, since newly constructed CScript
s are empty anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to clear()
it, since the default constructor CScript()
constructs a (logically) empty CScript
.
ACK, thanks for adding tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 3208906
Read code, ran tests, feel free to ignore the nit!
src/test/compress_tests.cpp
Outdated
CScript script; | ||
script.clear(); | ||
script << OP_DUP << OP_HASH160 << ToByteVector(pubkey.GetID()) << OP_EQUALVERIFY << OP_CHECKSIG; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to clear()
it, since the default constructor CScript()
constructs a (logically) empty CScript
.
4d3cdda
to
7756f06
Compare
7756f06
to
b05ec41
Compare
ACK b05ec41 |
b05ec41 Add unit testing for the CompressScript functions (marcaiaf) Pull request description: Salvaging #15104 which adds unit tests for CompressScript function in `compressor.cpp` Tested following cases for the CScript: - CKeyID - CScriptID - Uncompressed CPubKey (of size: 65) - Compressed CPubKey (of size: 32) ACKs for top commit: theStack: ACK b05ec41 Tree-SHA512: 7e23ace39383122802dfe5f7d38190d772f5db4045a67b7a9bd4c06797a17e0cdc41d6fac92d448057eb7df50172155dc824587c16c68c79fd1a4de37b772001
Summary: This is a backport of Core [[bitcoin/bitcoin#17220 | PR17220]] Test Plan: `ninja && ninja check` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D8192
…unction b05ec41 Add unit testing for the CompressScript functions (marcaiaf) Pull request description: Salvaging bitcoin#15104 which adds unit tests for CompressScript function in `compressor.cpp` Tested following cases for the CScript: - CKeyID - CScriptID - Uncompressed CPubKey (of size: 65) - Compressed CPubKey (of size: 32) ACKs for top commit: theStack: ACK bitcoin@b05ec41 Tree-SHA512: 7e23ace39383122802dfe5f7d38190d772f5db4045a67b7a9bd4c06797a17e0cdc41d6fac92d448057eb7df50172155dc824587c16c68c79fd1a4de37b772001
…unction b05ec41 Add unit testing for the CompressScript functions (marcaiaf) Pull request description: Salvaging bitcoin#15104 which adds unit tests for CompressScript function in `compressor.cpp` Tested following cases for the CScript: - CKeyID - CScriptID - Uncompressed CPubKey (of size: 65) - Compressed CPubKey (of size: 32) ACKs for top commit: theStack: ACK bitcoin@b05ec41 Tree-SHA512: 7e23ace39383122802dfe5f7d38190d772f5db4045a67b7a9bd4c06797a17e0cdc41d6fac92d448057eb7df50172155dc824587c16c68c79fd1a4de37b772001
…unction b05ec41 Add unit testing for the CompressScript functions (marcaiaf) Pull request description: Salvaging bitcoin#15104 which adds unit tests for CompressScript function in `compressor.cpp` Tested following cases for the CScript: - CKeyID - CScriptID - Uncompressed CPubKey (of size: 65) - Compressed CPubKey (of size: 32) ACKs for top commit: theStack: ACK bitcoin@b05ec41 Tree-SHA512: 7e23ace39383122802dfe5f7d38190d772f5db4045a67b7a9bd4c06797a17e0cdc41d6fac92d448057eb7df50172155dc824587c16c68c79fd1a4de37b772001
Salvaging #15104 which adds unit tests for CompressScript function in
compressor.cpp
Tested following cases for the CScript: