Page MenuHomePhabricator
Differential D1556

vout parameter check in createrawtransaction is confused with missing & type_error.
ClosedPublic
Actions

Authored by andychen on Jul 11 2018, 02:48.

Details

Reviewers
jasonbcox
schancel
Group Reviewers
Restricted Project
Restricted Owners Package(Owns No Changed Paths)
Commits
rSTAGING7e6d3e27bea9: vout parameter check in createrawtransaction is confused with missing &…
rABC7e6d3e27bea9: vout parameter check in createrawtransaction is confused with missing &…
Summary

vout parameter in createrawtransaction could be missing or non-num,
but the code simply check the type of vout and throws missing error, which is
not right for string vout.

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
fix/createrawtransaction_param_vout_check
Lint
Lint Passed
SeverityLocationCodeMessage
Auto-Fixsrc/rpc/rawtransaction.cpp:1CFMTCode style violation
Unit
No Test Coverage
Build Status
Buildable 2849
Build 3802: Bitcoin ABC Buildbot (legacy)
Build 3801: arc lint + arc unit

Event Timeline

andychen created this revision.Jul 11 2018, 02:48
Owners added a reviewer: Restricted Owners Package.Jul 11 2018, 02:48
teamcity added a subscriber: teamcity.Jul 11 2018, 02:50

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

andychen updated this revision to Diff 4272.Jul 11 2018, 02:57

fix some code formatting auto-done by editor.

teamcity added a comment.Jul 11 2018, 02:59

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster failed remote builds in B2846: Diff 4271!Jul 11 2018, 03:28
teamcity added a comment.Jul 11 2018, 03:28

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

teamcity added a comment.Jul 11 2018, 03:37

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B2847: Diff 4272!Jul 11 2018, 03:37
deadalnix added a subscriber: deadalnix.Jul 11 2018, 14:01

make check is not an appropriate test for this. This is changing behavior, and such behavior needs to be tested. Please update the test plan accordingly.

Except this, LGTM.

andychen added a comment.Jul 11 2018, 14:59
In D1556#32028, @deadalnix wrote:

make check is not an appropriate test for this. This is changing behavior, and such behavior needs to be tested. Please update the test plan accordingly.

Except this, LGTM.

I will make it :)

andychen updated this revision to Diff 4274.Jul 11 2018, 16:18

fix Code style violation

teamcity added a comment.Jul 11 2018, 16:20

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

jasonbcox added a subscriber: jasonbcox.Jul 11 2018, 16:37

Amaury is saying you need to add this to the test plan, at a minimum:

/test/functional/test_runner.py rawtransactions.py

This ensures you test changes pass locally before publishing the diff. :)

