Page MenuHomePhabricator

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

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 691
Build 691: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Aug 7 2017, 20:57
deadalnix added inline comments.
src/rpc/rawtransaction.cpp
756 ↗(On Diff #1101)

Please do not change the API.

945 ↗(On Diff #1101)

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 ↗(On Diff #1101)

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 ↗(On Diff #1101)

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?)

CCulianu requested review of this revision.Aug 7 2017, 21:13
CCulianu edited edge metadata.

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

src/rpc/rawtransaction.cpp
756 ↗(On Diff #1101)

It is a json value of type numeric.

945 ↗(On Diff #1101)

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
src/rpc/rawtransaction.cpp
756 ↗(On Diff #1101)
909 ↗(On Diff #1101)

Core added redeemScript here but actually did not add amount (which is numeric).

CCulianu edited edge metadata.

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.

src/rpc/rawtransaction.cpp
912

It's not about being valid json. It a string. We expect a (floating point) number.

deadalnix requested changes to this revision.Aug 8 2017, 12:21

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

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 ...