Skip to content

bitcoin-tx input verification (awemany, jnewbery) #10130

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 3 commits into from
Mar 31, 2017
Merged

bitcoin-tx input verification (awemany, jnewbery) #10130

laanwj merged 3 commits into from
Mar 31, 2017

Conversation

@jnewbery
Copy link
Contributor

@jnewbery jnewbery commented Mar 30, 2017

Check number of arguments passed to bitcoin-tx in outaddr and outpubkey.

This PR also updates the bitcoin-tx tests to check bitcoin-tx stderr and verify that expected errors are raised.

Adds test cases for calling bitcoin-tx with wrong number of separators in outaddr and outpubkey.

awemany and others added 3 commits March 30, 2017 15:35
@awemany @jnewbery
bitcoin-tx: Fix missing range check
eb66bf9 
The number of arguments is not checked MutateTxAddOutAddr(..), meaning
that

> ./bitcoin-tx -create outaddr=

accessed the vStrInputParts vector beyond its bounds.

This also includes work by jnewbery to check the inputs for
MutateTxAddPubKey()
@jnewbery
Check stderr when testing bitcoin-tx
21704f6
@jnewbery
Add tests for bitcoin-tx input checking
19ecd1e
@jnewbery jnewbery mentioned this pull request Mar 30, 2017
Closed
@jnewbery
Copy link
Contributor Author

jnewbery commented Mar 30, 2017

Replaces #10106. @gmaxwell @instagibbs @laanwj @jtimon

@fanquake fanquake added this to the 0.14.1 milestone Mar 30, 2017
@jnewbery
Copy link
Contributor Author

jnewbery commented Mar 30, 2017

@theuni has suggested that checking the stderr for error text may cause problems due to unicode, eols, etc, and that we'd be better off defining our own exit codes in bitcoin-tx and testing on those. I think it's ok to test for text in stderr for now, but we a future PR should define exit codes and test them in bctest.py.

@laanwj
Copy link
Member

laanwj commented Mar 31, 2017

I don't disagree with @theuni on that in general, however checking for text in error messages is perfectly fine for tests that are supposed to test these errors. Especially for a user-facing tool. We do a similar thing for checking e.g. RPC errors where we check both the code and the message.

@laanwj
Copy link
Member

laanwj commented Mar 31, 2017

utACK 19ecd1e

@gmaxwell
Copy link
Contributor

gmaxwell commented Mar 31, 2017

@awemany Fix moved over here, your review/testing would be very welcome!

gmaxwell
gmaxwell approved these changes Mar 31, 2017
View reviewed changes
Copy link
Contributor

@gmaxwell gmaxwell left a comment

Choose a reason for hiding this comment

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

ACK (did not test test, e.g. by introducing a bug).

@theuni
Copy link
Member

theuni commented Mar 31, 2017

As mentioned in the comments: for reference, I suggested using exit codes rather than comparing text because (here specifically), Wine piles nonsense about window creation into stderr when running without DISPLAY set. For that reason, we can't test that "stderr == expected", rather we have to test "expected in stderr". That's not great, but it's not a huge deal either.

utACK 19ecd1e

@maflcko maflcko changed the title bitcoin-tx input verification bitcoin-tx input verification (awemany, jnewbery) Mar 31, 2017
@laanwj laanwj merged commit 19ecd1e into bitcoin:master Mar 31, 2017
laanwj added a commit that referenced this pull request Mar 31, 2017
@laanwj
Merge #10130: bitcoin-tx input verification (awemany, jnewbery)
de301b0 
19ecd1e Add tests for bitcoin-tx input checking (John Newbery)
21704f6 Check stderr when testing bitcoin-tx (John Newbery)
eb66bf9 bitcoin-tx: Fix missing range check (Awemany)

Tree-SHA512: 08c6153cf7dd5e0ecd23e24d81af4c0f17534d484179dd91dcd78d42df14c91284341d31cc695469a64c507bce72c34231748b7cabb7df8f1051d228fb0a62c5
laanwj pushed a commit that referenced this pull request Mar 31, 2017
@awemany @laanwj
bitcoin-tx: Fix missing range check
04c21af 
The number of arguments is not checked MutateTxAddOutAddr(..), meaning
that

> ./bitcoin-tx -create outaddr=

accessed the vStrInputParts vector beyond its bounds.

This also includes work by jnewbery to check the inputs for
MutateTxAddPubKey()

Github-Pull: #10130
Rebased-From: eb66bf9
laanwj pushed a commit that referenced this pull request Mar 31, 2017
@jnewbery @laanwj
Check stderr when testing bitcoin-tx
ecc5232 
Github-Pull: #10130
Rebased-From: 21704f6
laanwj pushed a commit that referenced this pull request Mar 31, 2017
@jnewbery @laanwj
Add tests for bitcoin-tx input checking
f59e3df 
Github-Pull: #10130
Rebased-From: 19ecd1e
codablock pushed a commit to codablock/dash that referenced this pull request Jan 26, 2018
@laanwj @codablock
Merge bitcoin#10130: bitcoin-tx input verification (awemany, jnewbery)
a149129 
19ecd1e Add tests for bitcoin-tx input checking (John Newbery)
21704f6 Check stderr when testing bitcoin-tx (John Newbery)
eb66bf9 bitcoin-tx: Fix missing range check (Awemany)

Tree-SHA512: 08c6153cf7dd5e0ecd23e24d81af4c0f17534d484179dd91dcd78d42df14c91284341d31cc695469a64c507bce72c34231748b7cabb7df8f1051d228fb0a62c5
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
@laanwj @andvgal
Merge bitcoin#10130: bitcoin-tx input verification (awemany, jnewbery)
5f89df4 
19ecd1e Add tests for bitcoin-tx input checking (John Newbery)
21704f6 Check stderr when testing bitcoin-tx (John Newbery)
eb66bf9 bitcoin-tx: Fix missing range check (Awemany)

Tree-SHA512: 08c6153cf7dd5e0ecd23e24d81af4c0f17534d484179dd91dcd78d42df14c91284341d31cc695469a64c507bce72c34231748b7cabb7df8f1051d228fb0a62c5
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 27, 2019
@laanwj @CryptoCentric
Merge bitcoin#10130: bitcoin-tx input verification (awemany, jnewbery)
c5878f7 
19ecd1e Add tests for bitcoin-tx input checking (John Newbery)
21704f6 Check stderr when testing bitcoin-tx (John Newbery)
eb66bf9 bitcoin-tx: Fix missing range check (Awemany)

Tree-SHA512: 08c6153cf7dd5e0ecd23e24d81af4c0f17534d484179dd91dcd78d42df14c91284341d31cc695469a64c507bce72c34231748b7cabb7df8f1051d228fb0a62c5
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@gmaxwell gmaxwell gmaxwell approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.14.1

Development

Successfully merging this pull request may close these issues.

6 participants

@jnewbery @laanwj @gmaxwell @theuni @fanquake @awemany