jasonbcox requested changes to this revision.Jul 11 2018, 16:40
jasonbcox added inline comments.
test/functional/rawtransactions.py
181 ↗(On Diff #4274)

you introduced a typo. 'vot' <-> 'vout'

188 ↗(On Diff #4274)

The "1" for 'vout' shouldn't be a string by the looks of the other test cases.

This revision now requires changes to proceed.Jul 11 2018, 16:40
andychen added a comment.Jul 11 2018, 16:41
In D1556#32034, @jasonbcox wrote:

Amaury is saying you need to add this to the test plan, at a minimum:

/test/functional/test_runner.py rawtransactions.py

This ensures you test changes pass locally before publishing the diff. :)

OK, thanks, I had test it locally and it's OK, I will update it to the commit. The code format lint is quite hard to fix, any suggestion?

andychen added inline comments.Jul 11 2018, 16:45
test/functional/rawtransactions.py
181 ↗(On Diff #4274)

that's for missing of vout, but maybe just remove vout parameter is better than "vot"?

188 ↗(On Diff #4274)

that's for number test

andychen updated this revision to Diff 4275.Jul 11 2018, 16:50

update Test Plan & make rawtransaction test case better

teamcity added a comment.Jul 11 2018, 16:52

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Jul 11 2018, 17:01

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B2848: Diff 4274!Jul 11 2018, 17:01
Harbormaster failed remote builds in B2849: Diff 4275!Jul 11 2018, 17:35
teamcity added a comment.Jul 11 2018, 17:35

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

jasonbcox added a comment.Jul 11 2018, 17:52

Regarding linting errors, check clang-format --version and make sure you're on 4.x instead of 5 or 6.

test/functional/rawtransactions.py
181 ↗(On Diff #4274)

Yes, I think removing it all together is better, as it doesn't look like an error.

188 ↗(On Diff #4274)

Ah, sorry didn't read close enough.

andychen added a comment.Jul 12 2018, 03:14
In D1556#32053, @jasonbcox wrote:

Regarding linting errors, check clang-format --version and make sure you're on 4.x instead of 5 or 6.

found 7.0...thanks.

andychen updated this revision to Diff 4283.Jul 12 2018, 09:53

fix string concat bug by use string directly.

teamcity added a comment.Jul 12 2018, 09:55

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Jul 12 2018, 10:35

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B2855: Diff 4283.Jul 12 2018, 10:35
andychen added a comment.Jul 12 2018, 10:43
In D1556#32053, @jasonbcox wrote:

Regarding linting errors, check clang-format --version and make sure you're on 4.x instead of 5 or 6.

It's green now :)

schancel requested changes to this revision.Jul 12 2018, 18:18
schancel added a subscriber: schancel.

Seems like a good change. However, please fix the code style as per what I suggested. Just check for null first.

src/rpc/rawtransaction.cpp
510 ↗(On Diff #4283)

Please make this two ifs. rather than an or and an internal branch (via ternary operator)

This revision now requires changes to proceed.Jul 12 2018, 18:18
andychen added inline comments.Jul 13 2018, 01:30
src/rpc/rawtransaction.cpp
510 ↗(On Diff #4283)

The first version I made is 2 ifs, but the code format said the new if is coped from line 512:
https://reviews.bitcoinabc.org/D1556?id=4272

Then I turn the 2 ifs into one to avoid the coding style warning, I think 2ifs is more easy to read, but make the 2ifs into 1 is acceptable too, what do you think?

andychen updated this revision to Diff 4294.Jul 13 2018, 02:21

seperate two ifs

andychen added a comment.Jul 13 2018, 02:22
In D1556#32179, @schancel wrote:

Seems like a good change. However, please fix the code style as per what I suggested. Just check for null first.

It's done :)

teamcity added a comment.Jul 13 2018, 02:22

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Jul 13 2018, 03:03

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B2862: Diff 4294!Jul 13 2018, 03:03
andychen added a comment.Jul 13 2018, 03:43

sometimes bip68-sequence.py test will fail which I think is irrelevant with this commit.

teamcity added a comment.Jul 13 2018, 03:56

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster completed remote builds in B2862: Diff 4294.Jul 13 2018, 04:36
teamcity added a comment.Jul 13 2018, 04:36

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

jasonbcox accepted this revision.Jul 13 2018, 18:41
andychen added a comment.Jul 14 2018, 10:15
In D1556#32179, @schancel wrote:

Seems like a good change. However, please fix the code style as per what I suggested. Just check for null first.

It's done, it can be land after you accept it.

schancel accepted this revision.Jul 16 2018, 16:39
This revision is now accepted and ready to land.Jul 16 2018, 16:39
Closed by commit rABC7e6d3e27bea9: vout parameter check in createrawtransaction is confused with missing &… (authored by andychen). · Explain WhyJul 16 2018, 23:06
This revision was automatically updated to reflect the committed changes.
PiRK mentioned this in D8050: [backport#16078] replace tx hash with txid in test rawtransaction.Oct 22 2020, 09:37
PiRK mentioned this in rABC8244813e382f: [backport#16078] replace tx hash with txid in test rawtransaction.Oct 22 2020, 19:53

Revision Contents
Changeset List

PathSize
src/
rpc/
6 lines
test/
functional/
29 lines

Diff 4275

View Options

src/rpc/rawtransaction.cpp

Loading...
View Options

test/functional/rawtransactions.py

Loading...