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

Authored by CCulianu on Aug 7 2017, 16:59.

Details

Reviewers

dgenr8
freetrader
deadalnix

Group Reviewers

Restricted Project

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.

Test Plan

make check
also: rpc-tests.py raw_transactions

Diff Detail

Repository

rABC Bitcoin ABC

Branch

issue63_amount

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 688
Build 688: arc lint + arc unit

Event Timeline

CCulianu created this revision.Aug 7 2017, 16:59

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 7 2017, 16:59

CCulianu edited the test plan for this revision. (Show Details)Aug 7 2017, 17:34

deadalnix requested changes to this revision.Aug 7 2017, 20:57

deadalnix added inline comments.

src/rpc/rawtransaction.cpp
756	Please do not change the API.
945	It needs to be specified, the value doesn't matter.

This revision now requires changes to proceed.Aug 7 2017, 20:57

I need more review and comment since I'm new to this API, honestly. Please advise (see inline comments). Thanks in advance.

src/rpc/rawtransaction.cpp
756	My understanding is that the API is to allow for decimal values eg: 2.002 -- I guess we'll call that a 'numeric' in the API even though later down in the function it's basically treated as a string?
945	Can the value be 0? Because the code before, it was 0, and that apparently produced a bad tx. And it can't be negative (can it?)

Bump. I need more guidance/discussion here from @deadalnix and/or others. Thanks in advance..

deadalnix added inline comments.Aug 7 2017, 21:30

src/rpc/rawtransaction.cpp
756	It is a json value of type numeric.
945	Why do you care the value is ? Maybe you want to check for MoneyRange if you feel like it, but checking for zero specifically makes no sense. If you are checking the value, you need to do it properly.

deadalnix requested changes to this revision.Aug 7 2017, 23:31

This revision now requires changes to proceed.Aug 7 2017, 23:31

dgenr8 added inline comments.Aug 7 2017, 23:43

src/rpc/rawtransaction.cpp
756	See core https://github.com/bitcoin/bitcoin/commit/605e8473a7ddca13b24a4020c7bd630aa5d374e2
909	Core added redeemScript here but actually did not add amount (which is numeric).

Updated fix to include reviewer suggestions

@dgenr8 : From your github comment:

Yes, though I've only run it in test. It was previously failing on broadcast, because verification expects the actual amount (not 0) to be signed.

Should I check that the value not be zero in the C++ rpc server side, in addition to existing checks (1. that amount exists and 2. that it is a MoneyRange()) ?

Please advise, thanks.

deadalnix added inline comments.Aug 8 2017, 11:03

src/rpc/rawtransaction.cpp
912 ↗	(On Diff #1104)	It's not about being valid json. It a string. We expect a (floating point) number.

I think you screwed up something while updating the diff. Also please run autopep8 in another diff.

This revision now requires changes to proceed.Aug 8 2017, 12:21

In D441#7600, @deadalnix wrote:

I think you screwed up something while updating the diff. Also please run autopep8 in another diff.

autopep8 was automatically run for me by arcanist on all python files I touched.

What did I screw up? I don't understand ...

CCulianu abandoned this revision.Aug 8 2017, 15:06

Revision Contents
Changeset List

Path

Size

src/

rpc/

rawtransaction.cpp

13 lines

test/

rpc_tests.cpp

40 lines

Diff 1101

View Options

src/rpc/rawtransaction.cpp