Fix RPC signrawtransaction silently accepting missing amount field, make it throw an error instead
ClosedPublic
Actions

Authored by CCulianu on Aug 8 2017, 17:11.

Details

Reviewers

sickpig
freetrader
dgenr8
deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGING425af746dfdd: Fix RPC signrawtransaction silently accepting missing amount field, make it…
rABC425af746dfdd: Fix RPC signrawtransaction silently accepting missing amount field, make it…

Summary

After hard fork, replay protected tx's require the amount field to be non-zero for previous outputs in signed tx's. JSON RPC code was accepting transactions with this field missing when it should have been throwing an error. Users had the impression the transaction that was signed was good, but when trying to sent to network, they would get an error that the tx is bad. See github issue https://github.com/Bitcoin-ABC/bitcoin-abc/issues/63.

Depends on D448

Test Plan

Depends on D448

make check and also rpc-tests.py

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

CCulianu created this revision.Aug 8 2017, 17:11

CCulianu edited the summary of this revision. (Show Details)Aug 8 2017, 17:13

CCulianu edited the test plan for this revision. (Show Details)

CCulianu added a parent revision: D448: Run autopep8 signrawtransactions.py.

CCulianu retitled this revision from Fix RPC signrawtransaction silently accepting missing amount field, make it throw an error instead AbandonedPublic to Fix RPC signrawtransaction silently accepting missing amount field, make it throw an error instead.

You did not need to create a new diff. You could just have rebased the existing one.

qa/rpc-tests/signrawtransactions.py
39 ↗	(On Diff #1112)	Where does these amount come from ?
95 ↗	(On Diff #1112)	dito
src/rpc/rawtransaction.cpp
912 ↗	(On Diff #1112)	I think we should just keep it a number. This is what the spec says and having to handle various string may lead to bad surprises down the road.
960 ↗	(On Diff #1112)
src/test/rpc_tests.cpp
195 ↗	(On Diff #1112)	C++ tests >> python tests.

CCulianu added inline comments.Aug 8 2017, 18:02

qa/rpc-tests/signrawtransactions.py
39 ↗	(On Diff #1112)	They are invented out of thin air -- it is a test after all to see if a tx signs. This tx doesn't have to be valid or use valid inputs or anything. It's just testing the rpc signing code. I hope that's ok...
src/rpc/rawtransaction.cpp
912 ↗	(On Diff #1112)	Well as a general rule I'm all in favor of software being flexible with slightly malformed inputs if there isn't too much overhead in supporting them and if the intent is clear... so in that spirit I guess supporting both "2.002" and 2.002 is a decent idea. This is especially in light of the fact that it would be an API change to make it more strict -- actually I am not sure how old this API is, but Core, for example, supports both forms. Other people care to chime in on this?

Updated permissions on signrawtransaction.py to +x

Harbormaster completed remote builds in B700: Diff 1117.Aug 8 2017, 18:56

deadalnix added inline comments.Aug 8 2017, 20:25

src/rpc/rawtransaction.cpp
912 ↗	(On Diff #1112)	But then we have all kind of edge cases when JS already handle that for us and provide a standard for numbers.

CCulianu added inline comments.Aug 8 2017, 21:07

src/rpc/rawtransaction.cpp
912 ↗	(On Diff #1112)	Well existing functionality accepted both. Let's just leave it to work like existing, why make things harder for people that already wrote code against this RPC server?